GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111545: Add Py_HashPointer() function by vstinner · Pull Request #112096 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Nov 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Keep _Py_HashPointer() function as an alias to PyHash_Pointer().
  • Add tests on PyHash_Pointer() to test_capi.test_hash.
  • Remove _Py_HashPointerRaw() function: inline code in _Py_hashtable_hash_ptr().
  • Issue: Make _Py_HashDouble public again as "unstable" API #111545

📚 Documentation preview 📚: https://cpython-previews--112096.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot mentioned this pull request Nov 15, 2023
Make _Py_HashDouble public again as "unstable" API #111545
Closed
@vstinner vstinner mentioned this pull request Nov 15, 2023
gh-111545: Add PyHash_Double() function #112095
Closed
@vstinner
vstinner force-pushed the hash_pointer branch from eaf1414 to 3a0752a Compare November 15, 2023 02:55
@vstinner

vstinner commented Nov 15, 2023

Copy link
Copy Markdown
Member Author

I checked how _Py_rotateright_uintptr() is compiled on x86-64:

  • clang -O3 with __builtin_rotateright64(): ror rax, cl
  • gcc -O3 with (x >> bits) | (x << (8 * SIZEOF_UINTPTR_T - bits)): ror rbx,cl

As expected, the ROR instruction is used in both cases.


I used gdb: in Python, I run import _testinternalcapi, and then in gdb I type disassemble rotateright_uintptr.

For that, I made the following change. The check_rotateright_uintptr() cannot be used to check the machine code: since check_rotateright_uintptr() arguments are constant, GCC and clang just pre-compute the function result at build. The funciton is no longer called at runtime.

Change:

diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c
index 2d888e0ae2c..a08b3dfb168 100644
--- a/Modules/_testinternalcapi.c
+++ b/Modules/_testinternalcapi.c
@@ -290,6 +290,23 @@ test_rotateright_uintptr(PyObject *self, PyObject *Py_UNUSED(args))
 }
 
 
+static PyObject*
+rotateright_uintptr(PyObject *self, PyObject *args)
+{
+    PyObject *obj;
+    int bits;
+    if (!PyArg_ParseTuple(args, "O!i", &PyLong_Type, &obj, &bits)) {
+        return NULL;
+    }
+    unsigned long long x = PyLong_AsUnsignedLongLong(obj);
+    if (x == (unsigned long long)-1 && PyErr_Occurred()) {
+        return NULL;
+    }
+    uintptr_t y = _Py_rotateright_uintptr((uintptr_t)x, bits);
+    return PyLong_FromUnsignedLongLong(y);
+}
+
+
 #define TO_PTR(ch) ((void*)(uintptr_t)ch)
 #define FROM_PTR(ptr) ((uintptr_t)ptr)
 #define VALUE(key) (1 + ((int)(key) - 'a'))
@@ -1689,6 +1706,7 @@ static PyMethodDef module_functions[] = {
     {"test_popcount", test_popcount, METH_NOARGS},
     {"test_bit_length", test_bit_length, METH_NOARGS},
     {"test_rotateright_uintptr", test_rotateright_uintptr, METH_NOARGS},
+    {"rotateright_uintptr", rotateright_uintptr, METH_VARARGS},
     {"test_hashtable", test_hashtable, METH_NOARGS},
     {"get_config", test_get_config, METH_NOARGS},
     {"set_config", test_set_config, METH_O},
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the hash_pointer branch from 5b19577 to d132eb6 Compare November 15, 2023 11:12
@vstinner
vstinner marked this pull request as ready for review November 15, 2023 11:12
@vstinner
vstinner requested review from a team and tiran as code owners November 15, 2023 11:12
@bedevere-app bedevere-app Bot added the awaiting core review label Nov 15, 2023
@vstinner
vstinner force-pushed the hash_pointer branch 4 times, most recently from 992c39e to f63b726 Compare November 15, 2023 11:33
@vstinner

vstinner commented Nov 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I checked how _Py_rotateright_uintptr() is compiled on x86-64

Adding _Py_rotateright_uintptr() static inline function added too much code, and it's not needed: current code is already compiled to the expected most efficient x86-64 ROR instruction. I removed the _Py_rotateright_uintptr() function.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Nov 15, 2023
gh-112026: Restore removed private C API #112115
Merged
Consider restoring _PyHASH_INF/BITS/MODULUS/IMAG as public #111389
Closed
Functions which cannot fail capi-workgroup/api-evolution#43
Closed
vstinner
vstinner commented Nov 28, 2023
View reviewed changes
Comment thread Doc/c-api/hash.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

encukou
encukou reviewed Nov 28, 2023
View reviewed changes
Comment thread Python/pyhash.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

encukou
encukou reviewed Nov 28, 2023
View reviewed changes
Comment thread PC/pyconfig.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the hash_pointer branch from f63b726 to fb08414 Compare December 1, 2023 14:37
@vstinner

vstinner commented Dec 1, 2023

Copy link
Copy Markdown
Member Author

Changes:

  • PR rebased on main.
  • Rename the function to Py_HashPointer() following gh-111545: Add PyHash_Double() function #112095 (comment) discussion.
  • Keep _Py_HashPointerRaw() as a static inline function to avoid code duplication.
  • No longer use SIZEOF_UINTPTR_T, but sizeof().
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the hash_pointer branch from fb08414 to ffbdae1 Compare December 1, 2023 14:44
@vstinner

vstinner commented Dec 1, 2023

Copy link
Copy Markdown
Member Author

@encukou: Please review the updated PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the hash_pointer branch from ffbdae1 to cd8849b Compare December 1, 2023 14:53
@vstinner vstinner changed the title gh-111545: Add PyHash_Pointer() function gh-111545: Add Py_HashPointer() function Dec 1, 2023
encukou
encukou approved these changes Dec 1, 2023
View reviewed changes

@encukou encukou left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, thank you for adding it!

I hope Serhiy is OK with this iteration. (And that the rest of the C-API WG won't have major objections, but Idon't think that's likely.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/internal/pycore_pyhash.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Dec 1, 2023
@vstinner

vstinner commented Dec 1, 2023

Copy link
Copy Markdown
Member Author

@serhiy-storchaka: Do you want to review this change?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the hash_pointer branch from 136b536 to 8031210 Compare December 1, 2023 16:09
@vstinner

vstinner commented Dec 1, 2023

Copy link
Copy Markdown
Member Author

I rebased the PR on main to get a NoGIL fix.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Dec 1, 2023

Copy link
Copy Markdown
Member Author

@encukou:

(And that the rest of the C-API WG won't have major objections, but Idon't think that's likely.)

PEP 731 says:

The working group may operate primarily through reviews of GitHub issues and PRs.

Are you suggesting that the C API cannot be modified anymore unless whole C API Working Group approve a change? Or are you suggesting that the C API Working Group must review all C API additions? I don't think that was the plan. Well, the plan is not well defined.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Dec 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Are you suggesting that the C API cannot be modified anymore unless whole C API Working Group approve a change?

No, I'm saying that the others might disagree -- and that I don't think it's very likely that they will disagree.
I'd optimistically merge this (edit: after checking with Serhiy), but be prepared to fix it up or revert.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba
zooba reviewed Dec 4, 2023
View reviewed changes

@zooba zooba left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concerns from me, other than the lack of docs.

Adding new C API should include the WG up until we've developed and agreed upon the guidelines for adding new C API. Without those, yeah, we should all get pinged for any new APIs. (And if that takes up all our time and we don't get a chance to develop guidelines, so be it...)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/c-api/hash.rst Outdated

.. c:function:: Py_hash_t Py_HashPointer(const void *ptr)

Hash a pointer.

@zooba zooba Dec 4, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a bit more text to clarify that it ensures the pointer itself is usable as a hash value, and does not even attempt to access the memory the pointer refers to (let alone trying to call __hash__ or similar)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner Dec 5, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completed the doc. Is it more explicit? I clarified that only the pointer value is hashed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@encukou encukou mentioned this pull request Dec 5, 2023
Add Py_HashPointer capi-workgroup/decisions#1
Closed
vstinner added 3 commits December 5, 2023 18:48
@vstinner
pythongh-111545: Add Py_HashPointer() function
c0356a2
* Implement _Py_HashPointerRaw() as a static inline function.
* Add Py_HashPointer() tests to test_capi.test_hash.
* Keep _Py_HashPointer() function as an alias to Py_HashPointer().
@vstinner
Enhance the doc
462c5e8
@vstinner
Declare _Py_HashPointer() as an alias to Py_HashPointer()
5415271
@vstinner
vstinner force-pushed the hash_pointer branch from 8031210 to 5415271 Compare December 5, 2023 17:53
@vstinner

vstinner commented Dec 5, 2023

Copy link
Copy Markdown
Member Author

Adding new C API should include the WG up until we've developed and agreed upon the guidelines for adding new C API.

@encukou created capi-workgroup/decisions#1

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Dec 6, 2023
View reviewed changes
Comment thread Doc/c-api/hash.rst
``uintptr_t`` internally). The pointer is not dereferenced.

The function cannot fail: it cannot return ``-1``.

@iritkatriel iritkatriel Dec 6, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing some mention of the use case for this (not necessarily here). The issue talks about _Py_HashDouble, there is no mention of Py_HashPointer.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner

vstinner commented Dec 6, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@iritkatriel:

I'm missing some mention of the use case for this (not necessarily here). The issue talks about _Py_HashDouble, there is no mention of Py_HashPointer.

Proposed Py_HashDouble() API requires the caller to call Py_HashPointer(obj) for NaN: see #112449

Moreover, I removed the private function _Py_HashPointer() in Python 3.13 alpha1 and it broke a few projects. A code search on PyPI top 5,000 projects found 4 projects:

  • cffi (1.16.0)
  • gmpy2 (2.1.5)
  • numba (0.58.1)
  • tree_sitter (0.20.4)

Finally, igraph-0.11.2 contains a copy of the private _Py_HashPointer() function in src/_igraph/pyhelpers.c.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner merged commit 828451d into python:main Dec 6, 2023
@vstinner
vstinner deleted the hash_pointer branch December 6, 2023 14:09
@bedevere-app bedevere-app Bot removed the awaiting merge label Dec 6, 2023
@vstinner

vstinner commented Dec 6, 2023

Copy link
Copy Markdown
Member Author

I merged my PR. The API was approved by the C API Working Group: capi-workgroup/decisions#1

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@vstinner @aisk
pythongh-111545: Add Py_HashPointer() function (python#112096)
7569396
* Implement _Py_HashPointerRaw() as a static inline function.
* Add Py_HashPointer() tests to test_capi.test_hash.
* Keep _Py_HashPointer() function as an alias to Py_HashPointer().
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@vstinner @Glyphack
pythongh-111545: Add Py_HashPointer() function (python#112096)
c305506
* Implement _Py_HashPointerRaw() as a static inline function.
* Add Py_HashPointer() tests to test_capi.test_hash.
* Keep _Py_HashPointer() function as an alias to Py_HashPointer().
@coderabbitai coderabbitai Bot mentioned this pull request Jul 30, 2026
wasm/jit backend fixes, blackhole and frame-state repairs, and 3.14 interpreter surface parity pyre-lang/pyre#890
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel left review comments
@zooba zooba zooba left review comments
@encukou encukou encukou approved these changes
@tiran tiran Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@vstinner @encukou @iritkatriel @zooba
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.