On Tue, Jan 9, 2024 at 9:40 AM Masahiko Sawada <sawada.m...@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.
