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

Optimized string FromIterator + Extend impls #56548

Merged
merged 3 commits into from Dec 6, 2018

Conversation

Projects
None yet
4 participants
@Lucretiel
Copy link
Contributor

Lucretiel commented Dec 5, 2018

I noticed that there was a lost opportunity to reuse string buffers in FromIterator<String> and FromIterator<Cow<str>>; updated the implementations to use these. In practice this translates to at least one fewer allocation when using these APIs.

Additionally, rewrote Extend implementations to use iter.for_each, which (supposedly) helps the compiler optimize those loops (because iterator adapters are encouraged to provide optimized implementations of fold and try_fold.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 5, 2018

r? @sfackler

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

@Lucretiel

This comment has been minimized.

Copy link
Contributor

Lucretiel commented Dec 5, 2018

Fixes #51541

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 5, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:044c3220:start=1544049912417853147,finish=1544049995460392015,duration=83042538868
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:03:33]    Compiling rustc_msan v0.0.0 (/checkout/src/librustc_msan)
[00:03:33]    Compiling rustc_lsan v0.0.0 (/checkout/src/librustc_lsan)
[00:03:49]    Compiling libc v0.0.0 (/checkout/src/rustc/libc_shim)
[00:03:49]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:03:49] error: expected one of `=>`, `if`, or `|`, found `{`
[00:03:49]      |
[00:03:49] 1754 |             Some(cow) {
[00:03:49] 1754 |             Some(cow) {
[00:03:49]      |                       ^ expected one of `=>`, `if`, or `|` here
[00:03:50]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
2958716 .
1236624 ./obj
1236584 ./obj/build
---
184276 ./obj/build/cache/2018-10-30
153276 ./src/tools/clang
150340 ./obj/build/bootstrap/debug/incremental
134748 ./obj/build/bootstrap/debug/incremental/bootstrap-1plb86h2refwc
134744 ./obj/build/bootstrap/debug/incremental/bootstrap-1plb86h2refwc/s-f7bitqd6t3-asai72-jerzeai9r9z3
134544 ./.git/modules/src
115344 ./src/llvm/test/CodeGen
107888 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
107420 ./src/tools/lldb
---
travis_time:end:3c5356b0:start=1544050236676700128,finish=1544050236682471681,duration=5771553
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0705b5d4
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2f8958f8
travis_time:start:2f8958f8
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:st

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Lucretiel Lucretiel force-pushed the Lucretiel:string-extend-optimize branch from f4eba91 to 180dcc3 Dec 5, 2018

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 5, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:35f709bc:start=1544050359345521619,finish=1544050360566614207,duration=1221092588
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:03:40]    Compiling rustc_tsan v0.0.0 (/checkout/src/librustc_tsan)
[00:03:56]    Compiling libc v0.0.0 (/checkout/src/rustc/libc_shim)
[00:03:56]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:03:57]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:04:00] error[E0596]: cannot borrow `iterator` as mutable, as it is not declared as mutable
[00:04:00]      |
[00:04:00] 1735 |         let iterator = iter.into_iter();
[00:04:00]      |             -------- help: consider changing this to be mutable: `mut iterator`
[00:04:00] 1736 | 
[00:04:00] 1736 | 
[00:04:00] 1737 |         match iterator.next() {
[00:04:00]      |               ^^^^^^^^ cannot borrow as mutable
[00:04:00] 
[00:04:00] error[E0596]: cannot borrow `buf` as mutable, as it is not declared as mutable
[00:04:00]      |
[00:04:00] 1739 |             Some(buf) => {
[00:04:00]      |                  --- help: consider changing this to be mutable: `mut buf`
[00:04:00]      |                  --- help: consider changing this to be mutable: `mut buf`
[00:04:00] 1740 |                 buf.extend(iterator);

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 6, 2018

LGTM other than a quick explanatory comment!

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 6, 2018

@bors r+ rollup

Thanks!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 6, 2018

📌 Commit 811a2bf has been approved by sfackler

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 6, 2018

Rollup merge of rust-lang#56548 - Lucretiel:string-extend-optimize, r…
…=sfackler

Optimized string FromIterator + Extend impls

I noticed that there was a lost opportunity to reuse string buffers in `FromIterator<String>` and `FromIterator<Cow<str>>`; updated the implementations to use these. In practice this translates to at least one fewer allocation when using these APIs.

Additionally, rewrote `Extend` implementations to use `iter.for_each`, which (supposedly) helps the compiler optimize those loops (because iterator adapters are encouraged to provide optimized implementations of `fold` and `try_fold`.

bors added a commit that referenced this pull request Dec 6, 2018

Auto merge of #56557 - pietroalbini:rollup, r=pietroalbini
Rollup of 11 pull requests

Successful merges:

 - #56315 (Rustdoc inline macro reexport)
 - #56332 ([rustdoc] Specific crate search)
 - #56362 (Stabilise exhaustive integer patterns)
 - #56426 (libsyntax_pos: A few tweaks)
 - #56441 (rustbuild: Fix issues with compiler docs)
 - #56446 (pass the parameter environment to `traits::find_associated_item`)
 - #56500 (cleanup: remove static lifetimes from consts)
 - #56525 (Avoid extra copy and syscall in std::env::current_exe)
 - #56528 (Remove unused dependency (rustc_lint -> rustc_mir))
 - #56548 (Optimized string FromIterator + Extend impls)
 - #56553 (Don't print the profiling summary to stdout when -Zprofile-json is set)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Dec 6, 2018

Auto merge of #56557 - pietroalbini:rollup, r=pietroalbini
Rollup of 11 pull requests

Successful merges:

 - #56315 (Rustdoc inline macro reexport)
 - #56332 ([rustdoc] Specific crate search)
 - #56362 (Stabilise exhaustive integer patterns)
 - #56426 (libsyntax_pos: A few tweaks)
 - #56441 (rustbuild: Fix issues with compiler docs)
 - #56446 (pass the parameter environment to `traits::find_associated_item`)
 - #56500 (cleanup: remove static lifetimes from consts)
 - #56525 (Avoid extra copy and syscall in std::env::current_exe)
 - #56528 (Remove unused dependency (rustc_lint -> rustc_mir))
 - #56548 (Optimized string FromIterator + Extend impls)
 - #56553 (Don't print the profiling summary to stdout when -Zprofile-json is set)

Failed merges:

r? @ghost

@bors bors merged commit 811a2bf into rust-lang:master Dec 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Lucretiel Lucretiel deleted the Lucretiel:string-extend-optimize branch Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment