On Tue, 7 Jun 2022 at 03:09, Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Let's restart this, to simplify the review and commit work.
The patchset fails to apply. Could you send an updated version that
applies to the current master branch?
> Regarding the patches now, we have:
> 1) v1-001-aset-reduces-memory-consumption.patch
> Reduces memory used by struct AllocBlockData by minus 8 bits,
This seems reasonable, considering we don't generally use the field
for anything but validation.
> reducing the total size to 32 bits, which leads to "fitting" two structs in a 64bit cache.
By bits, you mean bytes, right?
Regarding fitting 2 structs in 64 bytes, that point is moot, as each
of these structs are stored at the front of each malloc-ed block, so
you will never see more than one of these in the same cache line. Less
space used is nice, but not as critical there IMO.
> Remove tests elog(ERROR, "could not find block containing chunk %p" and
> elog(ERROR, "could not find block containing chunk %p", moving them to
> MEMORY_CONTEXT_CHECKING context.
>
> Since 8.2 versions, nobody complains about these tests.
>
> But if is not acceptable, have the option (3) v1-003-aset-reduces-memory-consumption.patch
>
> 2) v1-002-generation-reduces-memory-consumption.patch
> Reduces memory used by struct GenerationBlock, by minus 8 bits,
That seems fairly straight-forward -- 8 bytes saved on each page isn't
a lot, but it's something.
> reducing the total size to 32 bits, which leads to "fitting" two structs in a 64bit cache.
Your size accounting seems wrong. On 64-bit architectures, we have
dlist_node (=16) + Size (=8) + 2*int (=8) + 2 * (char*) (=16) = 48
bytes. Shaving off the Size field reduces that by 8 bytes to 40 bytes.
The argument of fitting 2 of these structures into one cache line is
moot again, because here, too, two of this struct will not share a
cache line (unless somehow we allocate 0-sized blocks, which would be
a bug).
> 3) v1-003-aset-reduces-memory-consumption.patch
> Same to the (1), but without remove the tests:
> elog(ERROR, "could not find block containing chunk %p" and
> elog(ERROR, "could not find block containing chunk %p",
> But at the cost of removing a one tiny part of the tests and
> moving them to MEMORY_CONTEXT_CHECKING context.
I like this patch over 001 due to allowing less corruption to occur in
the memory context code. This allows for detecting some issues in 003,
as opposed to none in 001.
Kind regards,
Matthias van de Meent