Skip to content
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

Add triage-2021-02-24 #844

Merged
merged 2 commits into from
Feb 25, 2021
Merged

Add triage-2021-02-24 #844

merged 2 commits into from
Feb 25, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Feb 24, 2021

No description provided.


Inline try_get_cached [#82197](https://github.com/rust-lang/rust/issues/82197)
- Very large improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=8fe989dd768f5dfdb0fc90933f3f74fa4579fefd&end=ee88f46bb5e27c4d9f30326e69ff2298dcbeecb1&stat=instructions:u) (up to -10.4% on `full` builds of `externs-debug`)
- An attempt to fix an issue due to a bad interaction with [#81892](https://github.com/rust-lang/rust/issues/81892) which removed a bunch of largely unnecessary inline directives.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the formulation attribute the improvement to the three PRs ? rust-lang/rust#82197 , rust-lang/rust#81892 and rust-lang/rust#81855

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The improvement itself is just from what was measured after the merging of rust-lang/rust#82197, but I can reference rust-lang/rust#81855 if you'd like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes from rust-lang/rust#81855 are directly responsible for the improvement. I would appreciate if you could emphasize that. I would feel uncomfortable if the improvements were to be attributed to rust-lang/rust#82197 which merely added an inline hint to fix unintended interaction with rust-lang/rust#81892, which landed in the meantime. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjgillot @tmiasko I've adjusted the wording. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would describe it as:

rust-lang/rust#82197 shows improvements originally expected from rust-lang/rust#81855, which based query fast path on try_get_cached. In-between the initial successful perf run of rust-lang/rust#81855 and merging, another PR rust-lang/rust#81892 removed an inline hint from the function, which was soon to be on the fast path.

@rylev rylev merged commit 1b4bf4a into rust-lang:master Feb 25, 2021
@rylev rylev deleted the triage-2021-02-24 branch February 25, 2021 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants