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

Postpone HirIdification until HirId functions are optimized #58376

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@ljedrz
Copy link
Contributor

ljedrz commented Feb 11, 2019

Perf took a pretty big hit today and I'm suspecting that one of the HirIdification methods might be the cause, because they aren't optimized yet (some of them convert HirId to NodeId and call the old NodeId function for now). If this is indeed the cause, we will want to optimize these functions before using them.

Can I get a try+perf run?

ljedrz added some commits Feb 11, 2019

Revert ljedrz:HirIdify_typeck
This reverts commit 576df31, reversing
changes made to 4424a2c.
Revert HirIdify_mir
This reverts commit 4424a2c, reversing
changes made to 2d72528.
Revert HirIdify_rustc
This reverts commit c3d2490, reversing
changes made to 68650ca.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 11, 2019

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@ljedrz

This comment has been minimized.

Copy link
Contributor Author

ljedrz commented Feb 11, 2019

Cc @Zoxc

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Feb 11, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 11, 2019

⌛️ Trying commit 5fa4351 with merge fdfb126...

bors added a commit that referenced this pull request Feb 11, 2019

Auto merge of #58376 - ljedrz:postpone_HirIdification, r=<try>
Postpone HirIdification until HirId functions are optimized

Perf took a pretty big hit today and I'm suspecting that one of the HirIdification methods might be the cause, because they aren't optimized yet (some of them convert `HirId` to `NodeId` and call the old `NodeId` function for now). If this is indeed the cause, we will want to optimize these functions before using them.

Can I get a try+perf run?
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 11, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 11, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Feb 11, 2019

Success: Queued fdfb126 with parent 57d7cfc, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Feb 11, 2019

Finished benchmarking try commit fdfb126

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 12, 2019

r=me with a vengeance if you feel it's ready to do so, that's a hell of a lot of green. Can you take the lead on investigating the perf hit?

That being said, in the diff I see a couple of places that are only hot if there has been an error. Those cases could probably be kept without reverting (thinking of https://github.com/rust-lang/rust/pull/58376/files#diff-9d22f4d35c9bd98748f3e1a7cda1b3e7 and https://github.com/rust-lang/rust/pull/58376/files#diff-a4796471067f8f3d7549cb74eca68a4e).

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Feb 12, 2019

@bors r-

This is likely due to #58085 (@Mark-Simulacrum just turned this off on perf, which would explain the perf result here)

@ljedrz

This comment has been minimized.

Copy link
Contributor Author

ljedrz commented Feb 12, 2019

@Zoxc yep, the perf including these commits is green again. Whew ^^.

@ljedrz ljedrz closed this Feb 12, 2019

@ljedrz ljedrz deleted the ljedrz:postpone_HirIdification branch Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment