-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Make optVNAssertionPropCurStmtVisitor post-order #78630
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Detailsjust to see what jit-diffs/pintool think about it...
|
@SingleAccretion's idea, I was 100% sure it would regress TP 🙂 but it improves it in fact. https://dev.azure.com/dnceng-public/public/_build/results?buildId=89456&view=results Diffs in libraries collection are not too big that's why I didn't do this change in my previous PR (I though I would regress TP for a small diff). |
If memory is not failing me, this should also fix #57033. |
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.
LGTM!
It is quite curious we see a TP increase. Would be interesting (but not necessarily worthwhile) to find out why that is.
Yeah, it folds the whole thing to |
Seems like there's some more low hanging fruit here TP wise by using a |
SPMI failure:
Presumably collection failed on that one yesterday and we need to kick it off again |
This reverts commit 1ab2c4a.
just to see what spmi thinks about it...