-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
gh-93382: Cache result of PyCode_GetCode
in codeobject
#93383
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
@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. |
Quick n dirty microbenchmark:
|
No real change in pyperformance https://gist.github.com/Fidget-Spinner/55cd7ee60faaa403ddc970ec31401d35. Seems like weird build differences/noise only. |
Misc/NEWS.d/next/Core and Builtins/2022-05-31-16-36-30.gh-issue-93382.Jf6gAj.rst
Outdated
Show resolved
Hide resolved
…e-93382.Jf6gAj.rst Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Thanks for the review Kumar! |
I'm completely confused. How are tests relating to the peepholer now failing after we updated the news entry...? |
Without looking at it too carefully yet, I'm guessing the cache needs to be invalidated when the bytecode changes here. |
Co-Authored-By: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
I think you're spot on! Thank you. |
Since there are no perf regressions, I'm merging this. |
@pablogsal can I backport this PR to 3.11 please? In essence, 3.11 beta 1 made accessing 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. |
@kumaraditya303 I have one question about Seems like |
Marshal format is not deterministic and it seems to be a form of #73894 issue. Here is the @@ -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 |
No, I don't think so because I tried comparing deepfreeze output of |
Can you please check if this fixes this issue: If so, I am happy backporting it as long as two more core dev other than myself review it and approve it. |
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 |
…nGH-93383) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
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! |
Fixes gh-93382.