On Fri, 1 Apr 2022 at 10:50, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
>
> On Fri, 1 Apr 2022 at 07:38, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Thu, Mar 31, 2022 at 12:09:35PM +0200, Matthias van de Meent wrote:
> > > PageInit MAXALIGNs the size of the special area that it receives as an
> > > argument; so any changes to the page header that would misalign the
> > > value would be AM-specific; in which case it is quite unlikely that
> > > this is the right accessor for your page's special area.
> >
> > Right. I'd still be tempted to keep that per-AM rather than making
> > the checks deeper with one extra macro layer in the page header or
> > with a different macro that would depend on the opaque type, though.
> > Like in the attached, for example.
I noticed that you committed the equivalent of 0002 and 0003, thank you.
Here's a new 0001 to keep CFBot happy.
> I see. I still would like it better if the access could use this
> statically determined offset: your opaque-macros.patch doesn't fix the
> out-of-bound read/write scenariofor non-assert builds, nor does it
> remove the unneeded indirection through the page header that I was
> trying to remove.
>
> Even in assert-enabled builds; with my proposed changes the code paths
> for checking the header value and the use of the special area can be
> executed independently, which allows for parallel (pre-)fetching of
> the page header and special area, as opposed to the current sequential
> load order due to the required use of pd_special.
As an extra argument for this; it removes the need for a temporary
variable on the stack; as now all accesses into the special area can
be based on offsets off the base page pointer (which is also used
often). This would allow the compiler to utilize the available
registers more effectively.
-Matthias
PS. I noticed that between PageGetSpecialPointer and
PageGetSpecialOpaque the binary size was bigger by ~1kb for the
*Opaque variant when compiled with `CFLAGS="-o2" ./configure
--enable-debug`; but that varied a lot between builds and likely
related to binary layouts. For similar builds with `--enable-cassert`,
the *Opaque build was slimmer by ~ 50kB, which is a suprisingly large
amount.