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

Implement Chain with Option fuses #70896

Merged
merged 4 commits into from
Apr 9, 2020
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Apr 7, 2020

The iterators are now "fused" with Option so we don't need separate state to track which part is already exhausted, and we may also get niche layout for None. We don't use the real Fuse adapter because its specialization for FusedIterator unconditionally descends into the iterator, and that could be expensive to keep revisiting stuff like nested chains. It also hurts compiler performance to add more iterator layers to Chain.

This change was inspired by the proposal on the internals forum. This is an alternate to #70332, directly employing some of the same Fuse optimizations as #70366 and #70750.

r? @scottmcm

The iterators are now "fused" with `Option` so we don't need separate
state to track which part is already exhausted, and we may also get
niche layout for `None`. We don't use the real `Fuse` adapter because
its specialization for `FusedIterator` unconditionally descends into the
iterator, and that could be expensive to keep revisiting stuff like
nested chains. It also hurts compiler performance to add more iterator
layers to `Chain`.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2020
@cuviper
Copy link
Member Author

cuviper commented Apr 7, 2020

@bors try @rust-timer queue

Locally, I measure this being a little faster than nightly on deeply-nested-opt, but a little slower on deeply-nested-debug, which is probably OK. It also doesn't suffer from #70749.

@KrishnaSannasi I went ahead and tried your folding idea for nth too, but that added ~40% instruction count to deeply-nested-opt. We might still justify that if it helps the runtime performance of Chain::nth with internal iteration, but I'll let you explore that separately.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 7, 2020

⌛ Trying commit 859b8da with merge 1f6bcab4a99b6f10ce766a1fe581010d3dd68f6a...

@bors
Copy link
Contributor

bors commented Apr 7, 2020

☀️ Try build successful - checks-azure
Build commit: 1f6bcab4a99b6f10ce766a1fe581010d3dd68f6a (1f6bcab4a99b6f10ce766a1fe581010d3dd68f6a)

@rust-timer
Copy link
Collaborator

Queued 1f6bcab4a99b6f10ce766a1fe581010d3dd68f6a with parent 42abbd8, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 1f6bcab4a99b6f10ce766a1fe581010d3dd68f6a, comparison URL.

@cuviper
Copy link
Member Author

cuviper commented Apr 8, 2020

@scottmcm I updated count and last as you suggested -- locally it didn't seem to make much difference, for better or worse. I tried size_hint too, since that's a similar pattern, but that regressed performance quite a bit, so I dropped it. Perhaps you were wise not to ask for that one. 🙂

@cuviper
Copy link
Member Author

cuviper commented Apr 8, 2020

That perf report matches my own results -- better on deeply-nested-opt, worse on deeply-nested-debug, and slight improvement / neutral on the rest.

We can also compare to #70332, with the caveat that they're not on the exact same merge base, but this PR looks a lot better.

@scottmcm
Copy link
Member

scottmcm commented Apr 9, 2020

@bors r+

Perhaps you were wise not to ask for that one. 🙂

I almost did 🙂 But then I thought about it more and worried that the saturating_add is more scary to be doing every time than the wraps-in-release add in count, for example.

@bors
Copy link
Contributor

bors commented Apr 9, 2020

📌 Commit ce8abc6 has been approved by scottmcm

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

Successful merges:

 - rust-lang#67705 (Use unrolled loop for searching NULL in [u16] on Windows)
 - rust-lang#70367 (save/restore `pessimistic_yield` when entering bodies)
 - rust-lang#70822 (Don't lint for self-recursion when the function can diverge)
 - rust-lang#70868 (rustc_codegen_ssa: Refactor construction of linker arguments)
 - rust-lang#70896 (Implement Chain with Option fuses)
 - rust-lang#70916 (Support `#[track_caller]` on functions in `extern "Rust" { ... }`)
 - rust-lang#70918 (rustc_session: forbid lints override regardless of position)

Failed merges:

r? @ghost
@bors bors merged commit ecc4e2a into rust-lang:master Apr 9, 2020
chrysn added a commit to chrysn-pull-requests/typed-html that referenced this pull request Apr 22, 2020
Changes[1] to the Rust standard library are affecting[2] the nesting
size of types to the point where typed-html can not be built on nightly.

This resolves the issue by increasing the acceptable recursion depth for
the crate.

[1]: rust-lang/rust#70896
[1]: rust-lang/rust#71359

Closes: bodil#112
chrysn added a commit to chrysn-pull-requests/typed-html that referenced this pull request Apr 22, 2020
Changes[1] to the Rust standard library are affecting[2] the nesting
size of types to the point where typed-html can not be built on nightly.

This resolves the issue by increasing the acceptable recursion depth for
the crate.

[1]: rust-lang/rust#70896
[2]: rust-lang/rust#71359

Closes: bodil#112
@pnkfelix
Copy link
Member

This seems like it injected regression noted in issue #71359.

The regression in question might be fairly niche.

But I still wanted to check in about whether the benefit from this PR is significant enough to justify the risk associated with issue #71359?

cc @cuviper and @scottmcm

@cuviper
Copy link
Member Author

cuviper commented May 21, 2020

IMO that's niche enough to not worry about -- they were using a very long chain, but even that is now resolved by bodil/typed-html#117 (though not published yet). Plus, that project is already rife with increased recursion_limit, as high as 1024 in one example.

Also, this PR had an incidental benefit on the very long compilation time in #70749 -- that code now compiles quickly with the Option-based Chain.

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

6 participants