← 返回首页
[3.11] gh-93382: Cache result of `PyCode_GetCode` in codeobject (GH-93383) by Fidget-Spinner · Pull Request #93493 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

[3.11] gh-93382: Cache result of PyCode_GetCode in codeobject (GH-93383)#93493

Merged
pablogsal merged 8 commits into
python:3.11from
Fidget-Spinner:co_code_cached_backport
Jun 23, 2022
Merged

[3.11] gh-93382: Cache result of PyCode_GetCode in codeobject (GH-93383)#93493
pablogsal merged 8 commits into
python:3.11from
Fidget-Spinner:co_code_cached_backport

Conversation

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

Copy link
Copy Markdown
Member

Fidget-Spinner and others added 2 commits June 4, 2022 20:13
pythongh-93382: Cache result of PyCode_GetCode in codeobject (pytho… …
13642ce
…nGH-93383) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
Update test_frozenmain.h
0f9511f
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 mentioned this pull request Jun 4, 2022
Merged

Fidget-Spinner commented Jun 4, 2022 •
edited
Loading

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.

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.

brandtbucher reviewed Jun 4, 2022
View reviewed changes

brandtbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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:

Comment thread Include/cpython/code.h Outdated Show resolved Hide resolved
Comment thread Objects/codeobject.c Show resolved Hide resolved

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.

Fidget-Spinner commented Jun 5, 2022 •
edited
Loading

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.

brandtbucher reviewed Jun 8, 2022
View reviewed changes
Comment thread Programs/test_frozenmain.h Show resolved Hide resolved

brandtbucher commented Jun 8, 2022 •
edited
Loading

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?

Fidget-Spinner commented Jun 9, 2022 •
edited
Loading

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.

markshannon requested changes Jun 10, 2022
View reviewed changes
Comment thread Include/cpython/code.h Outdated Show resolved Hide resolved
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-05-31-16-36-30.gh-issue-93382.Jf6gAj.rst Outdated Show resolved Hide resolved

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.

kumaraditya303 mentioned this pull request Jun 11, 2022
Closed
Fix memory leak in deepfrozen code objects …
be1baad
Co-Authored-By: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
nedbat mentioned this pull request Jun 12, 2022
Open

Fidget-Spinner commented Jun 23, 2022

Copy link
Copy Markdown
Member Author

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

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.

bedevere-bot requested a review from markshannon June 23, 2022 12:11
markshannon approved these changes Jun 23, 2022
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Looks good.

Fidget-Spinner requested a review from pablogsal June 23, 2022 12:41
kumaraditya303 approved these changes Jun 23, 2022
View reviewed changes

kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

LGTM

pablogsal merged commit 852b4d4 into python:3.11 Jun 23, 2022
Fidget-Spinner mentioned this pull request Jun 24, 2022
Merged
Fidget-Spinner deleted the co_code_cached_backport branch June 24, 2022 17:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Footer

© 2026 GitHub, Inc.