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-95385 Fastpath for encoding dict to JSON by aivarsk · Pull Request #95374 · python/cpython

Conversation

@aivarsk

@aivarsk aivarsk commented Jul 28, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

When sorting of keys is not requested and we are encoding a dict
use PyDict_Next to iterate over keys and values.
When sorting is requested use PyList_GET_ITEM instead of PyIter_Next because we know we are working with a list.

  • Issue: Fastpath for encoding unsorted dict to JSON #95385

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the awaiting review label Jul 28, 2022
@aivarsk
aivarsk force-pushed the dict_to_json_fastpath branch 2 times, most recently from 4d40abf to e2929e7 Compare July 28, 2022 13:11
@aivarsk aivarsk changed the title gh-NNNNN Fastpath for encoding dict to JSON gh-95385 Fastpath for encoding dict to JSON Jul 28, 2022
@aivarsk

aivarsk commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

The code is based on another PR (gh-95382) not merged yet just to avoid passing extra parameters around.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aivarsk
aivarsk marked this pull request as ready for review July 28, 2022 14:30
corona10
corona10 requested changes Jul 28, 2022
View reviewed changes

@corona10 corona10 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.

Please separate the patch from removing indent_level params.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting review label Jul 28, 2022
@bedevere-bot

bedevere-bot commented Jul 28, 2022

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jul 28, 2022
aivarsk added 2 commits July 28, 2022 23:22
@aivarsk
Fastpath for encoding dict to JSON
c7b5cc2 
When sorting of keys is not requsted and we are encoding a dict
use PyDict_Next to iterate over keys and values.
Leave the old path with PyMapping_Items that creates a list of
key-value tuples for cases when sorting is requested.
Don't use mapping items and iterator: we check and know we are
using a dict and list.
@aivarsk
NEWS
9137b1b
@aivarsk
aivarsk force-pushed the dict_to_json_fastpath branch from 6e92af6 to 9137b1b Compare July 28, 2022 20:23
@aivarsk

aivarsk commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Jul 28, 2022
@bedevere-bot

bedevere-bot commented Jul 28, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from corona10 July 28, 2022 20:25
corona10
corona10 reviewed Jul 29, 2022
View reviewed changes

@corona10 corona10 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.

Would you like to provide a benchmark script and result for the following conditions?

  • json.dumps when sorting of key is required
  • json.dumps when sorting of key is not required (your suggestion)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aivarsk

aivarsk commented Jul 29, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

This is without sorting of keys, the default behavior of json.dumps
pyperformance on my machine before changes:

### json_dumps ###
Mean +- std dev: 10.6 ms +- 0.1 ms

pyperformance after changes:

### json_dumps ###
Mean +- std dev: 9.00 ms +- 0.08 ms
All reactions

Sorry, something went wrong.

Uh oh!

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

aivarsk commented Jul 29, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

With pyperformance sort_keys=True, I edited benchmarks/bm_json_dumps/run_benchmark.py for that

before changes

### json_dumps ###
Mean +- std dev: 14.0 ms +- 0.2 ms

after changes (PyList_GET_ITEM is a bit faster than PyIter_Next?)

### json_dumps ###
Mean +- std dev: 13.6 ms +- 0.2 ms
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aivarsk
aivarsk commented Jul 29, 2022
View reviewed changes
Comment thread Modules/_json.c Resolved

Uh oh!

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

corona10 commented Jul 29, 2022

Copy link
Copy Markdown
Member

@aivarsk Thank you, I will take a look at this PR through this weekend :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 self-assigned this Jul 29, 2022
@mdboom mdboom added the performance Performance or resource usage label Aug 1, 2022
@corona10 corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 3, 2022
@bedevere-bot

bedevere-bot commented Aug 3, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 9137b1b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 3, 2022
corona10
corona10 reviewed Aug 3, 2022
View reviewed changes
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 reviewed Aug 3, 2022
View reviewed changes
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
aivarsk added 2 commits August 3, 2022 20:34
@aivarsk
explicit checks
751e042
@aivarsk
smaller diff
8ae1d21
@aivarsk
aivarsk requested a review from corona10 August 4, 2022 09:43
corona10
corona10 reviewed Aug 5, 2022
View reviewed changes
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 reviewed Aug 5, 2022
View reviewed changes
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aivarsk
Use PyDict_Items
7677295
corona10
corona10 approved these changes Aug 5, 2022
View reviewed changes

@corona10 corona10 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.

Sorry for the delay, I should check several possibilities of regression including leaks but not found :)

Overall looks good LGTM, but please follow PEP 7 if possible.
I just left nit comments :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_json.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Aug 5, 2022
aivarsk and others added 4 commits August 5, 2022 23:03
@aivarsk @corona10
Update Modules/_json.c
a21f77a 
Co-authored-by: Dong-hee Na <[email protected]>
@aivarsk @corona10
Update Modules/_json.c
4734d70 
Co-authored-by: Dong-hee Na <[email protected]>
@aivarsk @corona10
Update Modules/_json.c
79e6880 
Co-authored-by: Dong-hee Na <[email protected]>
@aivarsk @corona10
Update Modules/_json.c
fda2ee7 
Co-authored-by: Dong-hee Na <[email protected]>
@corona10
corona10 merged commit 15f4a35 into python:main Aug 6, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 6, 2022
iritkatriel pushed a commit to iritkatriel/cpython that referenced this pull request Aug 11, 2022
@aivarsk @iritkatriel
pythongh-95385 Fastpath for encoding dict to JSON (pythongh-95374)
2d2e23a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes

Uh oh!

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

Assignees

@corona10 corona10

Labels

performance Performance or resource usage

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

@aivarsk @bedevere-bot @corona10 @mdboom
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.