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

Refactor how SwitchInt stores jump targets #77796

Merged
merged 2 commits into from
Oct 13, 2020
Merged

Refactor how SwitchInt stores jump targets #77796

merged 2 commits into from
Oct 13, 2020

Conversation

jonas-schievink
Copy link
Contributor

Closes #65693

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 10, 2020
@jonas-schievink
Copy link
Contributor Author

@bors try @rust-timer queue

r? @oli-obk

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@rust-highfive rust-highfive assigned oli-obk and unassigned eddyb Oct 10, 2020
@bors
Copy link
Contributor

bors commented Oct 10, 2020

⌛ Trying commit 432535d with merge f75713278ef412b926ff605fed784633ba5d6092...

@bors
Copy link
Contributor

bors commented Oct 10, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: f75713278ef412b926ff605fed784633ba5d6092 (f75713278ef412b926ff605fed784633ba5d6092)

@rust-timer
Copy link
Collaborator

Queued f75713278ef412b926ff605fed784633ba5d6092 with parent cae8bc1, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (f75713278ef412b926ff605fed784633ba5d6092): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

This allows building common SwitchTargets (eg. for `if`s) without
allocation.
@jonas-schievink
Copy link
Contributor Author

Looks like no noticeable change so far, good. Let's try using SmallVec instead of the old setup.

@jonas-schievink
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 10, 2020

⌛ Trying commit 9a47f74 with merge 70f6dc489a31b5239cdff8395134265d0d6452cc...

@bors
Copy link
Contributor

bors commented Oct 10, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 70f6dc489a31b5239cdff8395134265d0d6452cc (70f6dc489a31b5239cdff8395134265d0d6452cc)

@rust-timer
Copy link
Collaborator

Queued 70f6dc489a31b5239cdff8395134265d0d6452cc with parent 790d19c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (70f6dc489a31b5239cdff8395134265d0d6452cc): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@jonas-schievink
Copy link
Contributor Author

Hmm, instructions went down, but cycles and clocks went up. Not sure if that's just noise.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 12, 2020

cycles and clocks are always the least reliable. The change lgtm and without you mentioning it I wouldn't even have looked at anything but instructions.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 12, 2020

📌 Commit 9a47f74 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2020
@bors
Copy link
Contributor

bors commented Oct 13, 2020

⌛ Testing commit 9a47f74 with merge afb4514...

@bors
Copy link
Contributor

bors commented Oct 13, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing afb4514 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2020
@Mark-Simulacrum
Copy link
Member

Perf results after landing are about as expected; I agree with @oli-obk that I would not want to change anything here and I think investigating is not warranted to prioritize right now.

@jonas-schievink jonas-schievink deleted the switchint-refactor branch October 22, 2020 01:21
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2020
…=oli-obk

Refactor how SwitchInt stores jump targets

Closes rust-lang#65693
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider merging values and targets fields on MIR switchInt?
8 participants