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-111928: make "memo" dict local to scan_once call #112005

Merged
merged 5 commits into from
Nov 13, 2023

Conversation

aisk
Copy link
Contributor

@aisk aisk commented Nov 12, 2023

@aisk aisk changed the title Cherry pick json nogil gh-111928: cherry pick json nogil Nov 12, 2023
@Fidget-Spinner
Copy link
Member

Thanks for the PR! This looks okay, but I worry there will be a performance regression. Do you know how much of a hit performance takes with this approach?

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Would you like to add @colesbury as co-author since the PR is cherry-picked from his own repo?

Oh you did already.

@@ -0,0 +1 @@
Make the ``memo`` dict thread safe without the GIL in :mod:`json`.
Copy link
Member

Choose a reason for hiding this comment

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

NEWS.d is not needed since there is no user side changes.

@colesbury colesbury changed the title gh-111928: cherry pick json nogil gh-111928: make "memo" dict local to scan_once call Nov 13, 2023
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.

This lgtm.

@Fidget-Spinner, performance impact is <1% for parsing tiny strings of JSON (and less for larger JSON strings).

@colesbury
Copy link
Contributor

Meant to link to a quick and dirty JSON benchmark https://gist.github.com/colesbury/dd888920a1eacb97794fb6be2446dc10:

@corona10 corona10 enabled auto-merge (squash) November 13, 2023 04:34
@corona10 corona10 merged commit d0058cb into python:main Nov 13, 2023
aisk added a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
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.

4 participants