Cleanup resolve #55144

ljedrz · 2018-10-17T11:00:31Z

improve/remove allocations
truncate instead of popping in a loop
improve common patterns

rust-highfive · 2018-10-17T11:00:34Z

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

rust-highfive · 2018-10-17T11:50:57Z

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.

[00:47:00] .................................................................................................... 1400/4605
[00:47:02] .........................................................i.......................................... 1500/4605
[00:47:06] ........................i........................................................................... 1600/4605
[00:47:09] .................................................................................................... 1700/4605
[00:47:12] ..................................................F................................................. 1800/4605
[00:47:19] .................................................................................................... 2000/4605
[00:47:23] .................................................................................................... 2100/4605
[00:47:27] .................................................................................................... 2200/4605
[00:47:31] .................................................................................................... 2300/4605
---
[00:48:50] ---- [ui] ui/issues/issue-11692-2.rs stdout ----
[00:48:50] diff of stderr:
[00:48:50] 
[00:48:50] 3    |
[00:48:50] 4 LL |     concat!(test!()); //~ ERROR cannot find macro `test!` in this scope
[00:48:50] +    |
[00:48:50] +    |
[00:48:50] +    = help: have you added the `#[macro_use]` on the module/import?
[00:48:50] 7 error: aborting due to previous error
[00:48:50] 8 
[00:48:50] 
[00:48:50] 
[00:48:50] 
[00:48:50] The actual stderr differed from the expected stderr.
[00:48:50] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-11692-2/issue-11692-2.stderr
[00:48:50] To update references, rerun the tests and pass the `--bless` flag
[00:48:50] To only update this specific test, also pass `--test-args issues/issue-11692-2.rs`
[00:48:50] error: 1 errors occurred comparing output.
[00:48:50] status: exit code: 1
[00:48:50] status: exit code: 1
[00:48:50] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/issues/issue-11692-2.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-11692-2/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-11692-2/auxiliary" "-A" "unused"
[00:48:50] ------------------------------------------
[00:48:50] 
[00:48:50] ------------------------------------------
[00:48:50] stderr:
[00:48:50] stderr:
[00:48:50] ------------------------------------------
[00:48:50] {"message":"cannot find macro `test!` in this scope","code":null,"level":"error","spans":[{"file_name":"/checkout/src/test/ui/issues/issue-11692-2.rs","byte_start":491,"byte_end":495,"line_start":12,"line_end":12,"column_start":13,"column_end":17,"is_primary":true,"text":[{"text":"    concat!(test!()); //~ ERROR cannot find macro `test!` in this scope","highlight_start":13,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"have you added the `#[macro_use]` on the module/import?","code":null,"level":"help","spans":[],"children":[],"rendered":null}],"rendered":"error: cannot find macro `test!` in this scope\n  --> /checkout/src/test/ui/issues/issue-11692-2.rs:12:13\n   |\nLL |     concat!(test!()); //~ ERROR cannot find macro `test!` in this scope\n   |             ^^^^\n   |\n   = help: have you added the `#[macro_use]` on the module/import?\n\n"}
[00:48:50] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[00:48:50] ------------------------------------------
[00:48:50] 
[00:48:50] thread '[ui] ui/issues/issue-11692-2.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3277:9
[00:48:50] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:48:50] 
[00:48:50] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:501:22
[00:48:50] 
[00:48:50] 
[00:48:50] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:48:50] 
[00:48:50] 
[00:48:50] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:48:50] Build completed unsuccessfully in 0:03:33
[00:48:50] Build completed unsuccessfully in 0:03:33
[00:48:50] Makefile:58: recipe for target 'check' failed
[00:48:50] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:001b5a5a
$ 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)

Mark-Simulacrum · 2018-10-17T13:03:33Z

src/librustc_resolve/lib.rs

-        }
-        self.ribs[ValueNS].pop();
+        let valuens_len = self.ribs[ValueNS].len();
+        self.ribs[ValueNS].truncate(valuens_len - num_macro_definition_ribs - 1);


hm, why is this offset by 1 but the next one not?

This preserves the existing logic; self.ribs[ValueNS] was popped once more after the loop.

Please, keep the old obvious logic (pops are symmetric with pushes above), unless you have benchmarks.

@petrochenkov truncating is faster - the more elements to pop, the faster it becomes.

I know that it's faster, I meant that it doesn't matter that it's faster, because the speed up is unnoticeable in the general picture. In this case readability/simplicity is very much preferable.

(By benchmarks I mean e.g. benchmark showing that this specific code is hot and takes a significant percent of resolve time.)

Unfortunately I don't have a Linux rig to do perf runs :/. I can revert to the original code if you don't think this piece of code is hot enough.

Mark-Simulacrum · 2018-10-17T13:05:03Z

src/librustc_resolve/macros.rs

@@ -277,11 +275,12 @@ impl<'a, 'crateloader: 'a> base::Resolver for Resolver<'a, 'crateloader> {
                    if traits.is_empty() {
                        attrs.remove(i);
                    } else {
-                        let mut tokens = Vec::new();
+                        let mut tokens = Vec::with_capacity(traits.len().saturating_sub(1));


No real need for the saturating_sub here since we've already checked if traits is empty (it's not)

I'd say there's no real need to pollute code with reserves/capacities unless there are benchmarks.

(Also, this specific code is being removed in #54271.)

@petrochenkov push loops with specified capacity are always faster than with an empty Vec.

Mark-Simulacrum · 2018-10-17T13:06:01Z

src/librustc_resolve/macros.rs

@@ -931,7 +930,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
        }.or_else(|| {
            let names = self.builtin_macros.iter().chain(self.macro_use_prelude.iter())
                                                  .filter_map(|(name, binding)| {
-                if binding.macro_kind() == Some(kind) { Some(name) } else { None }
+                if binding.macro_kind().is_some() { Some(name) } else { None }


This is plausibly a change in behavior?

Whoops, I misread it as an if let; this might be the reason behind the error (I'll check on my testing rig before updating).

petrochenkov · 2018-10-17T16:49:45Z

Please, keep asserts in place, they are more readable and in the long term will give the same diagnostics as assert_eq (#44838).

petrochenkov · 2018-10-17T16:51:17Z

src/librustc_resolve/lib.rs

-            ).collect();
-            if fake_self.contains(&item_str)
+            );
+            if fake_self.any(|ident| ident == *item_str)


I think you can just use ["this", "my"].contains(&*item_str.as_str()) here.

Almost; ["this", "my"].contains(&&*item_str.as_str()).

ljedrz · 2018-10-17T18:01:04Z

@petrochenkov I fixed the test error (it was the one I expected), rolled back asserts (they compared against fixed values anyway) and the other pieces you requested. I just left the vectors with specified capacity, since that will be shortly overwritten anyway.

petrochenkov · 2018-10-17T18:42:43Z

Thanks!
@bors r+

bors · 2018-10-17T18:42:44Z

📌 Commit 89c20b7 has been approved by petrochenkov

Cleanup resolve - improve/remove allocations - `truncate` instead of `pop`ping in a loop - improve common patterns

Rollup of 7 pull requests Successful merges: - #54300 (Updated RELEASES.md for 1.30.0) - #55013 ([NLL] Propagate bounds from generators) - #55071 (Fix ICE and report a human readable error) - #55144 (Cleanup resolve) - #55166 (Don't warn about parentheses on `match (return)`) - #55169 (Add a `copysign` function to f32 and f64) - #55178 (Stabilize slice::chunks_exact(), chunks_exact_mut(), rchunks(), rchunks_mut(), rchunks_exact(), rchunks_exact_mut())

rust-highfive assigned eddyb Oct 17, 2018

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

petrochenkov assigned petrochenkov and unassigned eddyb Oct 17, 2018

Mark-Simulacrum reviewed Oct 17, 2018

View reviewed changes

petrochenkov reviewed Oct 17, 2018

View reviewed changes

petrochenkov 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 Oct 17, 2018

ljedrz added 2 commits October 17, 2018 19:57

resolve: improve common patterns

da40916

resolve: improve/remove allocations

89c20b7

ljedrz force-pushed the cleanup_resolve branch from eef2fe9 to 89c20b7 Compare October 17, 2018 17:57

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 17, 2018

kennytm mentioned this pull request Oct 18, 2018

Rollup of 9 pull requests #55181

Closed

kennytm added a commit to kennytm/rust that referenced this pull request Oct 18, 2018

Rollup merge of rust-lang#55144 - ljedrz:cleanup_resolve, r=petrochenkov

d398c16

Cleanup resolve - improve/remove allocations - `truncate` instead of `pop`ping in a loop - improve common patterns

kennytm mentioned this pull request Oct 19, 2018

Rollup of 7 pull requests #55194

Merged

kennytm added a commit to kennytm/rust that referenced this pull request Oct 19, 2018

Rollup merge of rust-lang#55144 - ljedrz:cleanup_resolve, r=petrochenkov

8d712aa

Cleanup resolve - improve/remove allocations - `truncate` instead of `pop`ping in a loop - improve common patterns

bors merged commit 89c20b7 into rust-lang:master Oct 19, 2018

ljedrz deleted the cleanup_resolve branch October 19, 2018 12:16

SimonSapin mentioned this pull request Oct 22, 2018

ICE 'allocation missing in dead_alloc_map' in rustc_mir::interpret when compiling Servo #55223

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup resolve #55144

Cleanup resolve #55144

ljedrz commented Oct 17, 2018

rust-highfive commented Oct 17, 2018

rust-highfive commented Oct 17, 2018

Mark-Simulacrum Oct 17, 2018

ljedrz Oct 17, 2018

petrochenkov Oct 17, 2018

ljedrz Oct 17, 2018 •

edited

Loading

petrochenkov Oct 17, 2018

ljedrz Oct 17, 2018 •

edited

Loading

Mark-Simulacrum Oct 17, 2018

petrochenkov Oct 17, 2018

petrochenkov Oct 17, 2018

ljedrz Oct 17, 2018

Mark-Simulacrum Oct 17, 2018

ljedrz Oct 17, 2018 •

edited

Loading

petrochenkov commented Oct 17, 2018

petrochenkov Oct 17, 2018

ljedrz Oct 17, 2018

ljedrz commented Oct 17, 2018

petrochenkov commented Oct 17, 2018

bors commented Oct 17, 2018

Cleanup resolve #55144

Cleanup resolve #55144

Conversation

ljedrz commented Oct 17, 2018

rust-highfive commented Oct 17, 2018

rust-highfive commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljedrz Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljedrz Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljedrz Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

petrochenkov commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljedrz commented Oct 17, 2018

petrochenkov commented Oct 17, 2018

bors commented Oct 17, 2018

ljedrz Oct 17, 2018 •

edited

Loading

ljedrz Oct 17, 2018 •

edited

Loading

ljedrz Oct 17, 2018 •

edited

Loading