-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat: parallelize downloads from TS compiler #2949
feat: parallelize downloads from TS compiler #2949
Conversation
1d5e968
to
01c246e
Compare
I must admit, doing this PR with JSON ops is a breeze... I couldn't figure it out with Flatbufs a couple times. |
"mediaType": out.media_type as i32, | ||
"sourceCode": String::from_utf8(out.source_code).unwrap(), | ||
}))) | ||
let files = tokio_util::block_on(futures::future::join_all(futures))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 elegant
(at some point we need to get rid of this block_on stuff tho...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - no comments
This is bad news. We really need to figure out what to do with |
@bartlomieju The issue is that we're starting a whole new tokio runtime for each module resolved - it is wildly inefficient.
No |
@bartlomieju After some thought, I will not revert your patch. Your patch speeds up an important (unfortunately unmeasured) benchmark. I think what you're doing is more or less correct and just exposing a lower-level problem. I've added #2960 to 1.0 blockers to solve this most important design problem. |
@ry alright, I will investigate alternate solutions as well |
Fixes #2626
A small change to the compiler that requests all imports from a file at once instead of doing N round trips. Still needs a bit of cleanup.
master
this PR
CC @ry @kitsonk