incr.comp.: We don't achieve 100% CGU re-use when recompiling Stylo

[michaelwoerister]

The following CGU are re-compiled even though there's been no change to the source code (just touch components/style/lib.rs).

alloc-vec.volatile
style-properties-animated_properties
style-gecko_bindings-structs-root
style-properties-declaration_block
smallvec.volatile
style-gecko-wrapper
hashglobe-table.volatile
servo_arc.volatile
style-gecko_bindings-structs-root-mozilla.volatile
core-iter.volatile
style-values-computed.volatile
style-properties-longhands-grid_template_columns
style-properties-longhands-grid_template_rows
style-properties-longhands-border_image_source
style-properties-longhands-clip_path
style-properties-longhands-shape_outside
style-values-animated.volatile

Version of Stylo used: https://github.com/rust-lang-nursery/rustc-perf/tree/6b3404678be0c1e3c013ca9234083984ae13b101/collector/benchmarks/style-2f3bc0de49

Steps to reproduce:

cd collector/benchmarks/style-2f3bc0de49
cargo clean
cargo rustc --release --features gecko --manifest-path components/style/Cargo.toml -- --cap-lints=warn -Zincremental=/tmp/somethingsomething -Ztime-passes -Zhuman-readable-cgu-names
touch components/style/lib.rs
cargo rustc --release --features gecko --manifest-path components/style/Cargo.toml -- --cap-lints=warn -Zincremental=/tmp/somethingsomething -Ztime-passes -Zhuman-readable-cgu-names

rustc 1.25.0-nightly (b8398d947 2018-02-11)
binary: rustc
commit-hash: b8398d947d160ad4f26cc22da66e5fbc7030817b
commit-date: 2018-02-11
host: x86_64-unknown-linux-gnu
release: 1.25.0-nightly
LLVM version: 6.0

Need to investigate further why these CGUs are being recompiled? @SimonSapin, @emilio, is there maybe some kind of build script or proc-macro that modifies the source code during a rebuild?

[SimonSapin]

Adding -v to the commands I see that the build script is run the first time but not the second. As expected, since cargo:rerun-if-changed=… is used.

There are multiple (derive) proc macros in use, but they should be deterministic as far as I know.

[michaelwoerister]

Thanks for the information, @SimonSapin!

[michaelwoerister]

One way to find out if this is due to a StableHash instability is to compile the crate a few times in a row with the -Zincremental-verify-ich flag enabled. That should point to any instabilities pretty quickly.

[goffrie]

I suspect it is because of hashset ordering in style_derive; see in cg.rs

impl<'input, 'path> WhereClause<'input, 'path> {
    pub fn add_trait_bound(&mut self, ty: &Ty) {
        let trait_path = self.trait_path;
        let params = self.params;
        let mut found = self.trait_output.map(|_| HashSet::new());

.. later:

        if let Some(found) = found {
            for ident in found {
                let ty = Ty::Path(None, ident.into());

At least, this is responsible for causing differences in the --pretty=expanded output.

[michaelwoerister]

@goffrie Wow, great find! That surely would explain instabilities.

@SimonSapin What would you suggest that we do with the benchmark? Do newer versions of style_derive have this problem too? We could also just patch the benchmark. Anything that will make the generated AST deterministic would help.

[SimonSapin]

https://github.com/servo/servo/blob/master/components/style_derive/cg.rs now doesn’t use use HashSet at all, but that’s part of a large-ish change that’s not easily backported.

We can update the entire benchmark to current Servo, but maybe it’s undesirable to change what’s being measured too much?

Another option is patching the benchmark to replace HashSet with BTreeSet or using a non-random hasher, to get deterministic iteration order.

[michaelwoerister]

I'd say let's patch the benchmark. One thing to look out for is if the values of pointers are being hashed (or compared in the case of BTreeSet). The version of syn being used seems to be an old one so I didn't see immediately if that could be the case.

[andjo403]

looks like it is possible to close this

[michaelwoerister]

We should verify that we get 100% re-use first.

[michaelwoerister]

I'm wondering why these CGUs are not named? Are you sure that this is an incr. build? You could try running the test again with -Zhuman-readable-cgu-names. That might give you some more insight.

[wesleywiser]

Doh! I forgot to pass -Cincremental. If I do that, I can confirm we get 100% CGU re-use. 🎉