GH-137759:Limit _PyObject_HashFast to dicts keys, rename it, and mark it as Py_ALWAYS_INLINE

[JasonMendoza2008]

Context:

setobject.c used a mix of _PyObject_HashFast and PyObject_Hash. The consensus¹ was that _PyObject_HashFast should only really be used for dict keys (string-only sets are not so omnipresent). This PR uses the standard PyObject_Hash without the fast path (making set[Any] faster and set[str] slower²), renames _PyObject_HashFast since its name was misleading and marked it as Py_ALWAYS_INLINE to force inlining.

Pretty sure this doesn't require a NEWS entry?

• Issue: Shouldn't set_intersection use _PyObject_HashFast()? #137759

Footnotes

1. There were discussions in the same issue around removing _PyObject_HashFast completely and adding the fast path for strings (and even potentially integers) to PyObject_Hash but there was no consensus so I'm not including it for now. ↩

2. In this comment , I run pyperformance on two versions of setobject.c, one using HashFast everywhere and one using the standard Hash everywhere. None of these correspond to the current implementation of setobject.c since the current implementation uses a mix of both. Obviously in all cases, dictionaries use HashFast. ↩

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[ZeroIntensity]

making set[Any] faster and set[str] slower

I'm not totally convinced that this is true. Detecting the fast-path for strings is just one pointer comparison, which really won't have a noticeable performance impact. I agree that we want _PyObject_HashFast/_PyObject_HashDictKey for inlining purposes, but I don't see why the fast path has to be limited to it.

[JasonMendoza2008]

making set[Any] faster and set[str] slower

I'm not totally convinced that this is true. Detecting the fast-path for strings is just one pointer comparison, which really won't have a noticeable performance impact. I agree that we want _PyObject_HashFast/_PyObject_HashDictKey for inlining purposes, but I don't see why the fast path has to be limited to it.

I'm fine with adding a fast-path for strings and integers (but then do we even need _PyObject_HashFast/_PyObject_HashDictKey , we could just put everything in PyObject_Hash¹). I didn't since there was no consensus.

Footnotes

1. However, we probably can't inline that function since it's bigger? I'm not sure... I'm no expert in what's too big to be inlined. ↩

[ZeroIntensity]

but then do we even need HashFast, we could just put everything in PyObject_Hash

Yeah, because even if there's no speedup there, it's just simpler maintenance (in the future, we might want to make other assumptions about a dictionary key's hash that improve performance).

[JasonMendoza2008]

I'm not totally convinced that this is true. Detecting the fast-path for strings is just one pointer comparison, which really won't have a noticeable performance impact

Playing devil's advocate: if we don't detect it, we're a a function pointer dereference and call away from getting the hash anyway, right? The hash is not recomputed without that fast-path since it's stored (for strings, but for integers it's just an AND operation if I remember correctly)?

[ZeroIntensity]

A function pointer call has a lot more to do than just some reads, but maybe the overhead isn't noticeable?

[JasonMendoza2008]

A function pointer call has a lot more to do than just some reads, but maybe the overhead isn't noticeable?

The best thing to answer this without guessing is probably to compile both (or all three, if we also include a fast path for integers) and run pyperformance.

[JasonMendoza2008]

making set[Any] faster and set[str] slower

I'm not totally convinced that this is true. Detecting the fast-path for strings is just one pointer comparison, which really won't have a noticeable performance impact. I agree that we want _PyObject_HashFast/_PyObject_HashDictKey for inlining purposes, but I don't see why the fast path has to be limited to it.

Here is a comparison using pyperformance for what it's worth.

Pyperformance version: 1.11.0
Report on Linux-5.15.153.1-microsoft-standard-WSL2-x86_64-with-glibc2.39
Number of logical CPUs: 24

Run 1

Run 2 (using the -r option which is supposed to get more accurate results by running everything more)

Both versions were compiled with ./configure --enable-optimizations --with-lto