Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
GH-112354: Initial implementation of warm up on exits and trace-stitching#114142
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
markshannon commented Jan 16, 2024 • edited by gvanrossum
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by gvanrossum
Uh oh!
There was an error while loading. Please reload this page.
markshannon commented Jan 17, 2024
brandtbucher commented Jan 18, 2024
Sorry, didn't get to a proper review of this today. I've been trying (and failing) to merge the JIT branch into this one without crashing. Maybe we can chat "in person" later? |
brandtbucher commented Jan 18, 2024
Okay, I was able to get it working-ish... but it's not pretty. We can go over it when we meet. |
markshannon commented Jan 18, 2024
gvanrossum left a comment
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.
Here's a first, partial pass. I've tried to focus on things that confused me. I haven't fully reviewed optimizer.c yet, nor the _COLD_EXIT uop definition, so I'll focus on those when I find some time.
Maybe it would be useful to add TODO comments indicating things you're planning to tackle in the near future (subsequent PRs)? Like the counters.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
markshannon commented Feb 16, 2024
I'd prefer to save that for another PR. Maybe the one that removes the |
markshannon commented Feb 16, 2024
FTR, the thresholds are likely to get completely changed soon. We probably want to change the fixed thresholds to some sort of adaptive thresholds, and consider the T1 and T2 thresholds together so that specialization works correctly. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gvanrossum commented Feb 16, 2024
If you look at GitHub there’s still a missing cast somewhere. |
brandtbucher left a comment
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.
Didn't look too closely at the cases generator or optimizer (I'm assuming there are other eyes on that).
- I appreciate the writeup!
- I like that the interpreter/JIT interfaces are a lot more unified on this iteration. I know you're not crazy about conditional macro stuff but I think it makes things a lot easier to follow in this case. Thanks.
- Suggestions to remove bit of tricky code duplication in
template.c. - A couple of questions about setting
tstate->previous_executor. - A few other random questions/suggestions I had while reading through.
- One thing I find sort of tricky to keep track of is the nuances of the first couple of instructions in the trace (when/where
_START_EXECUTORis added, when/where pointers to optimizers/executors are smuggled in as operands, etc). This might be worth cleaning up now or in the future, or at least writing up. Not sure.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
When you're done making the requested changes, leave the comment: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
encukou commented Feb 20, 2024
markshannon commented Feb 20, 2024
I don't think there is a new bug here, but this PR exposes it. #115727 |
Only works for boolean guards, not type guards, for now. And
_EXIT_TRACEnow.Needs to be carefully documented, there's some subtlety here.
Exits are implemented as an array of records attached to the executors, and a fixed set of cold exit executors.
We can shrink the
_PyExitDatafurther, by moving the counters into a global table, but that's for another PR.See faster-cpython/ideas#644 for a rough sketch of the design.