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-128914: Remove conditional stack effects from `bytecodes.c` and the code generators by markshannon · Pull Request #128918 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Jan 16, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This PR:

  • Removes support for conditional stack effects. Variable stack effects are still supported
  • Splits LOAD_ATTR into LOAD_ATTR and LOAD_METHOD. The specializations split neatly between the two, so no new specializations are needed.
  • Splits LOAD_SUPER_ATTR into LOAD_SUPER_ATTR and LOAD_SUPER_METHOD. This is a bit wasteful as LOAD_SUPER_ATTR is quite rare and it needs an additional instrumented instruction as well. It might be worth trying to merge them somehow in another PR later on but doing so now would complicate this PR unnecessarily.

Performance is 0.4% slower which is, I think, acceptable given the potential speedups from top of stack caching.

The slowdown appears to be mostly a result of the large number of extra PUSH_NULL instructions required. There are ways to mitigate this, but not in this PR.

  • Issue: Get rid of conditional inputs and outputs for instructions in bytecodes.c #128914

Sorry, something went wrong.

Uh oh!

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

All reactions
markshannon added 9 commits January 15, 2025 14:01
@markshannon
No conditional stack effects for LOAD_GLOBAL or LOAD_ATTR
d85c001
@markshannon
No conditional stack effects for LOAD_SUPER_ATTR or CALL_FUNCTION_EX
053327a
@markshannon
Remove support for conditional stack effects from code generators
029f844
@markshannon
Fix up tests
0f49a42
@markshannon
Remove 'split' annotation
0515341
@markshannon
Rename result of PUSH_NULL
402787c
@markshannon
Use full oparg for name index in LOAD_GLOBAL and LOAD_ATTR
6ac95d4
@markshannon
Use correct magic number
3e475e8
@markshannon
Fix magic number comment
a766382
@markshannon
markshannon requested review from Fidget-Spinner, ericsnowcurrently and iritkatriel as code owners January 16, 2025 14:15
@bedevere-app bedevere-app Bot mentioned this pull request Jan 16, 2025
Get rid of conditional inputs and outputs for instructions in bytecodes.c #128914
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 16, 2025
@markshannon markshannon added the skip news label Jan 16, 2025
@markshannon

markshannon commented Jan 16, 2025

Copy link
Copy Markdown
Member Author

This will conflict with #128722, so that PR should be merged first.

All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon added 2 commits January 16, 2025 14:19
@markshannon
Remove unused function
4104065
@markshannon
Merge branch 'main' into no-conditional-stack-effects
6c1a7eb
iritkatriel
iritkatriel approved these changes Jan 16, 2025
View reviewed changes

@iritkatriel iritkatriel left a comment

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.

LGTM. Nice to see so much red.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/test/test_monitoring.py Resolved

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jan 16, 2025
markshannon and others added 9 commits January 16, 2025 18:15
@markshannon @iritkatriel
Update Lib/test/test_monitoring.py
17249ba
Co-authored-by: Irit Katriel <[email protected]>
@markshannon
Merge branch 'main' into no-conditional-stack-effects
9ce0600
@markshannon
Merge remote-tracking branch 'faster/no-conditional-stack-effects' in…
7806d43
…to no-conditional-stack-effects
@markshannon
Document new LOAD_METHOD instruction. Update docs for LOAD_ATTR
bbcc0df
@markshannon
Add news
0a2f9d1
@markshannon
Remove old docs for LOAD_METHOD
be67c3e
@markshannon
Fix example
5e547a5
@markshannon
Fix another example
a805197
@markshannon
Merge branch 'main' into no-conditional-stack-effects
2c2ae8d
iritkatriel
iritkatriel approved these changes Jan 20, 2025
View reviewed changes
@markshannon
Merge branch 'main' into no-conditional-stack-effects
d5e47ea
@markshannon
markshannon merged commit ab61d3f into python:main Jan 20, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 20, 2025
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
@markshannon @srinivasreddy
pythonGH-128914: Remove conditional stack effects from bytecodes.c …
b0a2da6
…and the code generators (pythonGH-128918)
@markshannon
markshannon deleted the no-conditional-stack-effects branch January 21, 2025 10:30
@colesbury

colesbury commented Jan 21, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Hi @markshannon, this PR introduced performance regressions in the free threading build:

  • Single-threaded perf regressed by 3.1%
  • Calling a method from a module no longer scales well when called from multiple threads concurrently.

I think that the multithreading scaling issue is because previously module.foo() used to specialize to _LOAD_ATTR_MODULE_FROM_KEYS, but with this PR it now uses the unspecialized LOAD_METHOD. _LOAD_ATTR_MODULE_FROM_KEYS supports deferred reference counting, but LOAD_METHOD does not. That may be related to the single-threaded perf regression too, but I'm not sure.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner

Fidget-Spinner commented Jan 21, 2025

Copy link
Copy Markdown
Member

@colesbury IIRC I was the one that merged these two instructions together. Based on my memory at the time LOAD_ATTR does cover LOAD_METHOD in some cases, so I'm not surprised theres a perf regression.

I'm surprised by your benchmark results though. If you look into them, it says things like nbody, spectralnorm slowed down. However, these dont use LOAD_ATTR at all?

All reactions

Sorry, something went wrong.

Uh oh!

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

colesbury added a commit to colesbury/cpython that referenced this pull request Jan 22, 2025
@colesbury
Revert "pythonGH-128914: Remove conditional stack effects from `bytec…
68ce740
…odes.c` and the code generators (pythonGH-128918)"

The commit introduced a large performance regression in the free threading build.

This reverts commit ab61d3f.
@markshannon
markshannon restored the no-conditional-stack-effects branch January 22, 2025 17:40
colesbury added a commit to colesbury/cpython that referenced this pull request Jan 22, 2025
@colesbury
Revert "pythonGH-128914: Remove conditional stack effects from `bytec…
f60435e
…odes.c` and the code generators (pythonGH-128918)"

The commit introduced a ~2.5-3% regression in the free threading build.

This reverts commit ab61d3f.
@diegorusso

diegorusso commented Jan 23, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Not 100% sure, but this PR makes the test test.test__opcode failing if CPython is compiled with --enable-pystats

$ ./python -mtest test.test__opcode
Using random seed: 2376878257
0:00:00 load avg: 0.20 Run 1 test sequentially in a single process
0:00:00 load avg: 0.20 [1/1] test.test__opcode
test test.test__opcode failed -- Traceback (most recent call last):
  File "/home/dierus01/work/ce-sw/repos/cpython/Lib/test/test__opcode.py", line 131, in test_specialization_stats
    self.assertCountEqual(stats.keys(), specialized_opcodes)
    ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Element counts were not equal:
First has 0, Second has 1:  'load_super_method'
First has 0, Second has 1:  'load_method'

test.test__opcode failed (1 failure)

== Tests result: FAILURE ==

1 test failed:
    test.test__opcode

Total duration: 24 ms
Total tests: run=7 failures=1
Total test files: run=1/1 failed=1
Result: FAILURE

All reactions

Sorry, something went wrong.

Uh oh!

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

@diegorusso

diegorusso commented Jan 23, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

OK, this fixes it!

$ git diff
diff --git a/Python/specialize.c b/Python/specialize.c
index eb599028cef..bc44b776026 100644
--- a/Python/specialize.c
+++ b/Python/specialize.c
@@ -111,6 +111,8 @@ _Py_GetSpecializationStats(void) {
     int err = 0;
     err += add_stat_dict(stats, CONTAINS_OP, "contains_op");
     err += add_stat_dict(stats, LOAD_SUPER_ATTR, "load_super_attr");
+    err += add_stat_dict(stats, LOAD_SUPER_METHOD, "load_super_method");
+    err += add_stat_dict(stats, LOAD_METHOD, "load_method");
     err += add_stat_dict(stats, LOAD_ATTR, "load_attr");
     err += add_stat_dict(stats, LOAD_GLOBAL, "load_global");
     err += add_stat_dict(stats, BINARY_SUBSCR, "binary_subscr");
$ ./python -mtest test.test__opcode
Using random seed: 911391223
0:00:00 load avg: 0.31 Run 1 test sequentially in a single process
0:00:00 load avg: 0.31 [1/1] test.test__opcode

== Tests result: SUCCESS ==

1 test OK.

Total duration: 19 ms
Total tests: run=7
Total test files: run=1/1
Result: SUCCESS
All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon pushed a commit that referenced this pull request Jan 23, 2025
@colesbury
Revert "GH-128914: Remove conditional stack effects from `bytecodes.c…
a10f993
…` and the code generators (GH-128918)" (GH-129202)

The commit introduced a ~2.5-3% regression in the free threading build.

This reverts commit ab61d3f.
@diegorusso

diegorusso commented Jan 23, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This has been reverted for now: #129202 No need to push the fix.

All reactions

Sorry, something went wrong.

Uh oh!

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

@mdboom

mdboom commented Jan 23, 2025

Copy link
Copy Markdown
Contributor

@colesbury wrote:

Hi @markshannon, this PR introduced performance regressions in the free threading build:

  • Single-threaded perf regressed by 3.1%

FWIW, I tried to reproduce this, and got the same 0.8% slowdown we got on a non-free-threaded build: https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20250120-3.14.0a4+-d5e47ea-NOGIL

To be clear, I'm not advocating one result over the other, but there's a high likelihood that something is different between these runners. It might be meaningful, it might not...

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon
markshannon deleted the no-conditional-stack-effects branch January 31, 2025 16:57
@furkanonder furkanonder mentioned this pull request Apr 22, 2025
test__opcode fails with missing 'jump_backward' in specialization stats #132815
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently 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

skip news

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.

6 participants

@markshannon @colesbury @Fidget-Spinner @diegorusso @mdboom @iritkatriel
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.