Add async resolver and JS transformer functions using rayon #9147
Conversation
There was a problem hiding this comment.
Seems to make sense to me - again, I'm not a Rust expert so not sure if there's any gotchas with the way rayon is being used (like does there need to be any "config" done for rayon, or do it's defaults just make maximal use of available system resources?
Are we likely to run into any issues in CI where the OS can report different values for max CPUs compared to what our Docker agents are allocated, for example?
Can we make the async stuff opt-in via an environment variable initially so we can validate in CI safely?
| let (deferred, promise) = env.create_deferred()?; | ||
| let resolver = &self.resolver; | ||
|
|
||
| if matches!(resolver.cache.fs, EitherFs::A(..)) || resolver.module_dir_resolver.is_some() { |
There was a problem hiding this comment.
I tried to understand how the EitherFS thing works, but I don't think I fully have my head around it.
It's an enum with two generic params, and the value of the enum is either of the generics.. and the generic type of the Fs used for the resolver is EitherFS<JsFileSystem, OsFileSystem> - does that just mean that the fs has to be one of those two? I'm sure there's a good reason (that I'd love to know more about), but why can't the type just be JsFileSystem | OsFilesSystem?
It's a bit off topic, but I'd love to learn more.
There was a problem hiding this comment.
In rust there are no union types like that. EitherFs is a generic enum that chooses between (any) two options and proxies all of the FileSystem methods to the underlying instance.
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
devongovett
force-pushed
the
async-rust
branch
from
a00af09
to
5342521
Compare
yep, it defaults to the number of physical CPUs on the system. |
My only concern about this is in our Kube/Docker based CI environment, in Node for example, Is there a standard way to override this for rayon (with say an environment), or can we make it use |
|
Yes, there is a |
Depends on #9146.
With all of our rust modules in a single binary, we can now share a single Rayon thread pool between modules. This adds async versions of the resolver and JS transformer so that IO and processing can happen outside of the JS thread(s). This results in a ~20% performance improvement on esbuild's benchmark.
For the resolver, this is only supported when using a NodeFS, and when PnP is not enabled, because our implementation of calling JS functions from Rust is not currently thread safe. It could potentially be made so but this was a bunch of extra work for an edge case, so I think for now falling back to the sync version and accepting a small perf hit is fine.