On Fri, Jan 26, 2024 at 11:05 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Wed, Jan 24, 2024 at 3:42 PM John Naylor <johncnaylo...@gmail.com> wrote: > > > > On Tue, Jan 23, 2024 at 10:58 AM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > The new patches probably need to be polished but the VacDeadItemInfo > > > idea looks good to me. > > > > That idea looks good to me, too. Since you already likely know what > > you'd like to polish, I don't have much to say except for a few > > questions below. I also did a quick sweep through every patch, so some > > of these comments are unrelated to recent changes: > > Thank you! > > > > > +/* > > + * Calculate the slab blocksize so that we can allocate at least 32 chunks > > + * from the block. > > + */ > > +#define RT_SLAB_BLOCK_SIZE(size) \ > > + Max((SLAB_DEFAULT_BLOCK_SIZE / (size)) * (size), (size) * 32) > > > > The first parameter seems to be trying to make the block size exact, > > but that's not right, because of the chunk header, and maybe > > alignment. If the default block size is big enough to waste only a > > tiny amount of space, let's just use that as-is. > > Agreed. >
As of v55 patch, the sizes of each node class are: - node4: 40 bytes - node16_lo: 168 bytes - node16_hi: 296 bytes - node48: 784 bytes - node256: 2088 bytes If we use SLAB_DEFAULT_BLOCK_SIZE (8kB) for each node class, we waste (approximately): - node4: 32 bytes - node16_lo: 128 bytes - node16_hi: 200 bytes - node48: 352 bytes - node256: 1928 bytes We might want to calculate a better slab block size for node256 at least. > > > > + * TODO: The caller must be certain that no other backend will attempt to > > + * access the TidStore before calling this function. Other backend must > > + * explicitly call TidStoreDetach() to free up backend-local memory > > associated > > + * with the TidStore. The backend that calls TidStoreDestroy() must not > > call > > + * TidStoreDetach(). > > > > Do we need to do anything now? > > No, will remove it. > I misunderstood something. I think the above statement is still true but we don't need to do anything at this stage. It's a typical usage that the leader destroys the shared data after confirming all workers are detached. It's not a TODO but probably a NOTE. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
