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

Always calculate glob map but only for glob uses #57392

Merged
merged 3 commits into from Jan 17, 2019

Conversation

Projects
None yet
9 participants
@Xanewok
Copy link
Member

Xanewok commented Jan 6, 2019

Previously calculating glob map was opt-in, however it did record node id -> ident use for every use directive. This aims to see if we can unconditionally calculate the glob map and not regress performance.

Main motivation is to get rid of some of the moving pieces and simplify the compilation interface - this would allow us to entirely remove CrateAnalysis. Later, we could easily expose a relevant query, similar to the likes of maybe_unused_trait_import (so using precomputed data from the resolver, but which could be rewritten to be on-demand).

r? @nikomatsakis

Local perf run showed mostly noise (except ctfe-stress-*) but I'd appreciate if we could do a perf run run here and double-check that this won't regress performance.

fn add_to_glob_map(&mut self, id: NodeId, ident: Ident) {
if self.make_glob_map {
self.glob_map.entry(id).or_default().insert(ident.name);
#[inline]

This comment has been minimized.

@Xanewok

Xanewok Jan 6, 2019

Member

Not sure if premature? Was afraid this might evade inlining and it's small enough but we can get rid of it if needed.

This comment has been minimized.

@nikomatsakis

nikomatsakis Jan 7, 2019

Contributor

Seems fine

@Xanewok Xanewok referenced this pull request Jan 7, 2019

Merged

Add Xanewok to try users #125

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Jan 7, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

⌛️ Trying commit 0a82177 with merge 4ff2a6e...

bors added a commit that referenced this pull request Jan 7, 2019

Auto merge of #57392 - Xanewok:always-calc-glob-map, r=<try>
Always calculate glob map but only for glob uses

Previously calculating glob map was *opt-in*, however it did record node id -> ident use for every use directive. This aims to see if we can unconditionally calculate the glob map and not regress performance.

Main motivation is to get rid of some of the moving pieces and simplify the compilation interface - this would allow us to entirely remove `CrateAnalysis`. Later, we could easily expose a relevant query, similar to the likes of `maybe_unused_trait_import` (so using precomputed data from the resolver, but which could be rewritten to be on-demand).

r? @nikomatsakis

Local perf run showed mostly noise (except `ctfe-stress-*`) but I'd appreciate if we could do a perf run run here and double-check that this won't regress performance.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 7, 2019

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:086168f9:start=1546818570894539900,finish=1546818643800232979,duration=72905693079
$ 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
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:12:19] 
[01:12:19] running 118 tests
[01:12:46] .iiiii...i.....i..i...i..i.i..i.ii..i.....i..i....i..........iiii..........i...ii...i.......ii.i.i.i 100/118
[01:12:51] ......iii.i.....ii
[01:12:51] 
[01:12:51]  finished in 31.394
[01:12:51] travis_fold:end:test_debuginfo

---
travis_time:end:1c17a7e0:start=1546824872330960908,finish=1546824872335806369,duration=4845461
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:03358ddf
$ ln -s . checkout && for CORE 

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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

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

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Jan 7, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 7, 2019

Success: Queued 4ff2a6e with parent b92552d, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 7, 2019

Finished benchmarking try commit 4ff2a6e

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Jan 7, 2019

Perf seems unaffected, with interesting greens in ctfe-stress-* (due to inlined directive.is_glob() check? No idea 🤷‍♂️ ).

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 7, 2019

Perf seems unaffected, with interesting greens in ctfe-stress-* (due to inlined directive.is_glob() check? No idea

Probably noise.
add_to_glob_map is a drop in the ocean compared to what the rest of resolve does, I wouldn't expect the change to have any noticeable effect.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 7, 2019

r? @petrochenkov

Seems good to me, but I would prefer for @petrochenkov to sign off too, though based on their comments in thread I am guessing they are fine with it.

A question though: can the glob map be computed from other bits of data (i.e., lazilly, after the fact)? It seems like the idea scenario longer term would be that there is a "core query" that computes the main name resolution results, and then the glob map is computed only when needed based on those other results.

Note that there is a mild memory usage regression in many cases, though in some cases we see benefits too (presumably because we are computing less data? or noise? not sure).

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 8, 2019

can the glob map be computed from other bits of data (i.e., lazilly, after the fact)?

No, the only similar thing is export_map, but it doesn't keep the mapping "NodeId -> Names" for globs.
This kind of data, in general, doesn't leave resolve because it is not used by later stages, glob map was added solely for save-analysis.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 8, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 8, 2019

📌 Commit 71c6402 has been approved by petrochenkov

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Jan 8, 2019

can the glob map be computed from other bits of data (i.e., lazilly, after the fact)?

I wanted to take this approach but I wasn't sure how to tackle this - it seems that the data from which we can derive this isn't useful outside of the save-analysis, just like @petrochenkov is saying. Maybe another visiting pass would work? However, at a first glance it seems to be inherently tied to the core resolution. Maybe we could see if we can make it on-demand later, as we go?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

⌛️ Testing commit 71c6402 with merge e68c08e...

bors added a commit that referenced this pull request Jan 10, 2019

Auto merge of #57392 - Xanewok:always-calc-glob-map, r=petrochenkov
Always calculate glob map but only for glob uses

Previously calculating glob map was *opt-in*, however it did record node id -> ident use for every use directive. This aims to see if we can unconditionally calculate the glob map and not regress performance.

Main motivation is to get rid of some of the moving pieces and simplify the compilation interface - this would allow us to entirely remove `CrateAnalysis`. Later, we could easily expose a relevant query, similar to the likes of `maybe_unused_trait_import` (so using precomputed data from the resolver, but which could be rewritten to be on-demand).

r? @nikomatsakis

Local perf run showed mostly noise (except `ctfe-stress-*`) but I'd appreciate if we could do a perf run run here and double-check that this won't regress performance.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

💔 Test failed - status-appveyor

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 10, 2019

@bors retry
AppVeyor... what's wrong with you today?

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 13, 2019

@bors r=petrochenkov

It's not yet deployed.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 13, 2019

📌 Commit b1b64bd has been approved by petrochenkov

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Jan 13, 2019

@pietroalbini ah, I thought the r+ rights allowed me to retry in #51487 (comment) (but it seems it was the try permissions 🤔)

Thanks for the heads up!

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 13, 2019

Yeah, retry is granted by the try permission (along with try and p=).

@Xanewok Xanewok referenced this pull request Jan 13, 2019

Merged

Querify `entry_fn` #57573

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57392 - Xanewok:always-calc-glob-map, r=pet…
…rochenkov

Always calculate glob map but only for glob uses

Previously calculating glob map was *opt-in*, however it did record node id -> ident use for every use directive. This aims to see if we can unconditionally calculate the glob map and not regress performance.

Main motivation is to get rid of some of the moving pieces and simplify the compilation interface - this would allow us to entirely remove `CrateAnalysis`. Later, we could easily expose a relevant query, similar to the likes of `maybe_unused_trait_import` (so using precomputed data from the resolver, but which could be rewritten to be on-demand).

r? @nikomatsakis

Local perf run showed mostly noise (except `ctfe-stress-*`) but I'd appreciate if we could do a perf run run here and double-check that this won't regress performance.

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57392 - Xanewok:always-calc-glob-map, r=pet…
…rochenkov

Always calculate glob map but only for glob uses

Previously calculating glob map was *opt-in*, however it did record node id -> ident use for every use directive. This aims to see if we can unconditionally calculate the glob map and not regress performance.

Main motivation is to get rid of some of the moving pieces and simplify the compilation interface - this would allow us to entirely remove `CrateAnalysis`. Later, we could easily expose a relevant query, similar to the likes of `maybe_unused_trait_import` (so using precomputed data from the resolver, but which could be rewritten to be on-demand).

r? @nikomatsakis

Local perf run showed mostly noise (except `ctfe-stress-*`) but I'd appreciate if we could do a perf run run here and double-check that this won't regress performance.
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 15, 2019

@bors p=5

Rollup fairness due to submodule changes.

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Jan 15, 2019

Looking at https://buildbot2.rust-lang.org/homu/queue/rust I’m afraid the priority didn’t get assigned.

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Jan 16, 2019

Let’s try above again.

@bors p=5

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2019

⌛️ Testing commit b1b64bd with merge 59c1cac...

bors added a commit that referenced this pull request Jan 16, 2019

Auto merge of #57392 - Xanewok:always-calc-glob-map, r=petrochenkov
Always calculate glob map but only for glob uses

Previously calculating glob map was *opt-in*, however it did record node id -> ident use for every use directive. This aims to see if we can unconditionally calculate the glob map and not regress performance.

Main motivation is to get rid of some of the moving pieces and simplify the compilation interface - this would allow us to entirely remove `CrateAnalysis`. Later, we could easily expose a relevant query, similar to the likes of `maybe_unused_trait_import` (so using precomputed data from the resolver, but which could be rewritten to be on-demand).

r? @nikomatsakis

Local perf run showed mostly noise (except `ctfe-stress-*`) but I'd appreciate if we could do a perf run run here and double-check that this won't regress performance.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 16, 2019

The job arm-android 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.
[01:48:47] test string::test_str_truncate ... ok
[01:48:47] test string::test_reserve_exact ... ok
[01:48:47] test string::test_str_truncate_split_codepoint ... ok
[01:48:47] test string::test_str_truncate_invalid_len ... ok
[01:48:47] died due to signal 11
[01:48:47] 
[01:48:47] 
[01:48:47] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "arm-linux-androideabi" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--"
[01:48:47] expected success, got: exit code: 3
---
travis_time:end:0e1b8b14:start=1547673899900240099,finish=1547673899920749297,duration=20509198
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1c36552c
$ 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:324fe60d
travis_time:start:324fe60d
$ 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:18f5d224
$ 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)

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Jan 16, 2019

Segfault in collection tests 🤔 This should be unrelated.

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2019

⌛️ Testing commit b1b64bd with merge c40b977...

bors added a commit that referenced this pull request Jan 16, 2019

Auto merge of #57392 - Xanewok:always-calc-glob-map, r=petrochenkov
Always calculate glob map but only for glob uses

Previously calculating glob map was *opt-in*, however it did record node id -> ident use for every use directive. This aims to see if we can unconditionally calculate the glob map and not regress performance.

Main motivation is to get rid of some of the moving pieces and simplify the compilation interface - this would allow us to entirely remove `CrateAnalysis`. Later, we could easily expose a relevant query, similar to the likes of `maybe_unused_trait_import` (so using precomputed data from the resolver, but which could be rewritten to be on-demand).

r? @nikomatsakis

Local perf run showed mostly noise (except `ctfe-stress-*`) but I'd appreciate if we could do a perf run run here and double-check that this won't regress performance.

@bors bors referenced this pull request Jan 16, 2019

Closed

Prepare beta 1.33.0 #57670

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 17, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing c40b977 to master...

@bors bors merged commit b1b64bd into rust-lang:master Jan 17, 2019

1 check passed

homu Test successful
Details

Centril added a commit to Centril/rust that referenced this pull request Jan 19, 2019

Rollup merge of rust-lang#57476 - Xanewok:bye-crate-analysis, r=Zoxc
Move glob map use to query and get rid of CrateAnalysis

~Also includes commits from ~rust-lang#57392 and ~rust-lang#57436

With glob map calculated unconditionally in rust-lang#57392, this PR moves the calculated glob map to `GlobalCtxt` and exposes a relevant query (as we do with other queries which copy precomputed data over from the `Resolver`).

This allows us to get rid of the `CrateAnalysis` struct in an attempt to simplify the compiler interface.
cc @Zoxc

r? @nikomatsakis @Zoxc @petrochenkov

Centril added a commit to Centril/rust that referenced this pull request Jan 19, 2019

Rollup merge of rust-lang#57573 - Xanewok:querify-entry-fn, r=Zoxc
Querify `entry_fn`

Analogous to rust-lang#57570 but this will also require few fixups in Miri so I decided to separate that (and it seems [CI doesn't let us break tools anymore](rust-lang#57392 (comment))? Or was that because it was a rollup PR?)

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