Open Bug 1412077 Opened 7 years ago Updated 2 years ago

Make style crate build faster

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: gps, Unassigned)

References

(Depends on 2 open bugs)

Details

As data in bug 1411081 shows, the style crate takes a few minutes to compile with opt-level=2. It is also a long pole because it depends on bindgen, which also takes a while. We had to change the local build default to opt-level=1 in bug 1411081 largely because waiting on the style crate was taking too long. This negatively impacts build testing because opt-level=2 makes a significant difference to performance. I'm filing this bug to track improvements to making the style crate build faster. Ideally at opt-level=2. This not only tracks the style crate itself but its dependencies, notably bindgen. Manish: I know you have ideas. Could you please brain dump?
Flags: needinfo?(manishearth)
Making bindgen not build clap (which is completely unused for our purposes) and expensive to compile opt, would help.
(In reply to Nathan Froyd [:froydnj] from comment #1) > Making bindgen not build clap (which is completely unused for our purposes) > and expensive to compile opt, would help. Last time I checked this was blocked on https://github.com/rust-lang/cargo/issues/1982, but there's a workaround in that thread that sounds nice to me.
Short of splitting the style crate into smaller pieces, or the rust compiler finally getting faster, there is not much that can be done.
> This negatively impacts build testing because opt-level=2 makes a significant difference to performance. I mean, I think we might wish to do opt-level=2 for try release tests, but for local builds by default do opt-level=1. > Could you please brain dump? - If we switch to debug mode, bindgen is compiled in debug mode. Which is slow(er). I'm unsure if most of the time bindgen spends is in libclang or in the now-unoptimized rust code. We should measure this (emilio?) - Bindgen pulls in way too many headers. This means touching anything moderately important triggers rebuilds. This is a bad experience for folks working on the DOM or layout. We should reduce this. One way to do this is to extract the structs we care about into their own headers and make those headers depend on as few things as possible, and the original header file includes this one. This would be a lot of work. Also may not be worth it because C++ doesn't let you declare a struct and *declare* its methods out-of-line. We could use conditional compilation to make this work so that in bindgen mode we don't see any methods (so we can pull in fewer headers). Also a lot of work. There is probably low hanging fruit here though. - Bindgen generates way too much. I'm not sure if this impacts bindgen's runtime, but reducing this may improve style's compile time. Aggressively opaque-ifying types we never access would be nice. We generate way too many bindings for what we actually use. What we basically need is field access on style structs and most of the things they contain, but that doesn't need to pull in e.g. nsINode. - codegen-units isn't making the full use of parallelism. I don't yet know why. Alex mentioned that it can't magically parallelize and there must be that many divisions of the codegen work possible for this to work. I have to do some analysis with compiling style (also servo libscript) to see why it isn't parallelizing. - Compiling syntex takes a lot of time, especially for opt-level=2. We probably can get away with this being opt-level=1 or 0, *if* rust let us configure this.
Flags: needinfo?(manishearth)
(In reply to Manish Goregaokar [:manishearth] from comment #4) > - Compiling syntex takes a lot of time, especially for opt-level=2. We > probably can get away with this being opt-level=1 or 0, *if* rust let us > configure this. Upstream versions of bindgen use quote instead of syntex / aster / etc, and they improved a lot bindgen's CI time. A bindgen update may fix this kind of issues.
Depends on: 1412441
(In reply to Manish Goregaokar [:manishearth] from comment #4) > - Bindgen pulls in way too many headers. This means touching anything > moderately important triggers rebuilds. This is a bad experience for folks > working on the DOM or layout. We should reduce this. One way to do this is > to extract the structs we care about into their own headers and make those > headers depend on as few things as possible, and the original header file > includes this one. This would be a lot of work. Also may not be worth it > because C++ doesn't let you declare a struct and *declare* its methods > out-of-line. We could use conditional compilation to make this work so that > in bindgen mode we don't see any methods (so we can pull in fewer headers). > Also a lot of work. There is probably low hanging fruit here though. We can probably have the build script generate such stripped headers on the fly, and feed them into bindgen, rather than let bindgen read the headers itself. The build script is already reading the headers for dependency scanning anyway. Not exactly sure how it would work, though. > - Bindgen generates way too much. I'm not sure if this impacts bindgen's > runtime, but reducing this may improve style's compile time. Aggressively > opaque-ifying types we never access would be nice. We generate way too many > bindings for what we actually use. What we basically need is field access on > style structs and most of the things they contain, but that doesn't need to > pull in e.g. nsINode. I have an idea that we can probably add a new option to bindgen to aggressively opaque everything except those specified in the whitelist. Maybe we can call it "whitelist-only" or something like that. It should be doable although that may require us to extend the whitelist in the toml file for quite a bit. We can remove the opaque type list, though.
(In reply to Manish Goregaokar [:manishearth] from comment #4) > - Bindgen pulls in way too many headers. This means touching anything > moderately important triggers rebuilds. This is a bad experience for folks > working on the DOM or layout. We should reduce this. One way to do this is > to extract the structs we care about into their own headers and make those > headers depend on as few things as possible, and the original header file > includes this one. This would be a lot of work. Also may not be worth it > because C++ doesn't let you declare a struct and *declare* its methods > out-of-line. We could use conditional compilation to make this work so that > in bindgen mode we don't see any methods (so we can pull in fewer headers). > Also a lot of work. There is probably low hanging fruit here though. Another thought of this is that, if we can query clang for what types we depend on, and where are they defined, we should be able to shrink the list of headers in "cargo:rerun-if-changed". I guess that's something needs support from bindgen side.
>Another thought of this is that, if we can query clang for what types we depend on, and where are they defined, we should be able to shrink the list of headers in "cargo:rerun-if-changed". Weeellllll *technically* no, because any header can mess with #defines or whatever. Practically; yes (I don't know how hard the implementation is). This may mean some kinds of edits to headers lead to incorrect bindings on an incremental build. I think this is something that may be doable. > It should be doable although that may require us to extend the whitelist in the toml file for quite a bit. I like this idea. This might be overengineering but we could do something like `[whitelist] nsStyleFont = 3` or something, which would mean nsStyleFont is whitelisted and code will be generated for all types up to 3 levels reachable from it. (This may complicate the opaque-type-tracking stuff enough that it doesn't speed up bindgen though it may still speed up style crate perf)
(In reply to Manish Goregaokar [:manishearth] from comment #8) > >Another thought of this is that, if we can query clang for what types we depend on, and where are they defined, we should be able to shrink the list of headers in "cargo:rerun-if-changed". > > Weeellllll *technically* no, because any header can mess with #defines or > whatever. Practically; yes (I don't know how hard the implementation is). > This may mean some kinds of edits to headers lead to incorrect bindings on > an incremental build. I think this is something that may be doable. Yes, but in that case people would usually also mutate the corresponding files. Maybe clang can also tell us whether any typedef / #define involves in a type which we depends. I guess we don't really have many types involve #define. > > It should be doable although that may require us to extend the whitelist in the toml file for quite a bit. > > I like this idea. > > This might be overengineering but we could do something like `[whitelist] > nsStyleFont = 3` or something, which would mean nsStyleFont is whitelisted > and code will be generated for all types up to 3 levels reachable from it. > > (This may complicate the opaque-type-tracking stuff enough that it doesn't > speed up bindgen though it may still speed up style crate perf) I don't like this... I'd rather we keep things simple and just add everything we need to the whitelist. I don't really believe we have that many things to deal with.
Depends on: 1412486
Depends on: 1413731
FWIW, the rustc compile perf tracker does include a (fixed) revision of the style crate: http://perf.rust-lang.org/ .
(In reply to Manish Goregaokar [:manishearth] from comment #4) > - Compiling syntex takes a lot of time, especially for opt-level=2. We > probably can get away with this being opt-level=1 or 0, *if* rust let us > configure this. Is syntex only used as a transitive dependency of bindgen? As it turns out, fitzgen removed the syntex dependency recently: https://github.com/rust-lang-nursery/rust-bindgen/issues/925 That fix is in bindgen 0.31.2, which literally just got merged into m-c today: https://github.com/servo/servo/pull/19072 https://hg.mozilla.org/mozilla-central/rev/0c00ba0de42e
> - Bindgen pulls in way too many headers. This means touching anything moderately important triggers rebuilds. This is a bad experience for folks working on the DOM or layout. We should reduce this. One way to do this is to extract the structs we care about into their own headers and make those headers depend on as few things as possible, and the original header file includes this one. This would be a lot of work. Also may not be worth it because C++ doesn't let you declare a struct and *declare* its methods out-of-line. We could use conditional compilation to make this work so that in bindgen mode we don't see any methods (so we can pull in fewer headers). Also a lot of work. There is probably low hanging fruit here though. Would it be possible to split sets of headers apart into sub-crates that the style crate could depend on? That way ideally you'd only be rebuilding the specific bits that depend on those headers. Obviously you'd still wind up rebuilding the style crate itself, but if the bindgen-generated bindings were in separate crates you shouldn't have as much to compile. (Hopefully incremental compilation will help here as well.)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11) > Is syntex only used as a transitive dependency of bindgen? As it turns out, > fitzgen removed the syntex dependency recently: > https://github.com/rust-lang-nursery/rust-bindgen/issues/925 > > That fix is in bindgen 0.31.2, which literally just got merged into m-c > today: > https://github.com/servo/servo/pull/19072 > https://hg.mozilla.org/mozilla-central/rev/0c00ba0de42e Indeed, I think building gecko doesn’t involve building syntex anymore: https://hg.mozilla.org/mozilla-central/rev/d480e8818314#l213.143 third_party/rust/syntex/ is still there because js/rust/ still uses bindgen 0.30. Maybe we can update that as well?
(In reply to Simon Sapin (:SimonSapin) from comment #13) > Indeed, I think building gecko doesn’t involve building syntex anymore: > https://hg.mozilla.org/mozilla-central/rev/d480e8818314#l213.143 > > third_party/rust/syntex/ is still there because js/rust/ still uses bindgen > 0.30. Maybe we can update that as well? Probably fitzgen is the best person to ask to for that :)
Flags: needinfo?(nfitzgerald)
Only reason it hasn't been updated is because it hasn't been updated ;) I can look into it soon.
Flags: needinfo?(nfitzgerald)
We have an older copy of the pre-processed Stylo structs header checked into the `bindgen` repo that we use as a smoke test, and occasionally time how long it takes to generate bindings for. ### Release Mode Building `bindgen` in release mode: Finished release [optimized] target(s) in 174.59 secs Running the Stylo sanity test on master yields this result: time: 492.128 ms. parse time: 0.000 ms. process_replacements time: 21.272 ms. deanonymize_fields time: 553.664 ms. compute_whitelisted_and_codegen_items time: 32.282 ms. compute_has_vtable time: 163.831 ms. compute_sizedness time: 31.419 ms. compute_has_destructor time: 153.185 ms. find_used_template_parameters time: 126.824 ms. compute_cannot_derive_debug time: 0.000 ms. compute_cannot_derive_default time: 191.432 ms. compute_cannot_derive_copy time: 29.543 ms. compute_has_type_param_in_array time: 0.000 ms. compute_has_float time: 0.000 ms. compute_cannot_derive_hash time: 0.000 ms. compute_cannot_derive_partialord_partialeq_or_eq time: 264.948 ms. codegen Generated Stylo bindings in: Duration { secs: 4, nanos: 147347796 } Total release time: ~179 seconds. ### Dev Mode Building `bindgen` in dev mode: Finished dev [unoptimized + debuginfo] target(s) in 27.78 secs Generating Stylo bindings: time: 5025.307 ms. parse time: 0.001 ms. process_replacements time: 441.843 ms. deanonymize_fields time: 19990.504 ms. compute_whitelisted_and_codegen_items time: 1005.502 ms. compute_has_vtable time: 5715.124 ms. compute_sizedness time: 1046.051 ms. compute_has_destructor time: 4887.797 ms. find_used_template_parameters time: 4558.315 ms. compute_cannot_derive_debug time: 0.000 ms. compute_cannot_derive_default time: 6772.230 ms. compute_cannot_derive_copy time: 1010.969 ms. compute_has_type_param_in_array time: 0.000 ms. compute_has_float time: 0.000 ms. compute_cannot_derive_hash time: 0.000 ms. compute_cannot_derive_partialord_partialeq_or_eq time: 9615.932 ms. codegen Generated Stylo bindings in: Duration { secs: 64, nanos: 279007654 } Total dev time: ~92 seconds. -------------------------------------------------------------------------------- So build time is a significant amount of `bindgen` related time. But it only happens once, even after headers are modified. If we always build bindings with release mode, we pay the build time cost once up front, and then its only ~4 seconds to regenerate bindings. In addition to building without `clap`, Stylo can probably build without logging and the `env_logger` dependency, which is a significant amount of `bindgen` build time. Or at least make it optional (default off) behind some config option. The next thing to look into is how long it takes to build the `style` crate independent of `bindgen` and bindings regeneration. If it takes two minutes to rebuild the `style` crate even after (release mode) `bindgen` has already been built, then it seems like the majority of time is being spent in there.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #15) > Only reason it hasn't been updated is because it hasn't been updated ;) > > I can look into it soon. Updating js/rust's bindgen dep in https://bugzilla.mozilla.org/show_bug.cgi?id=1414030
FWIW, using `CARGO_INCREMENTAL=1` with nightly Rust drops a fresh release build's time down to ~45 seconds. https://internals.rust-lang.org/t/help-us-benchmark-incremental-compilation/6153/29
Product: Core → Firefox Build System
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.