-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
GH-125837: Split LOAD_CONST into three.
#125972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Tomas R. <[email protected]>
I think the range is -5 -> 256 (included) right? (at least _PyLong_SMALL_INTS works that way, but maybe 256 should be excluded even though it's stored in the _PyLong_SMALL_INTS). |
I want We can always add -5,-4,-3,-2 and -1 to |
|
Performance for tier 1 appears to be about 0.5% faster. Stats show the following fraction of constants loaded:
With a reduction in increfs of immortal objects of about 20%. |
Misc/NEWS.d/next/Core_and_Builtins/2024-10-25-15-56-14.gh-issue-125837.KlCdgD.rst
Show resolved
Hide resolved
Python/flowgraph.c
Outdated
| for (int i = 0; i < b->b_iused; i++) { | ||
| if (OPCODE_HAS_CONST(b->b_instr[i].i_opcode)) { | ||
| int opcode = b->b_instr[i].i_opcode; | ||
| if (OPCODE_HAS_CONST(opcode) && opcode != LOAD_SMALL_INT) { |
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.
hasconst is defined in the dis docs as "Sequence of bytecodes that access a constant". I think it will make more sense if we clarify that it means "access a constant from co_consts" and then remove LOAD_SMALL_INT from hasconst and OPCODE_HAS_CONST.
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.
That makes sense. LOAD_COMMON_CONST is not in hasconst.
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.
Maybe it's worth testing whether creating integers within the range(256) will not affect co_consts?
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 don't want to go overboard on testing internal details of the bytecode compiler.
LOAD_CONST with a small int is still correct, just a bit less efficient than LOAD_SMALL_INT.
We already have plenty of tests in test_dis that test this
…om code.co_consts
Splits
LOAD_CONSTinto three instructionsLOAD_INTfor ints inrange(256). Avoids the need for a space in theco_conststuple and avoids an increfLOAD_CONST_IMMORTALfor other immortal objects. Avoids an increfLOAD_CONSTfor the remaining constantsRETURN_VALUEandRETURN_CONST#125837📚 Documentation preview 📚: https://cpython-previews--125972.org.readthedocs.build/