gh-126835: Move constant tuple folding to CFG#130769
Conversation
picnixz
left a comment
There was a problem hiding this comment.
Small style comments. It's always better to split assertions into two when possible to know which condition failed.
Sorry, something went wrong.
|
After moving folding to codegen (70ce2c8) I can see couple refleaks after running test suite in refleak hunter mode. After some debugging I narrowed down the problem. Long tuple defined directly does not report refleaks: x = (0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30)but if it is run via compile("x = (0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30)", "", "single")no idea yet why this is happening. I will be debugging further, but maybe someone has some insight what could be the problem? |
Sorry, something went wrong.
|
Does it leak with compile with other modes (exec, eval)? |
Sorry, something went wrong.
Yes. I wonder if it could be some form of false positive. |
Sorry, something went wrong.
|
No idea. |
Sorry, something went wrong.
|
Problem found. Turns out |
Sorry, something went wrong.
|
Why does it not happen at the prompt? |
Sorry, something went wrong.
|
No idea. Maybe refleak checking flaw, but not sure. Maybe someone who knows more about it could tell us? Back to this PR - if we fold long tuples at codegen stage, we won't be able to fold long tuples that have constant expressions inside, for instance: (1, 2, 1 + 2, 4, ... 100) # long constant tuplewill not be folded, unless we fold it in peepholer as written initially. Drawback of having optimizations that have to work together split between different stages. |
Sorry, something went wrong.
|
I tend to agree. So I guess you're saying we should choose the second option from #130769 (comment). |
Sorry, something went wrong.
|
Second options assumes that if we have constant tuple, we can skip |
Sorry, something went wrong.
We could check recursively for constant expressions and emit long tuple bytecode if we know all subexpressions are constant, but I believe it would complicate code more than optimizing it in the peepholer. |
Sorry, something went wrong.
|
I understand. Ok, let's revert back to the peephole situation then. |
Sorry, something went wrong.
|
Another idea (not in this PR) would be to move all the |
Sorry, something went wrong.
|
I will make a note and investigate later. |
Sorry, something went wrong.
|
I've reverted to the peepholer optimization and addressed previous review, please take a look. |
Sorry, something went wrong.
iritkatriel
left a comment
There was a problem hiding this comment.
I think it's a bit simpler if we grab two instructions in each iteration.
Sorry, something went wrong.
577fb71 to
72a3e65
Compare
March 18, 2025 12:34
|
@iritkatriel can you review again when you have time? |
Sorry, something went wrong.
|
!buildbot refleak |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 4b11cf8 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130769%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
Sorry, something went wrong.
edited by bedevere-app
Bot
LoadingUh oh!
There was an error while loading. Please reload this page.
Copy link Copy MarkdownSorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.