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-119866: Spill before escaping calls in all cases. by markshannon · Pull Request #124392 · python/cpython Conversation
|
|
I've addressed all the issues. I've increased the number of functions that we treat as non-escaping, but not every function mentioned above. I think it safer to do this than to try to mark everything that is non-escaping and risk making a mistake, or any of those functions changing in the future such that it does escape. Since the cost of spilling is low, spilling around such maybe-non-escaping calls has no measurable performance impact. |
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.
I'm going to approve this on the condition that usage of DEAD is properly documented in a follow-up PR. I don't think it's so clear when to use it correctly and when it might be misused.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Alternatively, you can add the documentation in, since you need to fix the failing test cases anyways. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I will leave the documentation of |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| self.assertListEqual(gc.get_referrers(exc), []) | ||
|
|
||
| asyncio.run(main()) | ||
| self.assertListEqual(gc.get_referrers(exc), [main_coro]) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
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.
Because we are spilling the stack pointer, so the GC can see the locals of the generator.
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.
The changes to asyncio look fine to me, but maybe a comment (along the lines of what you explained to Kumar) would be helpful.
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.
I'm far from an expert on the cases generator, but I didn't see any obvious issues there. A few notes:
DEAD(...) or INPUTS_DEAD().DEAD(...)/INPUTS_DEAD() needs to be explicit in the code, and can't just be inferred from the location of the last use? I vaguely remember discussing this at the sprint, but I forget why.I'm approving this PR because I think it probably needs to happen and I trust that this is the best way of doing this. But I think in general, we might need to take a step back and consider the huge amount of complexity the has accumulated in bytecodes.c, and the thousands of lines of code in the cases generator that process it. The DSL was originally introduced to reduce the boilerplate and complexity in the interpreter loop... I think it still does a good job of this, as evidenced by this PR. But the mental load when reading and editing this file has crept up incrementally with time, and I'm worried that rate is accelerating. It's beyond the scope of this issue, but we should probably consider if the DSL should be reworked to better support the needs of the cases generator.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for tackling this, by the way. It was a big project and it's really cool to see it working correctly (the newly-failing test was neat to see). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Because variables hold references to objects the last use doesn't kill the variable. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Theoretically yes, but mixing scalars and arrays is awkward. We want to be able to move scalars into registers, but having gaps in the in-memory stack makes things complicated. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
#125046 for the other issues. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Interpreter code generators need to be able to handle basic flow control
There was an error while loading. Please reload this page.