gh-131525: Cache the result of tuple_hash

[mdboom]

Back in 2013, it was determined that caching the result of tuple_hash did not have any significant speedup.

However, a lot has changed since then, and in a recent experiment to add a tuple hash cache back in, the mdp benchmark increased by 86%.

Admittedly, there was no measurable improvement on any other benchmark, but it also seems to have no downside, including for memory usage when measured with max_rss.

• Issue: Caching the tuple hash calculation speeds up some code significantly #131525

[gpshead]

is there any reason not to just move this into tuple_alloc() itself instead of repeating it everywhere tuple_alloc() is called?

[mdboom]

I don't personally have an objection. Most *_alloc functions only do allocation, not initialization, so I didn't want to confuse that. But the cached hash is kind of a special case...

[markshannon]

You could set the hash of () statically in pycore_runtime_init.h, then this test would be unnecessary.

[mdboom]

Yes, but interestingly, this is measurably faster -- it doesn't have to chase the pointer.

[mdboom]

Seems like the difference here is in the noise, so might as well do this at build time and skip this extra branch.

[picnixz]

Just cosmetics nits and I think it's good.

Note: it's also possible to use a static inline function instead for _PyTuple_RESET_HASH_CACHE, but that would mean that *op needs to be a PyObject but I think not all calls pass a true PyObject.

[kumaraditya303]

The loads of ob_hash need relaxed memory ordering i.e. use FT_ATOMIC_LOAD_SSIZE_RELAXED

[mdboom]

Still learning the free-threaded conventions. Can you explain why (or point me at an explanation)? Since the write is atomic and we don't care if there is a race (worst case, the hash gets recomputed), and the thread sanitizer CI isn't complaining, I don't see why this would be necessary -- but also I'm new to this stuff so maybe I'm missing something.

[markshannon]

The FT_... macros translate to plain loads/stores on the default build, so there is no overhead there.

It is necessary because accessing a field from multiple threads without any kind of synchronization is undefined behavior.
Even on the free-threaded build the RELAXED part means that they is no synchronization at the hardware level on most platforms, it just convinces the compiler and sanitizers that this is OK.

[bedevere-app]

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

[mdboom]

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

@picnixz, @kumaraditya303: please review the changes made to this pull request.

[brandtbucher]

Now that all of these places have been replaced with a function call to _PyTuple_Recycle, is that sufficient, or would you prefer that these comments remain?

We should probably just remove the old comments and keep one in _PyTuple_Recycle, in my opinion.

[vstinner]

This PR changes 102 files, it's hard to review. Most files are generated by Argument Clinic. Would it be possible to have a first PR which only adds the ob_hash member? So the second PR would only contain the interesting changes, making it easier to review.

[mdboom]

This PR changes 102 files, it's hard to review. Most files are generated by Argument Clinic. Would it be possible to have a first PR which only adds the ob_hash member? So the second PR would only contain the interesting changes, making it easier to review.

GitHub seems to be doing the right thing by hiding the generated changes. Is that not happening for you?

I'd be happy to rearrange the commits, but I don't think merging a PR if the second one may not be approved makes sense. Let me know, as that would also invalidate the existing review comments on this PR.