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-93382: Cache result of `PyCode_GetCode` in codeobject by Fidget-Spinner · Pull Request #93383 · python/cpython

Conversation

@Fidget-Spinner

@Fidget-Spinner Fidget-Spinner commented May 31, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Fixes gh-93382.

Sorry, something went wrong.

Uh oh!

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

All reactions
@Fidget-Spinner
Cache result of PyCode_GetCode in codeobject
ed061f6
@Fidget-Spinner
Fidget-Spinner requested a review from markshannon as a code owner May 31, 2022 16:34
@bedevere-bot bedevere-bot added the awaiting core review label May 31, 2022
@blurb-it
📜🤖 Added by blurb_it.
1077bd3
@Fidget-Spinner

Fidget-Spinner commented May 31, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@markshannon do tell me if you feel that it isn't worth making PyCodeObject larger for this use case. I think it's worth it because it restores O(1) co_code access. Currently it's O(kn).

Benchmarks aren't available yet. I'll try to run it by the end of this week if no one beats me to it.

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner AA-Turner added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels May 31, 2022
@Fidget-Spinner

Fidget-Spinner commented Jun 1, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Quick n dirty microbenchmark:

py -3.10 -m timeit -s "import typing; c = typing.get_type_hints.__code__" "c.co_code"
10000000 loops, best of 5: 27.4 nsec per loop

py -3.11 -m timeit -s "import typing; c = typing.get_type_hints.__code__" "c.co_code"
500000 loops, best of 5: 555 nsec per loop

# This branch, release build without PGO
 .\PCbuild\amd64\python.exe -m timeit -s "import typing; c = typing.get_type_hints.__code__" "c.co_code"
10000000 loops, best of 5: 28.6 nsec per loop
All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner

Fidget-Spinner commented Jun 3, 2022

Copy link
Copy Markdown
Member Author

No real change in pyperformance https://gist.github.com/Fidget-Spinner/55cd7ee60faaa403ddc970ec31401d35. Seems like weird build differences/noise only.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner
Fidget-Spinner requested a review from sweeneyde June 3, 2022 08:33
kumaraditya303
kumaraditya303 reviewed Jun 3, 2022
View reviewed changes
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.

kumaraditya303
kumaraditya303 approved these changes Jun 3, 2022
View reviewed changes
@Fidget-Spinner @kumaraditya303
Update Misc/NEWS.d/next/Core and Builtins/2022-05-31-16-36-30.gh-issu…
76ada15
…e-93382.Jf6gAj.rst

Co-authored-by: Kumar Aditya <[email protected]>
@Fidget-Spinner

Fidget-Spinner commented Jun 3, 2022

Copy link
Copy Markdown
Member Author

Thanks for the review Kumar!

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner

Fidget-Spinner commented Jun 3, 2022

Copy link
Copy Markdown
Member Author

I'm completely confused. How are tests relating to the peepholer now failing after we updated the news entry...?

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner
Merge branch 'main' of https://github.com/python/cpython into cache_c…
f407dab
…o_code
@sweeneyde

sweeneyde commented Jun 3, 2022

Copy link
Copy Markdown
Member

Without looking at it too carefully yet, I'm guessing the cache needs to be invalidated when the bytecode changes here.

Fidget-Spinner reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

Fidget-Spinner and others added 3 commits June 3, 2022 23:07
@Fidget-Spinner @sweeneyde
invalidate cached code object when frame adds LOAD_FAST_CHECK
fde8a30
Co-Authored-By: Dennis Sweeney <[email protected]>
@Fidget-Spinner
Py_CLEAR already checks for NULL
0c7226c
@Fidget-Spinner
only invalidate cache if opcode changed
0b11670
@Fidget-Spinner

Fidget-Spinner commented Jun 3, 2022

Copy link
Copy Markdown
Member Author

Without looking at it too carefully yet, I'm guessing the cache needs to be invalidated when the bytecode changes here.

I think you're spot on! Thank you.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner

Fidget-Spinner commented Jun 3, 2022

Copy link
Copy Markdown
Member Author

Since there are no perf regressions, I'm merging this.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner
Fidget-Spinner merged commit d52ffc1 into python:main Jun 3, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Jun 3, 2022
@Fidget-Spinner
Fidget-Spinner deleted the cache_co_code branch June 3, 2022 16:41
@Fidget-Spinner

Fidget-Spinner commented Jun 3, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@pablogsal can I backport this PR to 3.11 please?

In essence, 3.11 beta 1 made accessing co_code O(kn) where it was previously O(1) in prior versions. I want to fix that performance regression because it affects anything that touches co_code (e.g coverage.py).

See e.g #93383 (comment) for microbenchmark. For even just a medium-sized code object, there's a nearly 20x slowdown from 3.10 to 3.11. I can't imagine how bad it would be for larger code in the wild.

This change affects the code object struct. I'm not sure if that will affect your decision.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner

Fidget-Spinner commented Jun 4, 2022

Copy link
Copy Markdown
Member Author

@kumaraditya303 I have one question about test_frozenmain and I think you have more expertise in this area than I do:

Seems like test_frozenmain was regenerated for this PR. Does this mean or indicate anything? I'm afraid that I inadvertently changed bytecode representation.

All reactions

Sorry, something went wrong.

Uh oh!

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

@kumaraditya303

kumaraditya303 commented Jun 4, 2022

Copy link
Copy Markdown
Contributor

Marshal format is not deterministic and it seems to be a form of #73894 issue.

Here is the diffoscope diff:

@@ -1,10 +1,10 @@
 00000000: ae0d 0d0a 0000 0000 4c71 5d62 bc01 0000  ........Lq]b....
 00000010: e300 0000 0000 0000 0000 0000 0008 0000  ................
-00000020: 0000 0000 0073 a000 0000 9700 6400 6401  .....s......d.d.
+00000020: 0000 0000 00f3 a000 0000 9700 6400 6401  ............d.d.
 00000030: 6c00 5a00 6400 6401 6c01 5a01 0200 6502  l.Z.d.d.l.Z...e.
 00000040: 6402 ab01 0000 0000 0000 0000 0100 0200  d...............
 00000050: 6502 6403 6500 6a03 0000 0000 0000 0000  e.d.e.j.........
 00000060: ab02 0000 0000 0000 0000 0100 0200 6501  ..............e.
 00000070: 6a04 0000 0000 0000 0000 ab00 0000 0000  j...............
 00000080: 0000 0000 6404 1900 0000 0000 0000 0000  ....d...........
 00000090: 5a05 6405 4400 5d17 5a06 0200 6502 6406  Z.d.D.].Z...e.d.
@@ -18,21 +18,21 @@
 00000110: 6162 6c65 da0f 7573 655f 656e 7669 726f  able..use_enviro
 00000120: 6e6d 656e 74da 1163 6f6e 6669 6775 7265  nment..configure
 00000130: 5f63 5f73 7464 696f da0e 6275 6666 6572  _c_stdio..buffer
 00000140: 6564 5f73 7464 696f 7a07 636f 6e66 6967  ed_stdioz.config
 00000150: 207a 023a 2029 07da 0373 7973 da11 5f74   z.: )...sys.._t
 00000160: 6573 7469 6e74 6572 6e61 6c63 6170 69da  estinternalcapi.
 00000170: 0570 7269 6e74 da04 6172 6776 da0b 6765  .print..argv..ge
-00000180: 745f 636f 6e66 6967 7372 0200 0000 da03  t_configsr......
+00000180: 745f 636f 6e66 6967 7372 0300 0000 da03  t_configsr......
 00000190: 6b65 79a9 00f3 0000 0000 fa1b 5072 6f67  key.........Prog
 000001a0: 7261 6d73 2f74 6573 745f 6672 6f7a 656e  rams/test_frozen
 000001b0: 6d61 696e 2e70 79fa 083c 6d6f 6475 6c65  main.py..<module
-000001c0: 3e72 1100 0000 0100 0000 738c 0000 00f8  >r........s.....
+000001c0: 3e72 1200 0000 0100 0000 738c 0000 00f8  >r........s.....
 000001d0: f006 0001 0b80 0a80 0a80 0ad8 0018 d000  ................
 000001e0: 18d0 0018 d000 18e0 0005 8005 d006 1ad4  ................
 000001f0: 001b d000 1bd8 0005 8005 806a 9023 9428  ...........j.#.(
 00000200: d400 1bd0 001b d809 26d0 091a d409 26d4  ........&.....&.
 00000210: 0928 a818 d409 3280 06f0 0206 0c02 f000  .(....2.........
 00000220: 0701 2af0 0007 012a 8043 f00e 0005 0a80  ..*....*.C......
 00000230: 45d0 0a28 9043 d00a 28d0 0a28 9836 a023  E..(.C..(..(.6.#
 00000240: 9c3b d00a 28d0 0a28 d404 29d0 0429 d004  .;..(..(..)..)..
-00000250: 29f0 0f07 012a f000 0701 2a72 0f00 0000  )....*....*r....
+00000250: 29f0 0f07 012a f000 0701 2a72 1000 0000  )....*....*r....

cc @methane

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.

@kumaraditya303

kumaraditya303 commented Jun 4, 2022

Copy link
Copy Markdown
Contributor

I'm afraid that I inadvertently changed bytecode representation.

No, I don't think so because I tried comparing deepfreeze output of test_frozenmain and it did not show any difference.

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

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

@pablogsal can I backport this PR to 3.11 please?

In essence, 3.11 beta 1 made accessing co_code O(kn) where it was previously O(1) in prior versions. I want to fix that performance regression because it affects anything that touches co_code (e.g coverage.py).

See e.g #93383 (comment) for microbenchmark. For even just a medium-sized code object, there's a nearly 20x slowdown from 3.10 to 3.11. I can't imagine how bad it would be for larger code in the wild.

This change affects the code object struct. I'm not sure if that will affect your decision.

Can you please check if this fixes this issue:

coveragepy/coveragepy#1287

If so, I am happy backporting it as long as two more core dev other than myself review it and approve it.

CC: @markshannon @brandtbucher @nedbat

All reactions

Sorry, something went wrong.

Uh oh!

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

@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

Can you please check if this fixes this issue:

It should help but I doubt it will fix all of it. My own research suggests that profiling has slowed down by >50% in 3.11, even without the co_code issue.

All reactions

Sorry, something went wrong.

Uh oh!

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

Fidget-Spinner added a commit to Fidget-Spinner/cpython that referenced this pull request Jun 4, 2022
@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

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

I opened the backport for 3.11 since it's simpler than the 3.12 code. I hope the discussion can happen there or on the issue instead #93493. Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Jun 8, 2022
Severe performance degradation for tracing under 3.11 #93516
Open
[3.11] gh-93382: Cache result of PyCode_GetCode in codeobject (GH-93383) #93493
Merged
pablogsal pushed a commit that referenced this pull request Jun 23, 2022
@Fidget-Spinner @kumaraditya303 @sweeneyde
[3.11] gh-93382: Cache result of PyCode_GetCode in codeobject (GH-9…
852b4d4
…3383) (#93493)

Co-authored-by: Kumar Aditya <[email protected]>
Co-authored-by: Dennis Sweeney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@sweeneyde sweeneyde Awaiting requested review from sweeneyde

Uh oh!

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

Assignees

No one assigned

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PyCode_GetCode could be faster

Uh oh!

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

6 participants

@Fidget-Spinner @sweeneyde @kumaraditya303 @pablogsal @AA-Turner @bedevere-bot
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.