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 — [3.11] gh-93382: Cache result of `PyCode_GetCode` in codeobject (GH-93383) by Fidget-Spinner · Pull Request #93493 · python/cpython

Conversation

@Fidget-Spinner

@Fidget-Spinner Fidget-Spinner commented Jun 4, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

(cherry picked from commit d52ffc1)

  • PyCode_GetCode could be faster #93382
  • PyCode_GetCode could be faster #93382

Sorry, something went wrong.

Uh oh!

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

All reactions
Fidget-Spinner and others added 2 commits June 4, 2022 20:13
@Fidget-Spinner @kumaraditya303 @sweeneyde
pythongh-93382: Cache result of PyCode_GetCode in codeobject (pytho…
13642ce
…nGH-93383)

Co-authored-by: Kumar Aditya <[email protected]>
Co-authored-by: Dennis Sweeney <[email protected]>
@Fidget-Spinner
Update test_frozenmain.h
0f9511f
@Fidget-Spinner
Fidget-Spinner requested a review from markshannon as a code owner June 4, 2022 12:16
@bedevere-bot bedevere-bot added the awaiting core review label Jun 4, 2022
@Fidget-Spinner Fidget-Spinner changed the title [3.11] Cache result of PyCode_GetCode in codeobject (GH-93383) [3.11] gh-93382: Cache result of PyCode_GetCode in codeobject (GH-93383) Jun 4, 2022
@Fidget-Spinner Fidget-Spinner mentioned this pull request Jun 4, 2022
gh-93382: Cache result of PyCode_GetCode in codeobject #93383
Merged
@Fidget-Spinner
Fidget-Spinner requested a review from brandtbucher June 4, 2022 12:39
@Fidget-Spinner

Fidget-Spinner commented Jun 4, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@pablogsal and @nedbat

This PR improves coverage performance on 3.11 by 5-7%. Using Ned's benchmarks

# 3.11 branch at 1497d7fdefff8207b8ccde82e96b6f471416c284
Median for bm_sudoku.py, python3.11, cov=none: 10.476s
Median for bm_sudoku.py, python3.11, cov=6.4.1: 69.124s
Median for bm_spectral_norm.py, python3.11, cov=none: 9.072s
Median for bm_spectral_norm.py, python3.11, cov=6.4.1: 72.143s

# 3.11 co_code_cached
Median for bm_sudoku.py, python3.11, cov=none: 10.363s
Median for bm_sudoku.py, python3.11, cov=6.4.1: 64.726s
Median for bm_spectral_norm.py, python3.11, cov=none: 9.325s
Median for bm_spectral_norm.py, python3.11, cov=6.4.1: 69.713s

An observation: bm_spectral_norm improved less than bm_sudoku because the size of its co_code is smaller. So the cost of getting a new one every time isn't as high. I think real-world code sizes are more likely to be like bm_sudoku or larger. So I'd guestimate we will see ~10% improvement in real-world code running coverage in 3.11.

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal

pablogsal commented Jun 4, 2022

Copy link
Copy Markdown
Member

Unfortunately this means that whatever is making Python 3.11 slower with coverage, unfortunately is not only this :(

Great investigation @Fidget-Spinner and thanks for working on this ♥️

I will try to review ASAP but it would be great if @markshannon @iritkatriel @brandtbucher or @ericsnowcurrently can take a look.

Fidget-Spinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

brandtbucher
brandtbucher reviewed Jun 4, 2022
View reviewed changes

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

I'm on mobile right now (probably can't get to a computer today), but here are a few thoughts based on a first look:

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Include/cpython/code.h Resolved

Uh oh!

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

Comment thread Objects/codeobject.c Resolved

Uh oh!

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

@nedbat

nedbat commented Jun 5, 2022

Copy link
Copy Markdown
Member

I also ran the benchmarks using #93493:

cov proj python3.10 python3.11 gh93493 3.11 vs 3.10 gh93493 vs 3.10
none bug1339.py 0.193 s 0.155 s 0.143 s 0.803 0.743
none bm_sudoku.py 10.686 s 10.393 s 11.867 s 0.973 1.111
none bm_spectral_norm.py 16.051 s 10.940 s 10.987 s 0.682 0.684
6.4.1 bug1339.py 0.439 s 0.842 s 0.771 s 1.918 1.757
6.4.1 bm_sudoku.py 30.148 s 61.392 s 61.606 s 2.036 2.043
6.4.1 bm_spectral_norm.py 40.672 s 79.562 s 73.221 s 1.956 1.800

It's a slight improvement, but isn't solving the problem.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner
Address Brandt's review
56b017d
@Fidget-Spinner

Fidget-Spinner commented Jun 5, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

It's a slight improvement, but isn't solving the problem.

Yeah I'm aware. I sent this PR in because 10% improvement on macrobenchmarks is still something! Even cProfile slowed down by 60% when profiling code in 3.11. My hunch is that accessing the full PyFrameObject is signifcantly more expensive now in 3.11. However, I can't fix that because it's part of the tracing Py_tracefunc C API.

All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher
brandtbucher reviewed Jun 8, 2022
View reviewed changes
Comment thread Programs/test_frozenmain.h Resolved

Uh oh!

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

@brandtbucher

brandtbucher commented Jun 8, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I also ran the benchmarks using #93493:

It's a slight improvement, but isn't solving the problem.

Hm, it looks like in some cases this actually makes things a bit slower (perhaps due to the extra memory consumption)?

Maybe we should pause this PR until @markshannon has finished reworking the line number calculations, which seem to be the bulk of the issue at this point. Or at least see if it slows down pyperformance at all before merging?

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner

Fidget-Spinner commented Jun 9, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I also ran the benchmarks using #93493:

It's a slight improvement, but isn't solving the problem.

Hm, it looks like in some cases this actually makes things a bit slower (perhaps due to the extra memory consumption)?

Maybe we should pause this PR until @markshannon has finished reworking the line number calculations, which seem to be the bulk of the issue at this point. Or at least see if it slows down pyperformance at all before merging?

When I benchmarked on the main/3.12 branch, it didn't make anything slower #93383 (comment). However, I agree on waiting for a while.

When benchmarking with Ned's benchmarks, I saw a slowdown in bm_sudoku but no slowdown in bm_spectral_norm. I'm not sure what's up with that.

All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon
markshannon requested changes Jun 10, 2022
View reviewed changes
Comment thread Include/cpython/code.h Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Core and Builtins/2022-05-31-16-36-30.gh-issue-93382.Jf6gAj.rst Resolved

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting core review label Jun 10, 2022
@bedevere-bot

bedevere-bot commented Jun 10, 2022

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: 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 the awaiting changes label Jun 10, 2022
@Fidget-Spinner
Apply suggestions by Mark
5af4e76
@kumaraditya303 kumaraditya303 mentioned this pull request Jun 11, 2022
Memory leak when reading co_code attribute of deepfrozen code objects #93728
Closed
@Fidget-Spinner @kumaraditya303
Fix memory leak in deepfrozen code objects
be1baad
Co-Authored-By: Kumar Aditya <[email protected]>
@nedbat nedbat mentioned this pull request Jun 12, 2022
Severe performance degradation for tracing under 3.11 #93516
Open
Fidget-Spinner added 3 commits June 22, 2022 23:41
@Fidget-Spinner
Merge remote-tracking branch 'upstream/3.11' into co_code_cached_back…
c979855
…port
@Fidget-Spinner
add to deepfreeze.py
887120f
@Fidget-Spinner
make regen-abidump
b34659c
@Fidget-Spinner

Fidget-Spinner commented Jun 23, 2022

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again Mark.

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 Jun 23, 2022
@bedevere-bot

bedevere-bot commented Jun 23, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

@markshannon: 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 markshannon June 23, 2022 12:11
markshannon
markshannon approved these changes Jun 23, 2022
View reviewed changes

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

Looks good.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Jun 23, 2022
@Fidget-Spinner
Fidget-Spinner requested a review from pablogsal June 23, 2022 12:41
kumaraditya303
kumaraditya303 approved these changes Jun 23, 2022
View reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions
@pablogsal
pablogsal merged commit 852b4d4 into python:3.11 Jun 23, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 23, 2022
@Fidget-Spinner Fidget-Spinner mentioned this pull request Jun 24, 2022
gh-93382: Sync up co_code changes with 3.11 #94227
Merged
@Fidget-Spinner
Fidget-Spinner deleted the co_code_cached_backport branch June 24, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher left review comments
@markshannon markshannon markshannon approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@pablogsal pablogsal Awaiting requested review from pablogsal

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.

7 participants

@Fidget-Spinner @pablogsal @nedbat @brandtbucher @bedevere-bot @markshannon @kumaraditya303
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.