Skip to content

Conversation

nnethercote
Copy link
Contributor

A couple of easy wins. Here are the NLL speedups that exceed 1%:

sentry-cli-check
        avg: -3.5%      min: -3.5%      max: -3.5%
inflate-check
        avg: -1.9%      min: -1.9%      max: -1.9%
inflate
        avg: -1.7%      min: -1.7%      max: -1.7%
clap-rs-check
        avg: -1.6%      min: -1.6%      max: -1.6%
cargo-check
        avg: -1.6%      min: -1.6%      max: -1.6%
ripgrep-check
        avg: -1.4%      min: -1.4%      max: -1.4%
serde-check
        avg: -1.2%      min: -1.2%      max: -1.2%
regex-check
        avg: -1.0%      min: -1.0%      max: -1.0%
sentry-cli
        avg: -1.0%      min: -1.0%      max: -1.0%

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2018
// non-handling of siblings);
// - ~99% of the time the loop isn't reached, and this code hot, so we
// don't want to allocate `todo` unnecessarily.
// the time
Copy link
Contributor

Choose a reason for hiding this comment

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

"the time" here is a dangling fragment.

And since you'll be updating the comment phrasing anyway, i would like it if you added an explicit note that we do not traverse siblings of the input mpi, merely its children. (I believe that's what the comment on push_siblings was trying to do in the previous version in this code.)

@pnkfelix
Copy link
Contributor

this looks fine to me; I left a nit about a typo in the comments and also about a comment that was deleted that I would like preserved.

@nikomatsakis
Copy link
Contributor

r? @pnkfelix

`has_any_child_of` is hot. It allocates a `Vec` that almost always
doesn't exceed a length of 1.

This patch peels off the first iteration of the loop, avoiding the need
for the `Vec` creation in ~99% of cases.
These vectors are always small, so this avoids lots of allocations.
@nnethercote
Copy link
Contributor Author

@pnkfelix: new version is up. I removed the dangling text and tweaked the new comment about siblings. (Note that the old comment about siblings is still present.)

@pnkfelix
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 19, 2018

📌 Commit ba0bb02 has been approved by pnkfelix

@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 Jun 19, 2018
@bors
Copy link
Collaborator

bors commented Jun 20, 2018

⌛ Testing commit ba0bb02 with merge 93a1611...

bors added a commit that referenced this pull request Jun 20, 2018
Reduce number of allocations done by NLL

A couple of easy wins. Here are the NLL speedups that exceed 1%:
```
sentry-cli-check
        avg: -3.5%      min: -3.5%      max: -3.5%
inflate-check
        avg: -1.9%      min: -1.9%      max: -1.9%
inflate
        avg: -1.7%      min: -1.7%      max: -1.7%
clap-rs-check
        avg: -1.6%      min: -1.6%      max: -1.6%
cargo-check
        avg: -1.6%      min: -1.6%      max: -1.6%
ripgrep-check
        avg: -1.4%      min: -1.4%      max: -1.4%
serde-check
        avg: -1.2%      min: -1.2%      max: -1.2%
regex-check
        avg: -1.0%      min: -1.0%      max: -1.0%
sentry-cli
        avg: -1.0%      min: -1.0%      max: -1.0%
```
r? @nikomatsakis
@bors
Copy link
Collaborator

bors commented Jun 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 93a1611 to master...

@bors bors merged commit ba0bb02 into rust-lang:master Jun 20, 2018
@nnethercote nnethercote deleted the nll-allocs branch June 20, 2018 04:42
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.

5 participants