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

Improve require speed #358

Merged
merged 4 commits into from Sep 27, 2019
Merged

Improve require speed #358

merged 4 commits into from Sep 27, 2019

Conversation

stroncium
Copy link
Contributor

@stroncium stroncium commented Jul 22, 2019

  • Lazy load templates(runtime overhead should be zero due to prototype check overlapping with === undefined check)
  • Don't access ansi-styles color space converters unless needed

This patch works in conjunction with(but works without them too, though to much smaller effect):

Require speed test results before:
before

Require speed test results after(pure require, without using template nor color conversion functionality):
after

Note: in both cases, a huge part of time is spent initializing tty(or initializing process.stdout and process.stderr, which includes initializing tty), which is also triggered by a lot of other parts of stdlib(for example by console.log with non-string arguments), and is required in order to detect if output supports color, so is expected to be initialized anyway by a lot of real life programs(notable exception: programs which conditionally produce no output). If we subtract the time required for this initializations, the final numbers will be ~10ms and ~5ms, respectively.

Fixes #180


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

source/index.js Outdated Show resolved Hide resolved
source/index.js Show resolved Hide resolved
@stroncium
Copy link
Contributor Author

stroncium commented Sep 20, 2019

With newer version of nodejs(I tested on 12.10.0) all the require speeds increased quite a lot and tty require speed increased dramatically(6.0ms => 3.8ms), so I'm currently getting 6.4 ms total require time with 3.8 going to tty on the same machine. (Too lazy to relink all the stuff to compare with results prior to all the fixes here and in deps.)

With this in mind, I think inlining basic color converters in this module to reduce require time even further doesn't really make sense anymore, as the gains should be minor. So I consider the patch complete in terms of functionality. (Though it still makes some sense to do in general, but can be implemented in ansi-styles.)

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Sep 20, 2019

Since we’re removing undocumented colour spaces, this is probably a breaking change.

@stroncium
Copy link
Contributor Author

stroncium commented Sep 20, 2019

@ExE-Boss changes in undocumented features is not considered API change, and not a breaking one for sure. Though in any case this code is scheduled for chalk 3.0 release as far as I understand.

@sindresorhus
Copy link
Member

@stroncium Can you fix the merge conflict?

@sindresorhus
Copy link
Member

Awesome. Thanks for working on this 🙌

@Turbo87
Copy link

Turbo87 commented Feb 28, 2020

@stroncium out of curiosity: what tool did you use to create that terminal screenshot in the PR description?

@stroncium
Copy link
Contributor Author

@Turbo87 That would be my private tool for node performance metrics and optimization. Didn't get to write docs for it and release publicly yet, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve require speed
5 participants