On Mon, 2024-04-08 at 21:29 +0900, Masahiko Sawada wrote:
> For v17, changes for #2 are smaller, but I'm concerned
> that the new API that requires a hash function to be able to use
> binaryheap_update_{up|down} might not be user friendly.
The only API change in 02 is accepting a hash callback rather than a
boolean in binaryheap_allocate(), so I don't see that as worse than
what's there now. It also directly fixes my complaint (hashing the
pointer) and does nothing more, so I think it's the right fix for now.
I do think that the API can be better (templated like simplehash), but
I don't think right now is a great time to change it.
> When it comes to performance overhead, I mentioned that there is some
> regression in the current binaryheap even without indexing.
As far as I can tell, you are just adding a single branch in that path,
and I would expect it to be a predictable branch, right?
Thank you for testing, but small differences in a microbenchmark aren't
terribly worrying for me. If other call sites are that sensitive to
binaryheap performance, the right answer is to have a templated version
that would not only avoid this unnecessary branch, but also inline the
comparator (which probably matters more).
Regards,
Jeff Davis