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-125837: Split `LOAD_CONST` into three. by markshannon · Pull Request #125972 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Oct 25, 2024 •
edited by github-actions Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Splits LOAD_CONST into three instructions

  • LOAD_INT for ints in range(256). Avoids the need for a space in the co_consts tuple and avoids an incref
  • LOAD_CONST_IMMORTAL for other immortal objects. Avoids an incref
  • LOAD_CONST for the remaining constants
  • Issue: Refactor RETURN_VALUE and RETURN_CONST #125837

📚 Documentation preview 📚: https://cpython-previews--125972.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
efimov-mikhail and vasily-v-ryabov reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
markshannon and others added 16 commits October 22, 2024 15:57
@markshannon
Replace RETURN_VLAUE/RETURN_CONST with RETURN_VALUE_FUNC/RETURN_VALUE…
2a61cfc 
…_GEN
@markshannon
Add LOAD_CONST_IMMORTAL
42a526c
@markshannon
Fix up all the tests apart from test_dis
2a3b1e2
@markshannon
Update optimizer
031f320
@markshannon
Fix up test_dis
cb5252b
@markshannon
Fix up a couple of comments
892edbc
@markshannon @tomasr8
Apply suggestions from code review
5ce99ca 
Co-authored-by: Tomas R. <[email protected]>
@markshannon
Clarify comment
325c5fe
@markshannon
Remove unused function
6149153
@markshannon
Update test
59a554a
@markshannon
Update dis docs
bcb4e65
@markshannon
Remove link to remove instruction
f3383e5
@markshannon
Restore RETURN_VALUE
68e97ba
@markshannon
Add LOAD_INT instruction
0b8a5ad
@markshannon
Fix up dis and some tests
1bfdd70
@markshannon
Fix up test dis
f934559
@bedevere-app bedevere-app Bot mentioned this pull request Oct 25, 2024
Refactor RETURN_VALUE and RETURN_CONST #125837
Closed
picnixz
picnixz reviewed Oct 25, 2024
View reviewed changes
Comment thread Python/bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

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

picnixz commented Oct 25, 2024

Copy link
Copy Markdown
Member

LOAD_INT for ints in range(256)

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).
All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon commented Oct 25, 2024

Copy link
Copy Markdown
Member Author

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 LOAD_INT 0 to push 0 not -5 to keep things simple.
So LOAD_INT will only cover range(256) even though the range of statically allocated ints is larger.

We can always add -5,-4,-3,-2 and -1 to LOAD_COMMON_CONST later if we want.
picnixz, Eclips4, and efimov-mikhail reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon added 2 commits October 25, 2024 15:15
@markshannon
Address review comments
f135b69
@markshannon
Fix __annotate__ docstring
8f2a781
@markshannon

markshannon commented Oct 25, 2024

Copy link
Copy Markdown
Member Author

Performance for tier 1 appears to be about 0.5% faster.

Stats show the following fraction of constants loaded:

Opcode Fraction
LOAD_INT 47%
LOAD_CONST_IMMORTAL 42%
LOAD_CONST 11%

With a reduction in increfs of immortal objects of about 20%.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
Teach optimizer about LOAD_INT
97c6cdc
@markshannon
markshannon marked this pull request as ready for review October 25, 2024 14:39
@markshannon
markshannon requested review from Eclips4, brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners October 25, 2024 14:39
Eclips4
Eclips4 reviewed Oct 25, 2024
View reviewed changes
Comment thread Lib/test/test_ast/test_ast.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
Make functions static again to avoid warning
79305d2
Eclips4
Eclips4 reviewed Oct 25, 2024
View reviewed changes
Comment thread Lib/test/test_ast/test_ast.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
implement review suggestion
7e4b2a8
iritkatriel
iritkatriel reviewed Oct 25, 2024
View reviewed changes
Comment thread Include/internal/pycore_magic_number.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_ast/test_ast.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Core_and_Builtins/2024-10-25-15-56-14.gh-issue-125837.KlCdgD.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/specialize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon
brettcannon removed their request for review October 25, 2024 20:01
JelleZijlstra
JelleZijlstra reviewed Oct 27, 2024
View reviewed changes
Comment thread Python/codegen.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
markshannon added 2 commits October 28, 2024 10:06
@markshannon
Address review comments
57e891e
@markshannon
Fix typo
207e35e
iritkatriel
iritkatriel reviewed Oct 28, 2024
View reviewed changes
Comment thread Doc/library/dis.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
markshannon added 4 commits October 28, 2024 10:47
@markshannon
Remove extra argument
7c88f69
@markshannon
Rename LOAD_INT to LOAD_SMALL_INT
9d4e575
@markshannon
Regenerate test file
ae11453
@markshannon
Fix test_dis
3a50437
@markshannon markshannon mentioned this pull request Oct 28, 2024
Add instructions for creating small objects. faster-cpython/ideas#609
Open
iritkatriel
iritkatriel reviewed Oct 28, 2024
View reviewed changes
Comment thread Python/codegen.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel
iritkatriel reviewed Oct 28, 2024
View reviewed changes
Comment thread 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) {

@iritkatriel iritkatriel Oct 28, 2024

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.

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.

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Oct 28, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

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

@Eclips4 Eclips4 Oct 28, 2024

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.

Maybe it's worth testing whether creating integers within the range(256) will not affect co_consts?

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Oct 29, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Eclips4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@markshannon
Don't mark LOAD_SMALL_INT as having a constant, as it doesn't load fr…
47927cf 
…om code.co_consts
Eclips4
Eclips4 reviewed Oct 28, 2024
View reviewed changes
Comment thread Lib/test/test_ast/test_ast.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
Add back int to test
3caac1a
@markshannon markshannon mentioned this pull request Oct 29, 2024
gh-126072: do not add None to co_consts if no docstring #126101
Merged
iritkatriel
iritkatriel approved these changes Oct 29, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Oct 29, 2024
@markshannon
markshannon merged commit faa3272 into python:main Oct 29, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 29, 2024
@markshannon
markshannon deleted the split-load-const branch November 18, 2024 14:37
picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
@markshannon @picnixz
pythonGH-125837: Split LOAD_CONST into three. (pythonGH-125972)
7487220 
* Add LOAD_CONST_IMMORTAL opcode

* Add LOAD_SMALL_INT opcode

* Remove RETURN_CONST opcode
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@markshannon @ebonnal
pythonGH-125837: Split LOAD_CONST into three. (pythonGH-125972)
a02dbb3 
* Add LOAD_CONST_IMMORTAL opcode

* Add LOAD_SMALL_INT opcode

* Remove RETURN_CONST opcode
@eendebakpt eendebakpt mentioned this pull request Apr 26, 2025
Increase _PY_NSMALLPOSINTS size faster-cpython/ideas#725
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@picnixz picnixz picnixz left review comments
@Eclips4 Eclips4 Eclips4 left review comments
@iritkatriel iritkatriel iritkatriel approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner

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.

5 participants

@markshannon @picnixz @JelleZijlstra @iritkatriel @Eclips4
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.