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

Tweak query code for performance #56613

Merged
merged 6 commits into from Dec 19, 2018

Conversation

Projects
None yet
7 participants
@Zoxc
Copy link
Contributor

Zoxc commented Dec 7, 2018

Split from #56509

r? @michaelwoerister

@@ -58,6 +60,26 @@ extern crate rustc_cratesio_shim;

pub use rustc_serialize::hex::ToHex;

#[macro_export]
macro_rules! likely {

This comment has been minimized.

@bjorn3

bjorn3 Dec 8, 2018

Contributor

This macro isn't used.

This comment has been minimized.

@Zoxc

Zoxc Dec 8, 2018

Contributor

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

This comment has been minimized.

@bjorn3

bjorn3 Dec 8, 2018

Contributor

Didn't look at that PR :)

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 10, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 10, 2018

⌛️ Trying commit ac56bf3 with merge c89a860...

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

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

Split from #56509

r? @michaelwoerister
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) {

This comment has been minimized.

@nikic

nikic Dec 10, 2018

Contributor

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.

This comment has been minimized.

@Zoxc

Zoxc Dec 10, 2018

Contributor

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

This comment has been minimized.

@michaelwoerister

michaelwoerister Dec 10, 2018

Contributor

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 10, 2018

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

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 10, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 10, 2018

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 10, 2018

Finished benchmarking try commit c89a860

@@ -159,6 +159,10 @@ impl Forest {
self.dep_graph.read(DepNode::new_no_params(DepKind::Krate));
&self.krate
}

pub fn untracked_krate<'hir>(&'hir self) -> &'hir Crate {

This comment has been minimized.

@michaelwoerister

michaelwoerister Dec 11, 2018

Contributor

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

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

This comment has been minimized.

@michaelwoerister

michaelwoerister Dec 11, 2018

Contributor

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;
}

This comment has been minimized.

@Zoxc

Zoxc Dec 11, 2018

Contributor

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

This comment has been minimized.

@michaelwoerister
@@ -299,7 +310,7 @@ macro_rules! define_dep_nodes {
/// Construct a DepNode from the given DepKind and DefPathHash. This
/// method will assert that the given DepKind actually requires a
/// single DefId/DefPathHash parameter.
#[inline]
#[inline(always)]
pub fn from_def_path_hash(kind: DepKind,
def_path_hash: DefPathHash)
-> DepNode {

This comment has been minimized.

@michaelwoerister

michaelwoerister Dec 11, 2018

Contributor

You can turn the assertion below into a debug_assertion.

pub fn new_no_params(kind: DepKind) -> DepNode {
assert!(!kind.has_params());
assert!(!kind.has_params_inlined());

This comment has been minimized.

@michaelwoerister

michaelwoerister Dec 11, 2018

Contributor

This can be turned into a debug_assertion too.

#[inline]
pub fn is_input(&self) -> bool {
#[inline(always)]
pub fn is_input_inlined(&self) -> bool {

This comment has been minimized.

@michaelwoerister

michaelwoerister Dec 11, 2018

Contributor

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.

This comment has been minimized.

@Zoxc

Zoxc Dec 11, 2018

Contributor

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.

This comment has been minimized.

@michaelwoerister

michaelwoerister Dec 12, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 11, 2018

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.

@Zoxc Zoxc force-pushed the Zoxc:query-perf1 branch from ac56bf3 to c4737a3 Dec 11, 2018

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 11, 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: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)

@Zoxc Zoxc force-pushed the Zoxc:query-perf1 branch from 1da5925 to 6360ae7 Dec 11, 2018

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Dec 14, 2018

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

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2018

📌 Commit 64d54e6 has been approved by michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2018

⌛️ Testing commit 64d54e6 with merge fd5abe9...

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

Auto merge of #56613 - Zoxc:query-perf1, r=michaelwoerister
Tweak query code for performance

Split from #56509

r? @michaelwoerister
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2018

💔 Test failed - status-appveyor

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 16, 2018

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

@Zoxc Zoxc force-pushed the Zoxc:query-perf1 branch from 64d54e6 to f0adf5a Dec 17, 2018

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 17, 2018

@bors r=michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2018

📌 Commit f0adf5a has been approved by michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 19, 2018

⌛️ Testing commit f0adf5a with merge 790f4c5...

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

Auto merge of #56613 - Zoxc:query-perf1, r=michaelwoerister
Tweak query code for performance

Split from #56509

r? @michaelwoerister
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 19, 2018

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

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

2 checks passed

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

@Zoxc Zoxc deleted the Zoxc:query-perf1 branch Dec 26, 2018

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