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

fulfill: try using SmallVec or Box for stalled_on #72776

Merged
merged 1 commit into from Jun 1, 2020

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 30, 2020

Tested both Box and SmallVec for stalled_on, with both resulting in a perf loss.
Adds a comment mentioning this and removes an now outdated FIXME.

Logging the length of stalled_on resulted in the following distribution while building a part of stage 1 libs:

22627647 counts:
(  1) 20983696 (92.7%, 92.7%): process_obligation_len: 1
(  2)   959711 ( 4.2%, 97.0%): process_obligation_len: 2
(  3)   682326 ( 3.0%,100.0%): process_obligation_len: 0
(  4)     1914 ( 0.0%,100.0%): process_obligation_len: 3

cc @eddyb
r? @nnethercote

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2020
@nnethercote
Copy link
Contributor

r=me if the perf CI run looks good:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 30, 2020

⌛ Trying commit 8be007786cb0aa917163757d9ee5fa7d8a7a7d4a with merge 95fdf3ad69575b3abd39f50ddc1c669115d57664...

@bors
Copy link
Contributor

bors commented May 30, 2020

☀️ Try build successful - checks-azure
Build commit: 95fdf3ad69575b3abd39f50ddc1c669115d57664 (95fdf3ad69575b3abd39f50ddc1c669115d57664)

@rust-timer
Copy link
Collaborator

Queued 95fdf3ad69575b3abd39f50ddc1c669115d57664 with parent 91fb72a, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 95fdf3ad69575b3abd39f50ddc1c669115d57664, comparison URL.

@lcnr
Copy link
Contributor Author

lcnr commented May 30, 2020

Well, I did not expect that 😆

So my understanding is that we very frequently look at the elements of stalled_on without allocating anything. As SmallVec has to first check the current len to get the relevant pointer this
is actually slower than using a Vec.

I think I will go ahead and update the docs, so we don't forget this.

@lcnr
Copy link
Contributor Author

lcnr commented May 30, 2020

I now changed stalled_on to Box<[TyOrConstVar<'tcx>]> and updated the comment for stalled_on.

Let's see if this changes perf 🤷

@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 30, 2020

⌛ Trying commit a7c4b936ea3b3f79883bfc7eb7be0c76027cb664 with merge 9c4cd0972aa23430f490bc23cab84804323de3de...

@lcnr lcnr changed the title fulfill: use SmallVec for stalled_on fulfill: use Box for stalled_on May 30, 2020
@bors
Copy link
Contributor

bors commented May 30, 2020

☀️ Try build successful - checks-azure
Build commit: 9c4cd0972aa23430f490bc23cab84804323de3de (9c4cd0972aa23430f490bc23cab84804323de3de)

@rust-timer
Copy link
Collaborator

Queued 9c4cd0972aa23430f490bc23cab84804323de3de with parent 74e8046, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9c4cd0972aa23430f490bc23cab84804323de3de, comparison URL.

@nnethercote
Copy link
Contributor

If you have a Linux machine I recommend getting familiar with local benchmarking and profiling, it'll be much faster and easier than having to create PRs and do perf runs on CI. Instructions are here.

@lcnr
Copy link
Contributor Author

lcnr commented May 31, 2020

Updated the comment of stalled_on and didn't change anything else.

@nnethercote
Copy link
Contributor

The new comment in the code looks good, thanks.

The first comment in this PR will be used in the merge commit. Can you update that so it describes the new change rather than the old change? That way the VCS history will be clearer.

@lcnr
Copy link
Contributor Author

lcnr commented May 31, 2020

👍 Like this?

@lcnr lcnr changed the title fulfill: use Box for stalled_on fulfill: try using SmallVec or Box for stalled_on May 31, 2020
@nnethercote
Copy link
Contributor

@bors r+ rollup=always

Thanks!

@bors
Copy link
Contributor

bors commented May 31, 2020

📌 Commit 0441763 has been approved by nnethercote

@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 May 31, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#72776 (fulfill: try using SmallVec or Box for stalled_on)
 - rust-lang#72818 (Clean up E0622 explanation)
 - rust-lang#72823 (Add descriptions for all queries)
 - rust-lang#72832 (RELEASES.md: Expand `cargo tree` note to mention `cargo tree -d`)
 - rust-lang#72834 (Rephrase term 'non-pointer type')

Failed merges:

r? @ghost
@bors bors merged commit fa29439 into rust-lang:master Jun 1, 2020
@lcnr lcnr deleted the stalled_on-smallvec branch June 1, 2020 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

7 participants