Make `SortedMap::new` const, also faster `is_empty` #56969

llogiq · 2018-12-19T01:47:08Z

(The latter is because [T]::is_empty compiles to a single pointer comparison, whereas len() == 0 first subtracts the pointers and only then compares with zero)

rust-highfive · 2018-12-19T01:47:11Z

r? @davidtwco

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

rust-highfive · 2018-12-19T01:54:47Z

The job x86_64-gnu-llvm-6.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:260093f8:start=1545184131617532413,finish=1545184132753039487,duration=1135507074
$ 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-6.0
---
[00:05:33]    Compiling backtrace v0.3.11
[00:05:34]    Compiling parking_lot v0.6.4
[00:05:36]    Compiling rustc-rayon v0.1.1
travis_time:start:0247b6fc
$ 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:177ba5db
travis_time:start:177ba5db
$ 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:start:33988aee
$ dmesg | grep -i kill

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)

sinkuu · 2018-12-19T07:48:14Z

Vec (self.data) has len field, and a slice is (ptr, len), right? It seems strange if len() == 0 involves pointer subtraction. len() == 0 -> is_empty() is a nice cleanup though.

rust-highfive · 2018-12-19T11:50:07Z

The job x86_64-gnu-llvm-6.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:0212eb17:start=1545219886964955679,finish=1545219889233250839,duration=2268295160
$ 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-6.0

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)

rust-highfive · 2018-12-19T12:04:25Z

The job x86_64-gnu-llvm-6.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:002583c8:start=1545220725012526478,finish=1545220726029633601,duration=1017107123
$ 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-6.0
---
[00:05:23]    |                   ^^^^^^ allocation not allowed in constant functions
[00:05:23]    |
[00:05:23]    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
[00:05:23] 
[00:05:23] error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants
[00:05:23]   --> src/librustc_data_structures/sorted_map.rs:35:19
[00:05:23] 35 |             data: vec![]
[00:05:23]    |                   ^^^^^^
[00:05:23]    |
[00:05:23]    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
---
[00:05:24] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:05:24] expected success, got: exit code: 101
[00:05:24] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:05:24] Build completed unsuccessfully in 0:01:59
[00:05:24] make: *** [all] Error 1
[00:05:24] Makefile:28: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0c39a6b0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Dec 19 12:04:18 UTC 2018
---
travis_time:end:0d502902:start=1545221059383467347,finish=1545221059388869534,duration=5402187
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0bb124fc
$ 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:0070f886
travis_time:start:0070f886
$ 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:start:01c3157c
$ dmesg | grep -i kill

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)

oli-obk

While these can be const fn, we've previously opted not to make internal functions const fn without concrete use cases

oli-obk · 2018-12-19T13:20:30Z

src/librustc_data_structures/sorted_map.rs

@@ -30,7 +30,7 @@ pub struct SortedMap<K: Ord, V> {

 impl<K: Ord, V> SortedMap<K, V> {
    #[inline]
-    pub fn new() -> SortedMap<K, V> {
+    pub const fn new() -> SortedMap<K, V> {
        SortedMap {
            data: vec![]


You need to use Vec::new() here

rust-highfive · 2018-12-19T16:17:12Z

The job x86_64-gnu-llvm-6.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:01f02d68:start=1545235788330528876,finish=1545235843265671827,duration=54935142951
$ 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-6.0
---
[00:05:16]    |                   ^^^^^^ allocation not allowed in constant functions
[00:05:16]    |
[00:05:16]    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
[00:05:16] 
[00:05:16] error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants
[00:05:16]   --> src/librustc_data_structures/sorted_map.rs:35:19
[00:05:16] 35 |             data: vec![]
[00:05:16]    |                   ^^^^^^
[00:05:16]    |
[00:05:16]    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
---
[00:05:16] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:05:16] expected success, got: exit code: 101
[00:05:16] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:05:16] Build completed unsuccessfully in 0:02:00
[00:05:16] Makefile:28: recipe for target 'all' failed
[00:05:16] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:01ab4770
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Dec 19 16:16:10 UTC 2018
---
travis_time:end:04163b64:start=1545236170886325549,finish=1545236170892816231,duration=6490682
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:035cd859
$ 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:1d614368
travis_time:start:1d614368
$ 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:start:00ecb710
$ dmesg | grep -i kill

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)

bors · 2018-12-19T23:05:14Z

☔ The latest upstream changes (presumably #56613) made this pull request unmergeable. Please resolve the merge conflicts.

scottmcm · 2018-12-20T02:44:09Z

I'm all in favour of replacing .len() == 0 with .is_empty(), but it's not a perf fix. LLVM is quite capable of changing a-b==0 into a == b. (And data is a Vec anyway, which implements is_empty() as len() == 0: https://doc.rust-lang.org/src/alloc/vec.rs.html#1184-1186)

Dylan-DPC-zz · 2019-01-14T18:48:43Z

ping from triage @davidtwco can you review this?
@llogiq you have a few conflicts to attend to.

davidtwco · 2019-01-14T18:51:01Z

ping from triage @davidtwco can you review this?

I've been checking regularly to see if the conflicts have been resolved before reviewing.

Dylan-DPC-zz · 2019-01-14T18:53:19Z

Okay. Updated the label to reflect the state.

llogiq · 2019-01-21T07:22:07Z

This is more trouble than it's worth. Closing.

rust-highfive assigned davidtwco Dec 19, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2018

llogiq force-pushed the const_new_sorted_map branch from b1a39d1 to 0e57f26 Compare December 19, 2018 11:43

llogiq force-pushed the const_new_sorted_map branch from 0e57f26 to ef5423b Compare December 19, 2018 11:57

oli-obk reviewed Dec 19, 2018

View reviewed changes

llogiq force-pushed the const_new_sorted_map branch from ef5423b to 0e6d1e9 Compare December 19, 2018 16:08

Make SortedMap::new const, also faster is_empty.

2430870

llogiq force-pushed the const_new_sorted_map branch from 0e6d1e9 to 2430870 Compare December 19, 2018 16:29

Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2019

llogiq closed this Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `SortedMap::new` const, also faster `is_empty` #56969

Make `SortedMap::new` const, also faster `is_empty` #56969

llogiq commented Dec 19, 2018

rust-highfive commented Dec 19, 2018

rust-highfive commented Dec 19, 2018

sinkuu commented Dec 19, 2018 •

edited

Loading

rust-highfive commented Dec 19, 2018

rust-highfive commented Dec 19, 2018

oli-obk left a comment

oli-obk Dec 19, 2018

rust-highfive commented Dec 19, 2018

bors commented Dec 19, 2018

scottmcm commented Dec 20, 2018

Dylan-DPC-zz commented Jan 14, 2019

davidtwco commented Jan 14, 2019

Dylan-DPC-zz commented Jan 14, 2019

llogiq commented Jan 21, 2019

Make SortedMap::new const, also faster is_empty #56969

Make SortedMap::new const, also faster is_empty #56969

Conversation

llogiq commented Dec 19, 2018

rust-highfive commented Dec 19, 2018

rust-highfive commented Dec 19, 2018

sinkuu commented Dec 19, 2018 • edited Loading

rust-highfive commented Dec 19, 2018

rust-highfive commented Dec 19, 2018

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk Dec 19, 2018

Choose a reason for hiding this comment

rust-highfive commented Dec 19, 2018

bors commented Dec 19, 2018

scottmcm commented Dec 20, 2018

Dylan-DPC-zz commented Jan 14, 2019

davidtwco commented Jan 14, 2019

Dylan-DPC-zz commented Jan 14, 2019

llogiq commented Jan 21, 2019

Make `SortedMap::new` const, also faster `is_empty` #56969

Make `SortedMap::new` const, also faster `is_empty` #56969

sinkuu commented Dec 19, 2018 •

edited

Loading