Tweak query code for performance by Zoxc · Pull Request #56613 · rust-lang/rust

Zoxc · 2018-12-07T22:39:34Z

Split from #56509

bjorn3 · 2018-12-08T09:53:00Z

src/librustc_data_structures/lib.rs

 pub use rustc_serialize::hex::ToHex;

+#[macro_export]
+macro_rules! likely {


This macro isn't used.

It gets used in #56614. Might as well add them both

Didn't look at that PR :)

michaelwoerister · 2018-12-10T12:59:08Z

@bors try

bors · 2018-12-10T12:59:20Z

⌛ Trying commit ac56bf3 with merge c89a860...

@michaelwoerister

Tweak query code for performance Split from #56509 r? @michaelwoerister

nikic · 2018-12-10T13:08:01Z

src/librustc/session/mod.rs

-        if self.opts.debugging_opts.self_profile || self.opts.debugging_opts.profile_json {
-            let mut profiler = self.self_profiling.borrow_mut();
-            f(&mut profiler);
+        if unlikely!(self.self_profiling_active) {


LLVM already assigns reduced branch weights to branches postdominated by calls to cold functions, so I wouldn't expect this unlikely!() to have an effect here.

I did find a case where it didn't though, so I used likely and unlikely just in case.

I'd really like to have PGO for the Rust compiler at some point :)

bors · 2018-12-10T15:26:06Z

☀️ Test successful - status-travis
State: approved= try=True

michaelwoerister · 2018-12-10T15:40:55Z

@rust-timer build c89a860

rust-timer · 2018-12-10T15:40:56Z

Success: Queued c89a860 with parent 9567a1c, comparison URL.

rust-timer · 2018-12-10T18:29:19Z

Finished benchmarking try commit c89a860

michaelwoerister · 2018-12-11T09:45:05Z

src/librustc/hir/map/mod.rs

        &self.krate
    }
+
+    pub fn untracked_krate<'hir>(&'hir self) -> &'hir Crate {


Please add a warning comment here not to use this function and that it only exists for internal use with the dep-tracking system.

michaelwoerister · 2018-12-11T10:07:30Z

src/librustc/ty/query/plumbing.rs

+            {
+                if let Err(cycle) = job.await(tcx, span) {
+                    return TryGetJob::JobCompleted(Err(cycle));
+                }


I think the following would be more readable:

if cfg!(parallel_queries) { if let Err(cycle) = job.await(tcx, span) { return TryGetJob::JobCompleted(Err(cycle)); } } else { // If we get here in non-parallel mode we know that we have cycle error. let result = job.await(tcx, span); debug_assert!(result.is_err()); return result; }

That doesn't work since await now has different signatures depending on cfg!(parallel_queries). I'll add the comment though.

michaelwoerister · 2018-12-11T10:19:31Z

src/librustc/dep_graph/dep_node.rs

+            #[inline(always)]
            pub fn from_def_path_hash(kind: DepKind,
                                      def_path_hash: DefPathHash)
                                      -> DepNode {


You can turn the assertion below into a debug_assertion.

michaelwoerister · 2018-12-11T10:19:53Z

src/librustc/dep_graph/dep_node.rs

This can be turned into a debug_assertion too.

michaelwoerister · 2018-12-11T10:29:02Z

src/librustc/dep_graph/dep_node.rs

Why are is_input and has_params split but not is_anon and is_eval_always? I'd prefer not to split them into two methods. Maybe, with some of the assertions in here turned into debug-assertions, a regular #[inline] would suffice.

is_anon and is_eval_always has uses where inlining them might not be beneficial. A regular #[inline] does not suffice since these are large functions and LLVM won't inline them unless you tell it to. I use the *_inlined variants for calls when LLVM can optimize it all away.

I want to turn these properties into associated constants anyway, but that is blocked on #56462.

I think switching to debug_assertions will remove most of the hot calls to those functions. Since they might be replaced soon anyway, I'd prefer not to split the functions in order to avoid churn. You can just mark them as #[inline(always)] if performance is bad otherwise.

michaelwoerister · 2018-12-11T10:34:05Z

Thanks, @Zoxc! I left some comments. You could also try to turn from_def_path_hash, to_dep_node, and new_no_params into const fns. They seem to be mostly called with constant arguments.

rust-highfive · 2018-12-11T18:00:50Z

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:0da1c2f0:start=1544550402261983730,finish=1544550404352765289,duration=2090781559
$ 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:39] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:39] tidy error: /checkout/src/librustc/hir/map/mod.rs:164: trailing whitespace
[00:03:40] some tidy checks failed
[00:03:40] 
[00:03:40] 
[00:03:40] 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" "--quiet"
[00:03:40] 
[00:03:40] 
[00:03:40] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:40] Build completed unsuccessfully in 0:00:56
[00:03:40] Build completed unsuccessfully in 0:00:56
[00:03:40] Makefile:79: recipe for target 'tidy' failed
[00:03:40] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2b3b7279
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Dec 11 17:50:33 UTC 2018
---
travis_time:end:13d338e0:start=1544550634283219523,finish=1544550634289726110,duration=6506587
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1c54bb64
$ 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:1093ca94
travis_time:start:1093ca94
$ 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:020a79c8
$ 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)

michaelwoerister · 2018-12-14T12:19:52Z

Looks like the nits have been addressed. Thanks, @Zoxc!

@bors r+

bors · 2018-12-14T12:19:53Z

📌 Commit 64d54e61b70245c0aa660c888c7545206ca872b1 has been approved by michaelwoerister

bors · 2018-12-16T02:15:22Z

⌛ Testing commit 64d54e61b70245c0aa660c888c7545206ca872b1 with merge fd5abe90bc2c0934babdd742ace6d7618035d0e2...

bors · 2018-12-16T02:57:58Z

💔 Test failed - status-appveyor

Zoxc · 2018-12-16T03:05:03Z

Hmm.. a crash, maybe #[thread_local] is to blame?

…me comments

Zoxc · 2018-12-17T20:57:41Z

@bors r=michaelwoerister

bors · 2018-12-17T20:57:42Z

📌 Commit f0adf5a has been approved by michaelwoerister

bors · 2018-12-19T20:13:47Z

⌛ Testing commit f0adf5a with merge 790f4c5...

@michaelwoerister

Tweak query code for performance Split from #56509 r? @michaelwoerister

bors · 2018-12-19T22:51:35Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 790f4c5 to master...

rust-highfive assigned michaelwoerister Dec 7, 2018

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

bjorn3 reviewed Dec 8, 2018

View reviewed changes

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

Auto merge of #56613 - Zoxc:query-perf1, r=<try>

c89a860

Tweak query code for performance Split from #56509 r? @michaelwoerister

nikic reviewed Dec 10, 2018

View reviewed changes

michaelwoerister reviewed Dec 11, 2018

View reviewed changes

src/librustc/dep_graph/dep_node.rs Outdated

Copy link

Member

michaelwoerister Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be turned into a debug_assertion too.

michaelwoerister reviewed Dec 11, 2018

View reviewed changes

Zoxc force-pushed the query-perf1 branch from ac56bf3 to c4737a3 Compare December 11, 2018 16:18

Zoxc force-pushed the query-perf1 branch from 1da5925 to 6360ae7 Compare December 11, 2018 18:17

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 Dec 14, 2018

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 Dec 16, 2018

Zoxc added 6 commits December 17, 2018 08:53

Tweak query code for performance

c66b844

Turn some asserts into debug_asserts

8e53ecf

Rename await into cycle_error for the single threaded case and add so…

9d888ea

…me comments

Add a comment

517725c

Remove *_inlined variants

5a6a133

Revert thread-local changes

f0adf5a

Zoxc force-pushed the query-perf1 branch from 64d54e6 to f0adf5a Compare December 17, 2018 07:53

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 Dec 17, 2018

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

Auto merge of #56613 - Zoxc:query-perf1, r=michaelwoerister

790f4c5

Tweak query code for performance Split from #56509 r? @michaelwoerister

bors merged commit f0adf5a into rust-lang:master Dec 19, 2018

bors mentioned this pull request Dec 19, 2018

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

Closed

Zoxc deleted the query-perf1 branch December 26, 2018 14:46

Uh oh!

Conversation

Zoxc commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Dec 10, 2018

Uh oh!

bors commented Dec 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Dec 10, 2018

Uh oh!

michaelwoerister commented Dec 10, 2018

Uh oh!

rust-timer commented Dec 10, 2018

Uh oh!

rust-timer commented Dec 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Dec 11, 2018

Uh oh!

rust-highfive commented Dec 11, 2018

Uh oh!

michaelwoerister commented Dec 14, 2018

Uh oh!

bors commented Dec 14, 2018

Uh oh!

bors commented Dec 16, 2018

Uh oh!

bors commented Dec 16, 2018

Uh oh!

Zoxc commented Dec 16, 2018

Uh oh!

Zoxc commented Dec 17, 2018

Uh oh!

bors commented Dec 17, 2018

Uh oh!

bors commented Dec 19, 2018

Uh oh!

bors commented Dec 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments