Skip to content
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

Async compiler processing #3043

Merged
merged 5 commits into from Oct 3, 2019

Conversation

@kitsonk
Copy link
Contributor

commented Oct 2, 2019

Fixes #2994

This has taken a while, but it is functionally complete, except I just found a few problems with the tests, specifically around supporting the type resolution feature, which I accidentally broke, but wanted to share early it as I finish it off.

Basically this does pre-processing of TypeScript files and gathers all the dependencies asynchronously. Only then after all the dependencies are gathered, does it do a compile, which at that point all the dependencies are cached in memory in the compiler, so with the exception of the hard coded assets, there are no ops during the compilation.

Because op_fetch_source_files is now handled asynchronously in the runtime, we can eliminate the tokio_util::block_on() which was causing the increase in threads. Benchmarking on my machine has shown about a 5% improvement in speed when dealing with compiling TypeScript. Still a long way to go, but an improvement.

In theory the module name resolution and the fetching of the source files could be broken out as two different ops. This would prevent situations of sending the full source file all the time when actually the module is the same module referenced by multiple modules, but that could be done subsequently to this.

@kitsonk kitsonk force-pushed the kitsonk:preprocess branch from 78a694e to 3f24f85 Oct 2, 2019
@kitsonk kitsonk changed the title [WIP] Async compiler processing Async compiler processing Oct 2, 2019
@kitsonk kitsonk force-pushed the kitsonk:preprocess branch from 3f24f85 to cbe220e Oct 2, 2019
@kitsonk

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

ok, fully ready for review @ry @bartlomieju

Copy link
Contributor

left a comment

@kitsonk this is very nice 👍 I just have a few nitpicks

In theory the module name resolution and the fetching of the source files could be broken out as two different ops. This would prevent situations of sending the full source file all the time when actually the module is the same module referenced by multiple modules, but that could be done subsequently to this.

I'd be very cautious about it. Currently all redirection handling logic is hidden in Rust, which effectively means that we need to make actual request to be sure that https://foo.bar/a.ts won't be turned into https://foo.bar/redirected/b.ts.

js/compiler.ts Outdated Show resolved Hide resolved
js/compiler.ts Show resolved Hide resolved
js/compiler.ts Outdated Show resolved Hide resolved
js/compiler.ts Show resolved Hide resolved
cli/ops/compiler.rs Show resolved Hide resolved
kitsonk added 2 commits Oct 2, 2019
@kitsonk kitsonk force-pushed the kitsonk:preprocess branch from cbe220e to dc9aed4 Oct 2, 2019
@ry

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

@kitsonk I've removed "Fixes #2960" from the description because there are still usages of block_on... but I suspect this is close to allowing us to remove the other call sites.

js/type_directives.ts Show resolved Hide resolved
js/compiler.ts Show resolved Hide resolved
ry added 2 commits Oct 3, 2019
This reverts commit 3aeaabc.
@ry
ry approved these changes Oct 3, 2019
Copy link
Collaborator

left a comment

LGTM - very nice!

I created an issue for splitting up the op into resolution and source fetching: #3054

@ry ry merged commit d9ff4ec into denoland:master Oct 3, 2019
8 checks passed
8 checks passed
test macOS-10.14
Details
lint
Details
test windows-2016
Details
test ubuntu-16.04
Details
bench ubuntu-16.04
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@ry

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

This PR resulted in massive thread usage improvements:

Screen Shot 2019-10-03 at 7 37 12 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.