-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
util: delay creation of debug context #2248
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We need the debug context to be able to inspect promises. However, this is very expensive and should not be done on default startup.
mscdex
added
util
Issues and PRs related to the built-in util module.
vm
Issues and PRs related to the vm subsystem.
labels
Jul 25, 2015
LGTM |
LGTM, the difference is huge ! |
LGTM. Obvious in hindsight. |
So does this mean that the first time you |
LGTM |
@Fishrock123 Yes, it is going to be expensive for the first promise that is inspected. |
Definitely worth the tradeoff IMO. LGTM. |
The CI looks decent to me, ignoring the flaky failures. I will land this later today. |
ofrobots
added a commit
that referenced
this pull request
Jul 28, 2015
We need the debug context to be able to inspect promises. However, this is very expensive and should not be done on default startup. PR-URL: #2248 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Christopher Monsanto <chris@monsan.to> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in ab47965. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that it is really simple to take profiles with node (thanks to #2090), I was looking a profile I got from a trivial
JSON.stringify
benchmark.I was really surprised by the number of ticks I got in
RunInDebugContext
. The full profile is here but here's an excerpt:I looked into this. It seems that #1471 introduced a change which now creates a Debug context by default in the util module. This is needed to be able to inspect promises. I think this is fine, but we should not be doing the really expensive operation of creating a Debug context on default startup of node. This patch fixes that.
Based on my measurements, this saves ~24ms in node startup time measured as follows:
This reduces startup time from ~90ms → ~66ms on my machine. I think it is important that node startup is as fast as possible. I don't like deferred initialization as I am proposing, but I think it is a much lesser evil than spending 24ms on each node startup.
R=@bnoordhuis, @monsanto