-
Notifications
You must be signed in to change notification settings - Fork 330
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
Massive regression in compile times #939
Comments
I've seen multiple responses that I think there's some widespread misunderstanding that the dependency count or something not already being in the dependency tree is what determines compile times, but the scheduling of when crates can start compiling is actually the most important and most overlooked problem. |
Thanks for the extra context. Looking at the build of I've filed unicode-org/icu4x#5121 and unicode-org/icu4x#5120 . (The latter thing should get LTOed out, so it's not an issue of ICU4X resulting in extra stuff in an optimized binary; you don't pay for what you don't use in binary size, but you do in compile time.) |
This was a misreading on my part, and it seems that |
TL;DR: I have implemented a prototype (READMEs and metadata unpolished, repos not in the right org, crates not on crates.io) of a back end adapter for I’d appreciate it if the folks for whom compile times or MSRV increase arising from ICU4X was a problem could comment if the approach described below addresses their concerns. Cargo does not have the Right ThingUnfortunately, Cargo lacks a mechanism for the top level of the compilation to choose a back end for a crate deep in the dependency tree. There is a pre-RFC by @epage, but it can’t be used today. Unsuitability of Cargo featuresUnsuited, because:
Which back end should be the defaultAs for which one of ICU4X or unicode-rs should be the default back end, I strongly believe that the default should be the one that has the better attributes for the compilation product rather than the one that has the better attributes for the compilation process. ICU4X results in a smaller binary (assuming that there isn’t something else in the dependency tree pulling in the unicode-rs crates anyway) and in faster run-time performance when the input contain Unicode/Punycode labels. ICU4X also has less opportunity for incorrect results due to different unicode-rs pieces not getting updated to a new Unicode version in sync (see below). The run-time performance is equal between the two options when the input contains only ASCII labels consisting only of lower-case letters. In other ASCII-only cases, unicode-rs is actually a bit (single-digit nanoseconds per domain) faster right now due to unicode-org/icu4x#5187 and the way the different tiers of ASCII optimizations are organized in MechanismIn the absence of global features for Cargo and regular Cargo features being unsuitable, I have implemented an approach where top-level dependents who wish to optimize for MSRV, compilation time, or for data overlap with something else in their dependency tree using CorrectnessCompared to those who wish to avoid ICU4X staying on The This can be addressed by not publishing a new If The best solution would be to get Run-time performanceurl crate benches (which don’t benefit properly from M3 Pro unicode-rstest hyphen ... bench: 194 ns/iter (+/- 4) = 159 MB/s M3 Pro ICU4Xtest hyphen ... bench: 195 ns/iter (+/- 1) = 158 MB/s Binary size[profile.release]
opt-level = "z"
lto = true
strip = true unicode-rsApple Silicon Mac wasm32-wasi wasm-opt -Oz -o opt.wasm --enable-bulk-memory target/wasm32-wasi/release/urldemo.wasm brotli -9 -o opt.wasm.br opt.wasm ICU4XApple Silicon Mac wasm32-wasi wasm-opt -Oz -o opt.wasm --enable-bulk-memory target/wasm32-wasi/release/urldemo.wasm brotli -9 -o opt.wasm.br opt.wasm Compile timeThese are the times for compiling M3 Pro, dev
M3 Pro, release
MSRVWith ICU4X, 1.67. With unicode-rs, 1.57 (for panic in const context). Testing itClone the repos https://github.com/hsivonen/rust-url , https://github.com/hsivonen/idna_adapter , and https://github.com/hsivonen/idna_mapping next to each other. Switch If you also have [patch.crates-io]
url = { path = "../rust-url/url/" }
form_urlencoded = { path = "../rust-url/form_urlencoded/" } (Trying to patch |
Honestly I'd like a way to opt out of IDN handling entirely - in most applications if a domain has unicode in it I can handle it on the frontend and get the punycode version. |
In fact, I'd strongly prefer the option to opt my entire application out of IDN handling entirely purely for homograph attack reasons. Not only does it remove code (yay!) but also makes my application (which has no use for IDNs, and in fact in may cases is requesting from URLs where other higher-level specifications mandate the lack of IDN usage) more secure! |
@TheBlueMatt , noted, but turning off IDN handling entirely is out of scope for the event of folks being unhappy about the implications of using ICU4X. |
Yea, that's totally fair, just highlighting it as a thing to consider as there may be nontrivial overlap between the groups. |
I had also thought it might be appropriate to take what we currently have with the ICU4X dependency (including the opportunity for incremental improvements) and make it the default in rust-url 3.0, keeping rust-url 2.0 on the current implementation. Then people need to effectively "opt in" to the worse compile time but better runtime characteristics. |
As a technical matter, I observe that your earlier comment said "in most applications if a domain has unicode in it I can handle it on the frontend and get the punycode version." Allowing Punycode through would require something more specific than taking the current
This can become a problem if parts of the ecosystem stay on If we put the |
Not necessarily, the data could be managed by this crate, which already maintains a copy of some Unicode data. I think the
cc @valenting One thing missing from this plan is whether or not the Servo team is or ought to commit to a maintenance plan for idna_adapter 1.1.x. I think this might need to be explicitly discussed by the Servo TSC: if our documented way of telling people to obtain the old behavior is "pin the older version", we need to decide under what situations we keep that old version maintained (otherwise in a year or so we'll be in a situation where the solution which we tell people about doesn't work). I'm not actually sure we need separate crates if this is going to be the default anyway |
My thoughts and assumptions related to my suggestion are:
|
It seems bad to take on even more non-ICU4X maintenance effort in order to proceed with ICU4X use. One way to deal with
The expected maintenance is updating I note that when I asked for volunteers for whom old MSRV or
What's "this" referring to in that sentence?
Is "default" in that sentence implying still offering a |
It's a single unicode property, the crate already maintains unicode tables, this is not that big a deal to include another table generated by the script.
That's not a maintenance plan: by maintenance plan I mean that for the benefit of the people who are choosing to stick to older "There are no volunteers to do this work" is irrelevant: This crate is formally maintained by the Servo TSC (in practice, largely by @valenting with some help from me). When things break, the buck stops there. If people are getting annoyed at the crate for breaking something, it's Valentin, me, and the TSC who have to deal with it. If we pick the route you're proposing, we need to make sure that we're ready to maintain patches of the older stuff to the extent needed by the community, otherwise it's not a solution, it's something that will work for a couple months and then get people angry at us again for putting them in a catch-22 of needing to update their project but being unable to because of other concerns. I would like us to pick a solution here that does not make the Rust community annoyed at us again for pulling the rug out underneath them. This may mean more maintenance work. If we can't commit to more maintenance work, then perhaps we shouldn't be doing this.
IDNA-via-icu4x. I'm not sure we actually need a new pluggable adapter crate if we're not really supporting pluggability and instead are just switching defaults. |
I was suggesting exclusively |
The purpose of the That is, what I'm suggesting here is about avoiding a situation that would create demand for bug fixes for old versions. (On a different topic:)
This is probably not accurate and it probably would be possible to accept Punycode labels by stubbing out |
Okay, but I'll admit all these version numbers are confusing, I think you have a specific understanding of what each version number implies but it's not clear to me. It would be helpful if you had something listing the status quo and the plan in terms of version numbers for all crates involved (maybe that's already in this thread but I can't find it) |
Current situation
I'm suggesting
This should lead to both Once ICU4X 2.0 is out, I expect folks who want ICU4X 1.5 to do MaintenanceI'm expecting the maintained crates to be the highest-version For For OptionalAlso publish |
Data point about a project downgrading from |
Per off-issue discussion with maintainers, I plan to pursue my suggestion above, but I have higher-priority items (outside this crate) to deal with first. |
This is indeed feasible. |
The implementation of the above plan is now #965 |
With |
Hm, I have to say that's very unfortunate as it still breaks our MSRV builds on multiple projects that are downstream ldependencies of |
I think the resolution to this issue is still very unsatisfying.
Update: Seems like there's some good discussion at: unicode-org/icu4x#5124 |
Smaller crates reduce compile time because in cargo, multiple crate can be compiled in parallel. Even if they have a dependency link, once rmetadata is generated they can be compiled in parallel. |
The crate graph will improve, but only a little bit and not enough to make
I note that as the reporter of this issue, you have had the opportunity to have visibility into the plan and the progress on its implementation since July 8, but you didn't speak up before all the work was done and, on November 4, released. As for the right default:
|
There is a concept of a plan to remove the syn and proc-macro2 deps from this corner of ICU4X, but it needs more design work. This statement is accurate for what we have landed so far. |
After #923 the dependency tree massively increased. The total amount of crates during compilation went from 8 to 36. In particular the "critical path" to compilation now consists of 17 crates as opposed to 5 crates. This means that the url crate is now the sole reason why basically all projects depending on the url crate (indirectly) should now take a whole 20 seconds or so longer to compile than before.
You can compile an entire GUI framework in the time the URL crate compiles now.
Before:
After:
The text was updated successfully, but these errors were encountered: