util: change inspect depth default #17907

BridgeAR · 2017-12-28T22:56:49Z

Right now the defaults for util.inspect are often not ideal. This is a first step to improve those. I tried not to break to much and keep the current behavior for the repl and console.

Refs: #12693

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

util, console

lpinca · 2017-12-29T08:06:03Z

lib/util.js

@@ -317,7 +317,9 @@ Object.defineProperty(inspect, 'defaultOptions', {
    if (options === null || typeof options !== 'object') {
      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object');
    }
-    Object.assign(inspectDefaultOptions, options);


Any reason for not using _extend?

The type check has to be done again in that case.

I doubt it will add a noticeable overhead but yes it makes sense.

Do we actually care about the performance of inspection? I thought we didn’t.

Same concern here as @lpinca. This is quite literally reinventing the wheel, and type checks for object are quite fast in modern V8.

Updated the use _extend as it will still be faster than Object.assign

About the performance - just recently I heard that inspect came up in a flamegraph. Seems like people use it a lot. I am aware that setting the default options will not be a big thing but the first implementation (not changing inspect for console.*) used it a lot and that is why I decided to optimize it as well.

silverwind · 2017-12-29T11:34:54Z

Not sure I like the complexity introduced here. Given that this is already flagged as semver-major, why not update it to 5 (I'd leave that value open to discussion, but it's certainly more useful than the current 2) everywhere via new defaults?

BridgeAR · 2017-12-29T11:46:55Z

@silverwind I do not see a big difference between 5 and unlimited. Most entries are not that deeply nested and if they are, you likely want to see everything. To prevent huge diffs, the max array entries option is way more efficient as this would otherwise be more difficult to handle.

About the complexity: I am fine with showing everything as a new default for console.* and this would remove the complexity again. But it would be a more significant change and I tried to minimize the breaking change.

For the repl it might be best to stay with the current default though. The reason is that some modules like fs have lots of nested elements and it would be be verbose otherwise.

BridgeAR · 2017-12-29T18:24:49Z

@jasnell what do you think about removing the limit for console.* as well? I guess most people would actually profit from that.

jasnell · 2017-12-29T18:36:10Z

Changing the default for console.* should be ok given that this is semver-major

Currently inspecting the BufferList can result a maximum call stack size error. Adding a individual inspect function prevents this.

The current default is not ideal in most use cases. Therefore it is changed to showing unlimited depth in case util.inspect is called directly. The default is kept as before for console.log and similar. Using console.dir will now show a depth of up to five and console.assert / console.trace will show a unlimited depth.

Object.assign is currently very slow. Using Object.keys is much faster in v8 6.3.

BridgeAR · 2017-12-30T01:57:13Z

I changed the code to default to unlimited for the console functions as well.
To make that work I had to add a custom inspect function to the BufferList.

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1179/
CI https://ci.nodejs.org/job/node-test-pull-request/12350/

TimothyGu · 2017-12-30T06:10:34Z

lib/internal/streams/BufferList.js

@@ -73,4 +74,8 @@ module.exports = class BufferList {
    }
    return ret;
  }
+
+  [customInspectSymbol]() {
+    return `${this.constructor.name} { length: ${this.length} }`;


You could return { __proto__: Object.getPrototypeOf(this), length: this.length } which would also give coloring etc. for free.

Using __proto__ will actually result in a `call stack size exceeded error. I decided to call inspect directly instead to highlight the color in case it is needed.

TimothyGu · 2017-12-30T06:15:18Z

lib/util.js

@@ -317,7 +317,9 @@ Object.defineProperty(inspect, 'defaultOptions', {
    if (options === null || typeof options !== 'object') {
      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object');
    }
-    Object.assign(inspectDefaultOptions, options);


Do we actually care about the performance of inspection? I thought we didn’t.

Same concern here as @lpinca. This is quite literally reinventing the wheel, and type checks for object are quite fast in modern V8.

BridgeAR · 2018-01-02T01:24:07Z

New CI https://ci.nodejs.org/job/node-test-pull-request/12363/

mcollina

LGTM

targos

Can you please change the documentation of util.inspect (and add an entry to its history)?

addaleax · 2018-01-02T15:19:05Z

lib/internal/streams/BufferList.js

@@ -73,4 +75,9 @@ module.exports = class BufferList {
    }
    return ret;
  }
+
+  [customInspectSymbol]() {


I’d just use util.inspect.custom here, it saves people looking at this code one round-trip to another file

Since the default for depth is changed to `Infinity` it is logical to change the %o default to the same as well. Using %o with `util.format` will now always print the whole object.

BridgeAR · 2018-01-03T15:51:57Z

I updated the documentation and added the history entry. I also changed the %o default of util.format to also use Infinity as default.

targos · 2018-01-07T22:15:25Z

doc/api/util.md

+  * `depth` {number} Specifies the number visible nested Objects in an `object`.
+    This is useful to minimize the inspection output for large complicated
+    objects. To make it recurse indefinitely pass `null` or `Infinity`. Defaults
+    to `null`.


suggestion: Infinity instead of null to match the changelog?

The current default is not ideal in most use cases. Therefore it is changed to inspect objects to a maximum depth of 20 in case util.inspect is called with it's defaults. The default is kept at 2 when using console.log() and similar in the repl. PR-URL: nodejs#17907 Refs: nodejs#12693

Since the default for depth is changed to `20` it is logical to change the %o default as well. It will now always use the default depth. PR-URL: nodejs#17907 Refs: nodejs#12693

The current default is not ideal in most use cases. Therefore it is changed to inspect objects to a maximum depth of 20 in case util.inspect is called with it's defaults. The default is kept at 2 when using console.log() and similar in the repl. PR-URL: nodejs#17907 Refs: nodejs#12693 PR-URL: nodejs#22846 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Since the default for depth is changed to `20` it is logical to change the %o default as well. It will now always use the default depth. PR-URL: nodejs#17907 Refs: nodejs#12693 PR-URL: nodejs#22846 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

nodejs-github-bot added console Issues and PRs related to the console subsystem. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module. labels Dec 28, 2017

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 28, 2017

lpinca reviewed Dec 29, 2017

View reviewed changes

jasnell approved these changes Dec 29, 2017

View reviewed changes

BridgeAR added 3 commits December 30, 2017 02:29

streams: add custom inspect to BufferList

94b7b01

Currently inspecting the BufferList can result a maximum call stack size error. Adding a individual inspect function prevents this.

util: improve setting default options

2721767

Object.assign is currently very slow. Using Object.keys is much faster in v8 6.3.

BridgeAR force-pushed the change-inspect-defaults branch from 2bb38d6 to 2721767 Compare December 30, 2017 01:36

TimothyGu reviewed Dec 30, 2017

View reviewed changes

starkwang mentioned this pull request Dec 30, 2017

Some jenkins jobs failed to push state. nodejs/github-bot#163

Closed

fixup address comments

606f06c

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 2, 2018

BridgeAR requested a review from a team January 2, 2018 01:42

mcollina approved these changes Jan 2, 2018

View reviewed changes

targos requested changes Jan 2, 2018

View reviewed changes

addaleax reviewed Jan 2, 2018

View reviewed changes

BridgeAR added 2 commits January 3, 2018 16:45

fixup update documentation

3ca34ca

util: change %o depth default

8ab8726

Since the default for depth is changed to `Infinity` it is logical to change the %o default to the same as well. Using %o with `util.format` will now always print the whole object.

targos approved these changes Jan 7, 2018

View reviewed changes

fixup

e220598

targos removed the dont-land-on-v10.x label Jun 8, 2018

BridgeAR deleted the change-inspect-defaults branch April 1, 2019 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: change inspect depth default #17907

util: change inspect depth default #17907

BridgeAR commented Dec 28, 2017

lpinca Dec 29, 2017

BridgeAR Dec 29, 2017

lpinca Dec 29, 2017

TimothyGu Dec 30, 2017

BridgeAR Jan 2, 2018

BridgeAR Jan 2, 2018

silverwind commented Dec 29, 2017

BridgeAR commented Dec 29, 2017 •

edited

Loading

BridgeAR commented Dec 29, 2017

jasnell commented Dec 29, 2017

BridgeAR commented Dec 30, 2017

TimothyGu Dec 30, 2017

BridgeAR Jan 2, 2018

TimothyGu Dec 30, 2017

BridgeAR commented Jan 2, 2018

mcollina left a comment

targos left a comment

addaleax Jan 2, 2018

BridgeAR Jan 3, 2018

BridgeAR commented Jan 3, 2018

targos Jan 7, 2018

util: change inspect depth default #17907

util: change inspect depth default #17907

Conversation

BridgeAR commented Dec 28, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Dec 29, 2017

BridgeAR commented Dec 29, 2017 • edited Loading

BridgeAR commented Dec 29, 2017

jasnell commented Dec 29, 2017

BridgeAR commented Dec 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 2, 2018

mcollina left a comment

Choose a reason for hiding this comment

targos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 3, 2018

Choose a reason for hiding this comment

BridgeAR commented Dec 29, 2017 •

edited

Loading