util: change inspect's default depth to 4

[silverwind]

This changed the default inspect depth to 4. This is a revival of #23062 which was closed in favor of #22846 but that one ended up being reverted so we're back at 2 which I think is just too low as I often find myself in need of one or two more depth levels.

Carefully marking it as semver-major, thought it probably doesn't need to be strictly speaking.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

[silverwind]

Note that this is just documenting existing behaviour. %o is already defaulting to depth of 4 before this change here.

[silverwind]

Not particularily happy with how these are printed, by the way. It almost seems like the linebreak logic in inspect has a bias towards depth of 2, but didn't investigate that yet.

[BridgeAR]

The default is currently set to compact: 3. That means the three most inner levels are combined on a single line in case they fit into breakLength. This has "five" levels (the most inner part counts as primitive, not as level). That's why the most outer two levels create the new lines.

[silverwind]

I see. Do you recall why the particular value of 3 was chosen? Was it to accomodate a depth of 2 or are they unrelated?

[BridgeAR]

Checking how many objects are all on the same line is difficult when it becomes to many. That's why I limited it to three by default. It is unrelated to the depth.

[devsnek]

above 2 tended to cause node to hang for long periods of time while inspecting large objects. I believe the example used when reverting was console.log(require) because of require.cache)

[BridgeAR]

I personally think it would be better to rewrite the algorithm to a breadth first algorithm (it's currently depth first). That way it would be possible to accommodate deeper levels in case the object itself is not that big but hide deeper levels when it becomes too big.

When I originally suggested to increase the depth I mainly focused on the performance and to make sure the inspection is done fast (and that's the case). I did not pay enough attention to the fact that rendering takes a long time and that huge outputs become less useful. That can be solved with the breadth first algorithm. For those reasons I am -0.75 on this.

[silverwind]

Did some tests inspecting some common objects with different depths:

$ cat http.js
process.stdout._handle.setBlocking(true);
require("http").createServer(req => {console.log(req); process.exit()}).listen(6000);
$ cat express.js
process.stdout._handle.setBlocking(true);
require("express")().get("/", req => {console.log(req); process.exit()}).listen(6000);
$ cat require.js
process.stdout._handle.setBlocking(true);
require("express")().get("/", req => {console.log(require); process.exit()}).listen(6000);

(setBlocking is used because of #6456. 😢)

$ node http.js | wc -c
17397
$ node express.js | wc -c
21480
$ node require.js | wc -c
38908
$ ./node http.js | wc -c
18876
$ ./node express.js | wc -c
51023
$ ./node require.js | wc -c
981542

The output for the require object gains significantly in size, but I guess it will never be as bad as with previous 20 (or was it Infinity?).

breadth first algorithm

Looking forward to that. If I understand correctly, this should gain us quite a bit performance as well for large objects. I don't think it should be a prereq here, thought.

[BridgeAR]

@silverwind

Looking forward to that. If I understand correctly, this should gain us quite a bit performance as well for large objects. I don't think it should be a prereq here, thought.

I think it'll be slower since the algorithm is likely more complex in our case and it will mainly allow deeper inspection for small objects. Big objects would probably just be printed as they are right now.
About it being a pre-requisition: I think it should be. Otherwise we'll break some use cases again and it seems like it's a relatively common thing to rely upon the current depth / fast inspection / smaller output.

[silverwind]

pre-requisition: I think it should be

ok, I'll close this then, given that there does not seem to be any interest from collaborators to change this default now. We should revisit later, of course.