8000 gh-111928: make "memo" dict local to scan_once call by aisk · Pull Request #112005 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-111928: make "memo" dict local to scan_once call #112005

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 13, 2023
Merged

gh-111928: make "memo" dict local to scan_once call #112005

merged 5 commits into from
Nov 13, 2023

Conversation

aisk
Copy link
Contributor
@aisk aisk commented Nov 12, 2023 •
edited by bedevere-app bot
Loading

@aisk aisk changed the title Cherry pick json nogil gh-111928: cherry pick json nogil Nov 12, 2023
@bedevere-app bedevere-app bot mentioned this pull request Nov 12, 2023
Closed
@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?

corona10
corona10 reviewed Nov 12, 2023
View reviewed changes
Copy link
Member
@corona10 corona10 left a comment •
edited
Loading

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.

Misc/NEWS.d/next/Library/2023-11-12-21-39-36.gh-issue-111928.SiYBqt.rst Outdated
@@ -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
colesbury
colesbury approved these changes Nov 13, 2023
8000 View reviewed changes
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
@aisk @colesbury
pythongh-111928: make "memo" dict local to scan_once call (pythongh-1…
1758976 
…12005)


Co-authored-by: Sam Gross <colesbury@gmail.com>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@aisk @colesbury @Glyphack
pythongh-111928: make "memo" dict local to scan_once call (pythongh-1…
68ce15f 
…12005)


Co-authored-by: Sam Gross <colesbury@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colesbury colesbury colesbury approved these changes

@corona10 corona10 corona10 approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aisk @Fidget-Spinner @colesbury @corona10
0