Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

GH-128914: Remove conditional stack effects frombytecodes.c and the code generators#128918

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@markshannon
Copy link
Member

@markshannonmarkshannon commentedJan 16, 2025
edited by bedevere-appbot
Loading

This PR:

  • Removes support for conditional stack effects. Variable stack effects are still supported
  • SplitsLOAD_ATTR intoLOAD_ATTR andLOAD_METHOD. The specializations split neatly between the two, so no new specializations are needed.
  • SplitsLOAD_SUPER_ATTR intoLOAD_SUPER_ATTR andLOAD_SUPER_METHOD. This is a bit wasteful asLOAD_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 is0.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 extraPUSH_NULL instructions required. There are ways to mitigate this, but not in this PR.

@markshannon
Copy link
MemberAuthor

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

Copy link
Member

@iritkatrieliritkatriel left a 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.

@markshannonmarkshannon merged commitab61d3f intopython:mainJan 20, 2025
65 checks passed
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull requestJan 21, 2025
@markshannonmarkshannon deleted the no-conditional-stack-effects branchJanuary 21, 2025 10:30
@colesbury
Copy link
Contributor

colesbury commentedJan 21, 2025
edited
Loading

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

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

@Fidget-Spinner
Copy link
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?

colesbury added a commit to colesbury/cpython that referenced this pull requestJan 22, 2025
…odes.c` and the code generators (pythonGH-128918)"The commit introduced a large performance regression in the free threading build.This reverts commitab61d3f.
@markshannonmarkshannon restored the no-conditional-stack-effects branchJanuary 22, 2025 17:40
colesbury added a commit to colesbury/cpython that referenced this pull requestJan 22, 2025
…odes.c` and the code generators (pythonGH-128918)"The commit introduced a ~2.5-3% regression in the free threading build.This reverts commitab61d3f.
@diegorusso
Copy link
Contributor

diegorusso commentedJan 23, 2025
edited
Loading

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

$ ./python -mtest test.test__opcodeUsing random seed: 23768782570:00:00 load avg: 0.20 Run 1 test sequentially in a single process0:00:00 load avg: 0.20 [1/1] test.test__opcodetest 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__opcodeTotal duration: 24 msTotal tests: run=7 failures=1Total test files: run=1/1 failed=1Result: FAILURE

@diegorusso
Copy link
Contributor

diegorusso commentedJan 23, 2025
edited
Loading

OK, this fixes it!

$ git diffdiff --git a/Python/specialize.c b/Python/specialize.cindex 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__opcodeUsing random seed: 9113912230:00:00 load avg: 0.31 Run 1 test sequentially in a single process0:00:00 load avg: 0.31 [1/1] test.test__opcode== Tests result: SUCCESS ==1 test OK.Total duration: 19 msTotal tests: run=7Total test files: run=1/1Result: SUCCESS

markshannon pushed a commit that referenced this pull requestJan 23, 2025
…` and the code generators (GH-128918)" (GH-129202)The commit introduced a ~2.5-3% regression in the free threading build.This reverts commitab61d3f.
@diegorusso
Copy link
Contributor

diegorusso commentedJan 23, 2025
edited
Loading

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

@mdboom
Copy link
Contributor

@colesbury wrote:

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

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...

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@iritkatrieliritkatrieliritkatriel approved these changes

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@Fidget-SpinnerFidget-SpinnerAwaiting requested review from Fidget-SpinnerFidget-Spinner is a code owner

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@markshannon@colesbury@Fidget-Spinner@diegorusso@mdboom@iritkatriel

[8]ページ先頭

©2009-2025 Movatter.jp