Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-112075: refactor dictionary lookup functions for better re-usability #114629

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

DinoV
Copy link
Contributor

@DinoV DinoV commented Jan 27, 2024

Refactor dictionary lookup functions for better usability

Free threaded builds of Python will need to expand upon the lookup functions to have thread-safe versions of them that can be run without the dictionary being locked. These lookups will just be subtly different - they'll use atomic loads and they'll need to contain some extra checks for values that may change in flight.

Currently there are 3 loookup functions so for free-threaded builds we'll need to get 6 of them. That's going to be a good amount of copy and pasting with sometimes subtle differences. So I'm curious if people would prefer an approach like this as opposed to the copy and pasting. I don't want to use macros as that'd destroy debugging. If we're concerned about perf implications for non-mainstream compilers or don't like this approach I can just go ahead with copy and pasting.

This factors these functions into one core function which contains the loop, and 3 comparison helper functions which implement the different comparisons. In free-threaded versions we'll get modified versions of these comparisons as that's where the differences will be.

This still generates nearly identical code in LTO builds, here's the before https://pastebin.com/UZdhKc6f and the after: https://pastebin.com/sdhcFZP5. Even in non-LTO builds the code is remarkably similar (with the new code maybe resembling the LTO code a little bit more as for some reason the non-LTO code gets a weird jmp at the beginning).

There is technically one change here in that the loop unrolling has been applied to all versions of the code, although we could easily have 2 versions of the loop function with one unrolled and one not-unrolled.

Perf seems to be be mostly neutral to me: https://pastebin.com/LPJYZAj4

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

A few comments below, but overall looks good to me.

Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
@DinoV DinoV merged commit 0990d55 into python:main Jan 30, 2024
30 checks passed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…sability (python#114629)

Refactor dict lookup functions to use force inline helpers
@DinoV DinoV deleted the nogil_dict_refactor_lookup branch May 31, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants