-
Notifications
You must be signed in to change notification settings - Fork 1.8k
TS: do more work in parallel #479
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
TS: do more work in parallel #479
Conversation
@xiemaisi it will most likely conflict with the parallel extraction PR. I'm happy to withhold this until that lands, as long as it gets in before feature freeze. |
a0021f3
to
b5d3dd5
Compare
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.
One question, otherwise LGTM. I really like the idea of this PR, and I don't think it needs to wait until the parallel extraction has landed (which is looking doubtful again anyway).
extractorState.getTypeScriptParser().prepareFiles(files); | ||
for (File f : files) { | ||
Path path = f.toPath(); | ||
if (extractedFiles.add(path)) { |
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.
If we skip a file here (because it has already been extracted), won't the TypeScript parser throw an error on the next file we request an AST for?
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.
Good point. It can't happen, but it's not very obvious at the moment.
The list of files given here has been built to only contain unextracted files, so the return value of add
really doesn't need to be checked. I'll try to make this a bit more clear.
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.
Oh wait, there was a huge bug in the last commit here.
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.
Turns out the attempt to get AutoBuildTest
working actually introduced a bug. The extracted file was added twice, and thereby skipped in the extraction step.
We'll obviously need some better testing for this file.
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.
I've put up an internal PR that fixes the test that should catch this.
I've confirmed that the test fails for the previous commit, and passes after 84c1ba0.
…-notes Add missing change notes
This PR speeds up TypeScript extraction by making the Java process and Node.js process do more work in parallel.
When the Node.js process has sent an AST, it immediately starts to work on the next AST while the Java process is processing the first one. In order for the Node.js process knows which file to work on next, a list of files to work on is sent in advance using the new
prepare-files
command.In principle the Node.js process could prepare more than one file in advance, but then we risk being busy when the Java process is ready to receive the next file, delaying overall extraction. So it only looks one file ahead.
Extraction-time benchmarks:
The full-mode extraction for TypeScript itself is a bit of an outlier, but I'll look into that separately.