On Tue, Jan 9, 2024 at 9:40 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> In addition, I've made some changes and cleanups:
These look good to me, although I have not tried dumping a node in a while.
> 0011 - simplify the radix tree iteration code. I hope it makes the
> code clear and readable. Also I removed RT_UPDATE_ITER_STACK().
I'm very pleased with how much simpler it is now!
> 0013 - In RT_SHMEM case, we use SIZEOF_VOID_P for
> RT_VALUE_IS_EMBEDDABLE check, but I think it's not correct. Because
> DSA has its own pointer size, SIZEOF_DSA_POINTER, it could be 4 bytes
> even if SIZEOF_VOID_P is 8 bytes, for example in a case where
> !defined(PG_HAVE_ATOMIC_U64_SUPPORT). Please refer to dsa.h for
> details.
Thanks for the pointer. ;-)
> BTW, now that the inner and leaf nodes use the same structure, do we
> still need RT_NODE_BASE_XXX types? Most places where we use
> RT_NODE_BASE_XXX types can be replaced with RT_NODE_XXX types.
That's been in the back of my mind as well. Maybe the common header
should be the new "base" member? At least, something other than "n".
> Exceptions are RT_FANOUT_XX calculations:
>
> #if SIZEOF_VOID_P < 8
> #define RT_FANOUT_16_LO ((96 - sizeof(RT_NODE_BASE_16)) / sizeof(RT_PTR_ALLOC))
> #define RT_FANOUT_48 ((512 - sizeof(RT_NODE_BASE_48)) / sizeof(RT_PTR_ALLOC))
> #else
> #define RT_FANOUT_16_LO ((160 - sizeof(RT_NODE_BASE_16)) / sizeof(RT_PTR_ALLOC))
> #define RT_FANOUT_48 ((768 - sizeof(RT_NODE_BASE_48)) / sizeof(RT_PTR_ALLOC))
> #endif /* SIZEOF_VOID_P < 8 */
>
> But I think we can replace them with offsetof(RT_NODE_16, children) etc.
That makes sense. Do you want to have a go at it, or shall I?
I think after that, the only big cleanup needed is putting things in a
more readable order. I can do that at a later date, and other
opportunities for beautification are pretty minor and localized.
Rationalizing locking is the only thing left that requires a bit of thought.
