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

Rollup of 18 pull requests #50611

Merged
merged 54 commits into from
May 11, 2018
Merged

Rollup of 18 pull requests #50611

merged 54 commits into from
May 11, 2018

Conversation

alexcrichton
Copy link
Member

Successful merges:

Failed merges:

ExpHP and others added 30 commits April 30, 2018 07:36
A previous PR fixed one method that was legitimately buggy;
this cleans up the rest to be less diverse, mirroring the
corresponding impls on [T] to the greatest extent possible
without introducing any unnecessary UTF-8 boundary checks at 0.
GitHub users: I think you can add ?w=1 to the url
for a vastly cleaner whitespace-ignoring diff
m*n lines of implementation deserves m*n lines of tests
Because they are simple and hot.

This change speeds up some incremental runs of a few rustc-perf
benchmarks, the best by 3%.
This avoids a decent number of allocations, enough to speed up
incremental runs of many rustc-benchmarks, the best by 2%.
This commit is spawned out of a performance regression investigation in rust-lang#50496.
In tracking down this regression it turned out that the `expand_statements`
function in the compiler was taking quite a long time. Further investigation
showed two key properties:

* The function was "fast" on glibc 2.24 and slow on glibc 2.23
* The hottest function was memmove from glibc

Combined together it looked like glibc gained an optimization to the memmove
function in 2.24. Ideally we don't want to rely on this optimization, so I
wanted to dig further to see what was happening.

The hottest part of `expand_statements` was `Drop for Drain` in the call to
`splice` where we insert new statements into the original vector. This *should*
be a cheap operation because we're draining and replacing iterators of the exact
same length, but under the hood memmove was being called a lot, causing a
slowdown on glibc 2.23.

It turns out that at least one of the optimizations in glibc 2.24 was that
`memmove` where the src/dst are equal becomes much faster. [This program][prog]
executes in ~2.5s against glibc 2.23 and ~0.3s against glibc 2.24, exhibiting
how glibc 2.24 is optimizing `memmove` if the src/dst are equal.

And all that brings us to what this commit itself is doing. The change here is
purely to `Drop for Drain` to avoid the call to `ptr::copy` if the region being
copied doesn't actually need to be copied. For normal usage of just `Drain`
itself this check isn't really necessary, but because `Splice` internally
contains `Drain` this provides a nice speed boost on glibc 2.23. Overall this
should fix the regression seen in rust-lang#50496 on glibc 2.23 and also fix the
regression on Windows where `memmove` looks to not have this optimization.

Note that the way `splice` was called in `expand_statements` would cause a
quadratic number of elements to be copied via `memmove` which is likely why the
tuple-stress benchmark showed such a severe regression.

Closes rust-lang#50496

[prog]: https://gist.github.com/alexcrichton/c05bc51c6771bba5ae5b57561a6c1cd3
also make a drive-by typo fix
@bors
Copy link
Contributor

bors commented May 10, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2018
@alexcrichton
Copy link
Member Author

@bors: r+

@bors
Copy link
Contributor

bors commented May 10, 2018

📌 Commit a46d1dd has been approved by alexcrichton

@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 10, 2018
@bors
Copy link
Contributor

bors commented May 10, 2018

⌛ Testing commit a46d1ddc89d1a3a52d187fcd57c388a9283dd09e with merge e0f3a0a2cfa2e9245e425f1207c5d44118653563...

@bors
Copy link
Contributor

bors commented May 10, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 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.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/18/61/4e0f977cfe063188d73622a91cab8b8b409b662f422303fc687f362d941f/awscli-1.15.18-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 37.9MB/s eta 0:00:01
    1% |▌                               | 20kB 1.8MB/s eta 0:00:01
    2% |▉                               | 30kB 2.1MB/s eta 0:00:01
    3% |█                               | 40kB 1.8MB/s eta 0:00:01
---
[00:00:11] travis_time:end:2ab1c090:start=1525985727743557782,finish=1525985739345480126,duration=11601922344
travis_fold:start:build_docker
travis_time:start:0f38db4c
Attempting to download s3://rust-lang-ci-sccache2/docker/de94e11fb622ebd51292930689046e4dcd653833568b97f8dbcebaae8181ce5ef0a7935c29e835bdd3f7ec37b1c23cf77eb4686cd315c7beefdafb75370cd2ba
[00:00:11] Attempting with retry: curl -f -L -C - -o /tmp/rustci_docker_cache https://s3-us-west-1.amazonaws.com/rust-lang-ci-sccache2/docker/de94e11fb622ebd51292930689046e4dcd653833568b97f8dbcebaae8181ce5ef0a7935c29e835bdd3f7ec37b1c23cf77eb4686cd315c7beefdafb75370cd2ba
[00:00:11]                                  Dload  Upload   Total   Spent    Left  Speed
[00:00:18] 
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0  234M    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
---
[00:05:13] * wrapping_int_impl                lib      unstable     None    
[00:05:13] * wrapping_iter_arith              lib      stable       1.14.0  
[00:05:13] * wrapping_neg                     lib      stable       1.10.0  
[00:05:13] * wrapping_ref                     lib      stable       1.14.0  
[00:05:13] tidy error: /checkout/src/liballoc/tests/str.rs:482: platform-specific cfg: cfg(not(target_os = "emscripten"))
[00:05:14] some tidy checks failed
[00:05:14] 
[00:05:14] 
[00:05:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
[00:05:14] 
[00:05:14] 
[00:05:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:14] Build completed unsuccessfully in 0:02:05
[00:05:14] Build completed unsuccessfully in 0:02:05
[00:05:14] Makefile:79: recipe for target 'tidy' failed
[00:05:14] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:09488b0c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

1 similar comment
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 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.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/18/61/4e0f977cfe063188d73622a91cab8b8b409b662f422303fc687f362d941f/awscli-1.15.18-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 37.9MB/s eta 0:00:01
    1% |▌                               | 20kB 1.8MB/s eta 0:00:01
    2% |▉                               | 30kB 2.1MB/s eta 0:00:01
    3% |█                               | 40kB 1.8MB/s eta 0:00:01
---
[00:00:11] travis_time:end:2ab1c090:start=1525985727743557782,finish=1525985739345480126,duration=11601922344
travis_fold:start:build_docker
travis_time:start:0f38db4c
Attempting to download s3://rust-lang-ci-sccache2/docker/de94e11fb622ebd51292930689046e4dcd653833568b97f8dbcebaae8181ce5ef0a7935c29e835bdd3f7ec37b1c23cf77eb4686cd315c7beefdafb75370cd2ba
[00:00:11] Attempting with retry: curl -f -L -C - -o /tmp/rustci_docker_cache https://s3-us-west-1.amazonaws.com/rust-lang-ci-sccache2/docker/de94e11fb622ebd51292930689046e4dcd653833568b97f8dbcebaae8181ce5ef0a7935c29e835bdd3f7ec37b1c23cf77eb4686cd315c7beefdafb75370cd2ba
[00:00:11]                                  Dload  Upload   Total   Spent    Left  Speed
[00:00:18] 
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0  234M    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
---
[00:05:13] * wrapping_int_impl                lib      unstable     None    
[00:05:13] * wrapping_iter_arith              lib      stable       1.14.0  
[00:05:13] * wrapping_neg                     lib      stable       1.10.0  
[00:05:13] * wrapping_ref                     lib      stable       1.14.0  
[00:05:13] tidy error: /checkout/src/liballoc/tests/str.rs:482: platform-specific cfg: cfg(not(target_os = "emscripten"))
[00:05:14] some tidy checks failed
[00:05:14] 
[00:05:14] 
[00:05:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
[00:05:14] 
[00:05:14] 
[00:05:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:14] Build completed unsuccessfully in 0:02:05
[00:05:14] Build completed unsuccessfully in 0:02:05
[00:05:14] Makefile:79: recipe for target 'tidy' failed
[00:05:14] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:09488b0c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

@alexcrichton
Copy link
Member Author

@bors: r+

Thanks tidy...

@bors
Copy link
Contributor

bors commented May 10, 2018

📌 Commit 2c5d13d has been approved by alexcrichton

@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 10, 2018
@bors
Copy link
Contributor

bors commented May 10, 2018

⌛ Testing commit 2c5d13d with merge a006328...

bors added a commit that referenced this pull request May 10, 2018
Rollup of 18 pull requests

Successful merges:

 - #49423 (Extend tests for RFC1598 (GAT))
 - #50010 (Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check))
 - #50447 (Fix update-references for tests within subdirectories.)
 - #50514 (Pull in a wasm fix from LLVM upstream)
 - #50524 (Make DepGraph::previous_work_products immutable)
 - #50532 (Don't use Lock for heavily accessed CrateMetadata::cnum_map.)
 - #50538 ( Make CrateNum allocation more thread-safe. )
 - #50564 (Inline `Span` methods.)
 - #50565 (Use SmallVec for DepNodeIndex within dep_graph.)
 - #50569 (Allow for specifying a linker plugin for cross-language LTO)
 - #50572 (Clarify in the docs that `mul_add` is not always faster.)
 - #50574 (add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (#49022))
 - #50575 (std: Avoid `ptr::copy` if unnecessary in `vec::Drain`)
 - #50588 (Move "See also" disambiguation links for primitive types to top)
 - #50590 (Fix tuple struct field spans)
 - #50591 (Restore RawVec::reserve* documentation)
 - #50598 (Remove unnecessary mutable borrow and resizing in DepGraph::serialize)
 - #50606 (Retry when downloading the Docker cache.)

Failed merges:

 - #50161 (added missing implementation hint)
 - #50558 (Remove all reference to DepGraph::work_products)
@bors
Copy link
Contributor

bors commented May 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing a006328 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR which is a rollup 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.