Improvements for transforms running before dependency extraction #32
Conversation
- passes through transform options when getting dependencies - passes back all properties provided by a custom transform - `ModuleCache` can create `Polyfill` objects that are completly functional - `ResolutionRequest` parallelizes dependency extraction instead of running it serially
davidaurelio
force-pushed
the
additional-transform-support
branch
from
e5632b3
to
980f968
Compare
| }); | ||
|
|
||
| return p; | ||
| return Promise.all([modDep, recursive ? collect(modDep) : []]); |
There was a problem hiding this comment.
This effectively parallelizes dependency extraction.
There was a problem hiding this comment.
This is nice, I'm a bit concerned about I/O thrashing though because we'll fire off file-reads for every single JS file in the tree at the same time. In Jest on www, this completely stalled the process for about an hour (yes!). To mitigate this, I made a queue, like this: https://github.com/facebook/jest/blob/master/src/TestRunner.js#L203-L234
Another solution to limit concurrent jobs would be throat by our own @ForbesLindesay: https://www.npmjs.com/package/throat
Do you think this will also be an issue here?
There was a problem hiding this comment.
Can I try it out on www easily? throat looks good, will add it. It probably wasn’t a problem for packager, because most time is spent in the transform, effectively limiting i/o. What’s a good number? 64? 256? 1024?
There was a problem hiding this comment.
Not super easy because I actually had to disable this for www because of how slow it is :(
You could potentially try it on react-native after I land my diffs but landcastle is giving me trouble.
I used lower numbers, I think 16, 24 or 32 should be the max for concurrent promises that access the file system here.
There was a problem hiding this comment.
ok. I fear in the current form this will deadlock, because the the promise for a module only returns after all of its child promises return. Will refactor a bit.
cpojer
changed the title
|
Looks great, feel free to merge. A bit worried about the parallelization. I think we should throttle it. Feel free to add throat and merge this? |
Improvements for transforms running before dependency extraction
| const MAX_CONCURRENT_FILE_READS = 32; | ||
| const getDependencies = throat( | ||
| MAX_CONCURRENT_FILE_READS, | ||
| (module, transformOptions) => module.getDependencies(transformOptions) |
There was a problem hiding this comment.
Does this end up being recursive? If module.getDependencies ends up calling the throttled getDependencies, you could end up in deadlock, which would ultimately fail silently as it's undetectable. Providing getDependencies is not recursive, this looks fine 👍
There was a problem hiding this comment.
no, it’s not recursive – that’s why I’ve pulled it out of collect
There was a problem hiding this comment.
👍 great, just thought I should check.
|
Nice work!! |
ModuleCachecan createPolyfillobjects that are completly operationalResolutionRequestparallelizes dependency extraction instead of running it serially