GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-130704: Strength reduce `LOAD_FAST{_LOAD_FAST}` by mpage · Pull Request #130708 · python/cpython Conversation
|
|
A note for reviewers: I realized that we need to special case opcodes that leave some of their operands on the stack. This doesn't appear to reduce the effectiveness of the optimization. Stats look roughly the same as the previous version. Performance is a little worse (2.1% for the free-threaded build, 2.5% for the default), but it's hard for me to tell if that's just from noise in the runner or from changes that landed in main since the last time I ran the benchmarks. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Missing one hint for the cases generator, otherwise looks good.
We could make the analysis more robust by using the cases generator, but that's for a later PR.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| _PyInterpreterFrame *gen_frame = &gen->gi_iframe; | ||
| STACK_SHRINK(1); | ||
| _PyFrame_StackPush(gen_frame, v); | ||
| _PyFrame_StackPush(gen_frame, PyStackRef_MakeHeapSafe(v)); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think you need a DEAD(v); here
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| break; | ||
| } | ||
|
|
||
| // We treat opcodes that do not consume all of their inputs on |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This approach seems fine for now, but the code generator knows exactly how many values are popped and consumed, as opposed to peek at.
We should add a _PyOpcode_num_peeked function, the we'd have consumed = _PyOpcode_num_popped() - _PyOpcode_num_peeked() which would be more robust.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @mpage for commit 2e38f0d 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130708%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
- Header files have moved around. - Reference counting has changed. It appears to be python/cpython#130708 that's eliding some reference counting within functions and caused us to need to lower our expected reference count in a few places. NOTE: I'm not 100% sure this is the case; dis.dis is broken and won't show the function bodies so I can't confirm the new opcodes are being used.
- Header files have moved around. - Reference counting has changed. It appears to be python/cpython#130708 that's eliding some reference counting within functions and caused us to need to lower our expected reference count in a few places. NOTE: I'm not 100% sure this is the case; dis.dis is broken and won't show the function bodies so I can't confirm the new opcodes are being used.
- Header files have moved around. - Reference counting has changed. It appears to be python/cpython#130708 that's eliding some reference counting within functions and caused us to need to lower our expected reference count in a few places. NOTE: I'm not 100% sure this is the case; dis.dis is broken and won't show the function bodies so I can't confirm the new opcodes are being used.
- Header files have moved around. - Reference counting has changed. It appears to be python/cpython#130708 that's eliding some reference counting within functions and caused us to need to lower our expected reference count in a few places. NOTE: I'm not 100% sure this is the case; but `dis.dis` shows the new opcode being used for the variables we're testing the refcount of.
Running that test on py3.14 fails:
____________________ test_strings_refcount _____________________
def test_strings_refcount():
# buffer with string data - not bytes nor unicode so that when builtin
# string types are patched there is no case where bytes is created from the
# same bytes, or unicode is created from the same unicode - only increasing
# refcount of original object.
data = bytearray([ord('a'), ord('b'), ord('c'), ord('4')])
# first verify our logic on std type
obj = bytes(data); assert type(obj) is bytes
> gc.collect(); assert sys.getrefcount(obj) == 1+1 # +1 due to obj passed to getrefcount call
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E AssertionError: assert 1 == (1 + 1)
E + where 1 = <built-in function getrefcount>(b'abc4')
E + where <built-in function getrefcount> = sys.getrefcount
golang/golang_str_test.py:301: AssertionError
That happens because on py3.14 the interpreter now does not add a
reference when passing on-stack argument to a function if it can prove
that is safe to do so.
Please see python/cpython#130708 and
python/cpython@f2379535 for details.
-> Adjust the test to account for that on py3.14 .
/reviewed-by @jerome
/reviewed-on https://lab.nexedi.com/nexedi/pygolang/-/merge_requests/39 + https://lab.nexedi.com/nexedi/slapos/-/merge_requests/1863#note_260064
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.