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

chore: make jsii fully synchronous #3467

Merged
merged 9 commits into from Apr 4, 2022
Merged

chore: make jsii fully synchronous #3467

merged 9 commits into from Apr 4, 2022

Conversation

RomainMuller
Copy link
Contributor

With a sample set of 1, I have detemined that there is about 30% tax
caused by asynchronous functions in the compiler. The typescript
compiler itself is fully synchronous, and we basically moved to do the
same, and this turns out reducing the amount of time wasted in the
runloop and in promise bookeeping.

Tested on aws-cdk-lib, it turns out to be reducing the compile time
from ~142 seconds to ~100 seconds.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

With a sample set of 1, I have detemined that there is about 30% tax
caused by asynchronous functions in the compiler. The `typescript`
compiler itself is fully synchronous, and we basically moved to do the
same, and this turns out reducing the amount of time wasted in the
runloop and in promise bookeeping.

Tested on `aws-cdk-lib`, it turns out to be reducing the compile time
from ~142 seconds to ~100 seconds.
@RomainMuller RomainMuller requested a review from a team April 1, 2022 01:09
@RomainMuller RomainMuller self-assigned this Apr 1, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 1, 2022
@MrArnoldPalmer
Copy link
Contributor

That seems like an inordinate % of time spent just in promise/callback/event loop management. I wonder if the TS compiler being fully synchronous is for performance or design reasons. Even their fs calls when writing .d.ts and .js file output are sync? Either way it's quite interesting. I'd be curious to run comparisons across more libraries and different hardware configurations with some flamegraphs for a clearer picture of the difference.

@RomainMuller
Copy link
Contributor Author

That seems like an inordinate % of time spent just in promise/callback/event loop management. I wonder if the TS compiler being fully synchronous is for performance or design reasons. Even their fs calls when writing .d.ts and .js file output are sync? Either way it's quite interesting. I'd be curious to run comparisons across more libraries and different hardware configurations with some flamegraphs for a clearer picture of the difference.

I reckon the TS maintainers wanted to turn the compiler async but it turned out to be a humongous task. I think the TS compiler itself spends enough time doing I/O that using async will yield performance improvements, whereas the jsii compiler performs mostly busy compute in JS, where async turns out to be mostly just overhead.

But all in all I agree this magnitude of a difference looks pretty surprising. I expected some, but not that much... It could be just the particular case I used magnifies that (the measurement protocol isn't very scientific here)...

Copy link
Contributor

@yuth yuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -186,7 +184,7 @@ export class Compiler implements Emitter {
return watch;
}
// In blocking mode, returns a never-resolving promise.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be this is comment not relevant anymore

@mergify
Copy link
Contributor

mergify bot commented Apr 4, 2022

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Apr 4, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 4, 2022

Merging (with squash)...

@mergify mergify bot merged commit 2be1cde into main Apr 4, 2022
@mergify mergify bot deleted the rmuller/performance-audit branch April 4, 2022 17:06
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Apr 4, 2022
yuth added a commit to yuth/jsii that referenced this pull request Apr 12, 2022
In aws#3467 the comiler was made fully synchronous which also removed a never resolving promise used by --watch. When the compile is run in watch mode, the watcher is started in the background and control is returned back with watcher. For the compiler watcher to keep running the control should not be yeilded back to the caller
mergify bot pushed a commit that referenced this pull request Apr 13, 2022
While making  the compiler fully synchronous (in  #3467) a un-resolveable promise was removed which is used by `--watch`. When using watch mode, the typescript compiler creates a watcher, and hands back the control to the caller of watch, to allow caller to control the watcher instance. Since JSII does not do anything with the returned control, we need to return an un-resolveable promise which will keep the watcher running for ever (or till the whole program is killed)


---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants