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

make super_relate_consts use trait objects #78288

Closed
wants to merge 2 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Oct 23, 2020

super_relate_consts generates quite a lot of llvm-ir considering that it is still fairly rarely used, so let's try using trait objects for that.

@rust-highfive
Copy link
Contributor

r? @oli-obk

(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 23, 2020
@lcnr
Copy link
Contributor Author

lcnr commented Oct 23, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@lcnr lcnr force-pushed the dyn-super-relate branch from 350d7f4 to 6ee91e2 Compare October 23, 2020 14:36
@lcnr
Copy link
Contributor Author

lcnr commented Oct 23, 2020

@bors try

@bors
Copy link
Collaborator

bors commented Oct 23, 2020

⌛ Trying commit 1a28bb0 with merge 49d7eb9f08603d5d5d3a7f1e34de7ab706096d3c...

@bors
Copy link
Collaborator

bors commented Oct 23, 2020

☀️ Try build successful - checks-actions
Build commit: 49d7eb9f08603d5d5d3a7f1e34de7ab706096d3c (49d7eb9f08603d5d5d3a7f1e34de7ab706096d3c)

@rust-timer
Copy link
Collaborator

Queued 49d7eb9f08603d5d5d3a7f1e34de7ab706096d3c with parent 7bade6e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (49d7eb9f08603d5d5d3a7f1e34de7ab706096d3c): 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

@jackh726
Copy link
Member

Perf is basically neutral and this just increases complexity. I'm not sure the tradeoff is worth it.

@lcnr
Copy link
Contributor Author

lcnr commented Nov 1, 2020

It looks like a small improvement to me, but as @jackh726 mentioned it does worsen the experience of working on it in the future so I am going ahead and close this for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants