-
Notifications
You must be signed in to change notification settings - Fork 30k
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: add computeTime option to inspect #19994
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
|
||
// `util.inspect` is capable of computing a string that is bigger than 2 ** 28 | ||
// in one second, so let's just skip this test on 32bit systems. | ||
common.skipIf32Bits(); | ||
|
||
// Test that big objects are running only up to the maximum complexity plus ~1 | ||
// second. | ||
|
||
const util = require('util'); | ||
|
||
// Create a difficult to stringify object. Without the limit this would crash. | ||
let last = {}; | ||
const obj = last; | ||
|
||
for (let i = 0; i < 1000; i++) { | ||
last.next = { circular: obj, last, obj: { a: 1, b: 2, c: true } }; | ||
last = last.next; | ||
obj[i] = last; | ||
} | ||
|
||
common.expectWarning( | ||
'Warning', | ||
'util.inspect took to long.', | ||
'INSPECTION_ABORTED' | ||
); | ||
|
||
util.inspect(obj, { depth: Infinity, budget: 1e6 }); |
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.
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.
Is this necessary? Leaving out wall time tracking results in a simpler algorithm.
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.
The point is: without this it is hard to tell how much time this will actually need on different CPUs. Using the wall time is future proof by always delivering the best result with a best effort mechanism.
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.
What I mean is, is tracking any kind of time necessary?
What you call complexity, I would call budget. You start with a fixed budget and spend it until it's gone.
Since you have that kind of logic in place already, and since it's deterministic and clock agnostic, why also track time?
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.
Budget is indeed a good description. I am just not sure if this would be understood by looking at a option named
budget
.Relying on the budget completely without checking the wall time as well means I can measure the time it takes on my machine and estimate a arbitrary time limit that should not be exceeded in the default case (e.g. 1000-1250 ms as currently done). However, we also have Raspberry PIs and similar and their CPUs are much much slower and the same budget would get spend slower and therefore block the event loop longer. Our CPUs in general get faster over time with new inventions though, so the current default case for a fast machine would soon drop and the budget is spend faster. Meaning the maximum time the event loop would be allowed to be blocked would drop to e.g., 250ms instead of the current limit.
Using both together makes sure that it will always be around the 1000-1250ms range. That is the reason why I would like to have the combination of both, even though it is not fully deterministic after spending the complete budget.
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.
That's a fair point. Have you considered splitting it out into two separate options, one for budget/complexity and one for wall time? Or are you saying that you feel a single option is best?
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 feel a single option is best for two reasons: