Remove attribute_cache from CrateMetadata

[whitfin]

This PR will fix #50508 by removing the attribute_cache from the CrateMetadata struct. Seeing as performance was referenced in the original issue, I also cleaned up a self.entry(node_id); call which might have occasionally happened redundantly.

r? @michaelwoerister

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[michaelwoerister]

Looks great! Thanks, @whitfin!

Let's see how this affects performance.
@bors try

[bors]

⌛ Trying commit 1307f7b with merge abed56d...

[bors]

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

[michaelwoerister]

@Mark-Simulacrum, could we get a perf run please?

[Mark-Simulacrum]

Perf queued.

[michaelwoerister]

Link: http://perf.rust-lang.org/compare.html?start=295d98069f34dce6f7a88422bfa7fbd0a1425aac&end=abed56dae29ead0e37943accbdd424e59dbf0310&stat=instructions%3Au

[michaelwoerister]

Huh, this seems to affect performance quite a bit. Good thing we checked!

[michaelwoerister]

Taking a closer look though, it doesn't seem so bad. The benchmarks affected most are small crates and they regress by 0.01 - 0.02 seconds, which is well below our measurement accuracy.
@nnethercote, how would you assess these results?

[nnethercote]

First of all, ignore the style-servo numbers. The "clean-incremental" job for that has enormous variance, unfortunately. For the other benchmarks, however, instruction counts have low variance and are reliable.

The regressions are mostly in smaller benchmarks, as you say. But the regressions are sizeable, and the affect both non-incremental and incremental builds. I've been landing a lot of speed-ups that are much smaller than this. So a regression of this size doesn't please me, alas :(

[michaelwoerister]

Let's look into how we can get the performance back without having the cache then. I think these are the main offenders:

rust/src/librustc_metadata/cstore.rs

Lines 186 to 227 in 8ff4b42

	pub fn needs_allocator(&self, sess: &Session) -> bool {
	let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
	attr::contains_name(&attrs, "needs_allocator")
	}
	pub fn has_global_allocator(&self) -> bool {
	self.root.has_global_allocator.clone()
	}
	pub fn has_default_lib_allocator(&self) -> bool {
	self.root.has_default_lib_allocator.clone()
	}
	pub fn is_panic_runtime(&self, sess: &Session) -> bool {
	let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
	attr::contains_name(&attrs, "panic_runtime")
	}
	pub fn needs_panic_runtime(&self, sess: &Session) -> bool {
	let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
	attr::contains_name(&attrs, "needs_panic_runtime")
	}
	pub fn is_compiler_builtins(&self, sess: &Session) -> bool {
	let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
	attr::contains_name(&attrs, "compiler_builtins")
	}
	pub fn is_sanitizer_runtime(&self, sess: &Session) -> bool {
	let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
	attr::contains_name(&attrs, "sanitizer_runtime")
	}
	pub fn is_profiler_runtime(&self, sess: &Session) -> bool {
	let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
	attr::contains_name(&attrs, "profiler_runtime")
	}
	pub fn is_no_builtins(&self, sess: &Session) -> bool {
	let attrs = self.get_item_attrs(CRATE_DEF_INDEX, sess);
	attr::contains_name(&attrs, "no_builtins")
	}

[michaelwoerister]

@whitfin, would you be willing to update your PR to optimize the above methods (the ones that call get_item_attrs())? This could be done the following way:

1. Decode the crate attributes already in CrateLoader::register_crate(). The crate_root contains them but you'd have to replicate a bit of the logic in CrateMetadata::entry() to get to them.
2. Add a bunch of boolean fields to CrateMetadata, one for each of the methods.
3. Initialize the fields in register_crate() the way it's done in the methods.
4. Change the methods to just read the boolean.

This way, we only decode and search the crate attributes once.

[whitfin]

@michaelwoerister I can definitely take a shot at doing so! I'll take a look today at some point & I'll get back to you if I need any further pointers.

[michaelwoerister]

Great, thanks!

[whitfin]

@michaelwoerister ok; should have something to push up shortly. One minor thing though - if I'm adding booleans to CrateMetadata, they're going to be pub because we create the struct directly... so should I remove the methods in favour of reading the fields directly?

With duplicating the entry logic inside register_crate, this is giving me pains:

rust/src/librustc_metadata/decoder.rs

Lines 164 to 166 in 8ff4b42

	pub fn cdata(&self) -> &'a CrateMetadata {
	self.cdata.expect("missing CrateMetadata in DecodeContext")
	}

Do you have any ideas how to work around it? My current approach is to set up the attributes inside the CrateMetadata itself. Feels messy, but I can't see any other reasonable approach.

Edit: Ok, I pushed up some working changes for this. It's not particularly clean because of having to decode using the CrateMetadata for context. If you have any ideas on how to avoid the derive_attributes function, I'll be glad to change it up - I couldn't find anything given how the decoding works.

[michaelwoerister]

One minor thing though - if I'm adding booleans to CrateMetadata, they're going to be pub because we create the struct directly... so should I remove the methods in favour of reading the fields directly?

Good idea! You can also make them bool instead of Option<bool>, unless I'm overlooking something.

Do you have any ideas how to work around it? My current approach is to set up the attributes inside the CrateMetadata itself. Feels messy, but I can't see any other reasonable approach.

Yeah, it feels a bit messy to call methods on a potentially only partially initialized object. If you are interested, I can show you how to "manually" decode the attributes before the CrateMetadata instance is created.

[michaelwoerister]

So, I haven't actually tried to compile this, but you should be able to do something like the following in CrateLoader::register_crate():

let crate_entry = crate_root
    .index
    .lookup(metadata.raw_bytes(), CRATE_DEF_INDEX)
    .unwrap()
    .decode(&metadata);

let crate_attrs = crate_entry
    .attributes
    .decode((&metadata, self.sess))
    .collect();

[whitfin]

@michaelwoerister that's very close to what I had been trying and results in the same issue with the decoder. I have pushed these changes up so that you can see the error I'm talking about and maybe we can figure out a way around it?

So you don't have to wait for the build to see the error, here it is. It occurs when compiling libstd.

thread 'main' panicked at 'missing CrateMetadata in DecodeContext', libcore/option.rs:914:5

It appears to be due to .decode((&metadata, self.sess)) wanting a CrateMetadata instead of metadata, but I'm not sure if there's some variation we can use to avoid this requirement. The error stems from here, which expects the option to contain a metadata value:

https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/decoder.rs#L341

As a total side note, I noticed that CrateMetadata::entry and by extension CrateMetadata::maybe_entry are called all over the place; would it make sense to cache the entries somehow in future? It looks like it calls LazySeq::lookup though, so perhaps that's not that much of a slowdown.

[rust-highfive]

The job x86_64-gnu-llvm-3.9 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:32:13]    Compiling rustc_asan v0.0.0 (file:///checkout/src/librustc_asan)
[00:32:13]    Compiling rustc_msan v0.0.0 (file:///checkout/src/librustc_msan)
[00:32:13]    Compiling rustc_tsan v0.0.0 (file:///checkout/src/librustc_tsan)
[00:32:15]    Compiling rustc_lsan v0.0.0 (file:///checkout/src/librustc_lsan)
[00:33:26] thread 'main' panicked at 'missing CrateMetadata in DecodeContext', libcore/option.rs:914:5
[00:33:26] 
[00:33:26] error: internal compiler error: unexpected panic
[00:33:26] 
[00:33:26] 
[00:33:26] note: the compiler unexpectedly panicked. this is a bug.
[00:33:26] 
[00:33:26] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:33:26] note: rustc 1.27.0-dev running on x86_64-unknown-linux-gnu
[00:33:26] 
[00:33:26] 
[00:33:26] note: compiler flags: -Z force-unstable-if-unmarked -C opt-level=3 -C prefer-dynamic -C panic=abort -C debug-assertions=no -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type lib
[00:33:26] 
[00:33:26] note: some of the compiler flags provided by cargo are hidden
[00:33:26] error: Could not compile `compiler_builtins`.
[00:33:26] 
[00:33:26] Caused by:
[00:33:26] Caused by:
[00:33:26]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name compiler_builtins rustc/compiler_builtins_shim/../../libcompiler_builtins/src/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=3 --cfg feature="c" --cfg feature="compiler-builtins" --cfg feature="default" --cfg feature="rustbuild" -C metadata=b4e196030fb3059f -C extra-filename=-b4e196030fb3059f --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/release/deps --extern core=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/libcore-07c899b6c3d44697.rlib -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/build/compiler_builtins-3c62409c53174beb/out --cfg use_c -l static=compiler-rt` (exit code: 101)
[00:33:26] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:33:26] expected success, got: exit code: 101
[00:33:26] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1091:9
[00:33:26] travis_fold:end:stage1-std

[00:33:26] travis_time:end:stage1-std:start=1525969611605273142,finish=1525969705086182894,duration=93480909752


[00:33:26] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:33:26] Build completed unsuccessfully in 0:27:10
[00:33:26] make: *** [all] Error 1
[00:33:26] Makefile:28: recipe for target 'all' failed

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

[michaelwoerister]

Ah, I see what's going on. Attributes contain spans and for decoding those we need to be able to import FileMaps. This, in turn, needs the CrateMetadata for caching stuff.

I think the best way to solve this is to already convert the crate attributes in question to booleans already during serialization. They would be fields of CrateRoot defined in schema.rs and set in encoder.rs. Then the crate attributes wouldn't have to be decoded at all.

[bors]

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

[whitfin]

@michaelwoerister good idea! I've implemented that, I think, and all the tests seem to pass. Want to take another look at this and see if the performance is any better?

I removed the unnecessary CrateMetadata functions from cstore.rs, but there's a provide! macro inside cstore_impl.rs which re-exposes something which looks like similar function names (last commit diff will show them). Should those also be removed? I can't quite tell if they're called from anywhere.

[michaelwoerister]

Thanks, @whitfin! Yes, this looks good. Let's see what it does to performance.

@bors try

[michaelwoerister]

Alright, now we are seeing actual performance improvements:
https://perf.rust-lang.org/compare.html?start=3ec2058bfee1e6c57d4c84d873737f84d4636bea&end=f3f0922614227432cadf4aba1d74b04b94b53a68&stat=instructions%3Au

@whitfin, if you fix the compile errors, this should be ready to go!

[whitfin]

@michaelwoerister is there something specific you're referring to? It looks like the last CI build ran successfully!

If I'm missing something, just drop me a note and I'll pick it up ASAP!

[michaelwoerister]

@whitfin, I think I just overlooked that last commit. Thanks for the PR, it's great!

@bors r+

[bors]

📌 Commit d95ba30 has been approved by michaelwoerister

[bors]

⌛ Testing commit d95ba30 with merge 9380fd2d098bdc5c7c32bd9f33703d56ee259aa9...

[bors]

💔 Test failed - status-travis

[kennytm]

@bors retry

30 minute no output while linking LLVM binaries again 🤔

[bors]

⌛ Testing commit d95ba30 with merge e0bee604b298fa412ce34401ba11dbf2a578cb3c...

[bors]

💔 Test failed - status-appveyor

[kennytm]

@bors retry

Failed to clone llvm or something

[bors]

⌛ Testing commit d95ba30 with merge c3733a7...

[bors]

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