deps,lib,src: remove dependency on --allow-natives-syntax #20719

addaleax · 2018-05-14T15:36:23Z

This is blocked on the freshly created https://chromium-review.googlesource.com/#/c/v8/v8/+/1057467 for now anyway. I don’t even have a good feeling for whether the V8 team is going to accept that, given that I get the impression that they seem a bit reluctant to add methods to the API that are not coming from the JS spec :)

fyi @BridgeAR @bnoordhuis @hashseed

Checklist

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

addaleax · 2018-05-14T15:37:33Z

test/parallel/test-util-inspect.js

@@ -935,8 +934,7 @@ if (typeof Symbol !== 'undefined') {
  const aSet = new Set([1, 3]);
  assert.strictEqual(util.inspect(aSet.keys()), '[Set Iterator] { 1, 3 }');
  assert.strictEqual(util.inspect(aSet.values()), '[Set Iterator] { 1, 3 }');
-  assert.strictEqual(util.inspect(aSet.entries()),
-                     '[Set Iterator] { [ 1, 1 ], [ 3, 3 ] }');
+  assert.strictEqual(util.inspect(aSet.entries()), '[Set Iterator] { 1, 3 }');


@nodejs/tsc Would we consider this a semver-major change? I’d prefer not to. If we do, I’d probably want to adjust the V8 CL first.

Edit: Would be cool to see as a quick poll: 👎 if you think it is semver-major, 👍 if you think this is fine as a patch change, 😕 if you don’t have an opinion

I think this is more a kind of bugfix than a breaking change.

BridgeAR

In general I am fine with the approach besides that it would be great to have the limit option as before.

About how this is exposed should be the decision of @hashseed and @bmeurer. We spoke about V8-extras.

BridgeAR · 2018-05-14T15:46:58Z

lib/util.js

@@ -915,7 +909,7 @@ function formatMap(ctx, value, recurseTimes, keys) {

 function formatWeakSet(ctx, value, recurseTimes, keys) {
  const maxArrayLength = Math.max(ctx.maxArrayLength, 0);
-  const entries = previewWeakSet(value, maxArrayLength + 1);
+  const entries = previewEntries(value).slice(0, maxArrayLength + 1);


This is pretty bad for performance. The same for the other parts.

Yeah, especially the lack of a limit is a bit “:confused:”, but on the other hand I don’t think any of these would typically be hot paths. (In particular, I don’t think we need to make showHidden: true fast.)

BridgeAR · 2018-05-14T15:47:49Z

test/parallel/test-util-inspect.js

@@ -935,8 +934,7 @@ if (typeof Symbol !== 'undefined') {
  const aSet = new Set([1, 3]);
  assert.strictEqual(util.inspect(aSet.keys()), '[Set Iterator] { 1, 3 }');
  assert.strictEqual(util.inspect(aSet.values()), '[Set Iterator] { 1, 3 }');
-  assert.strictEqual(util.inspect(aSet.entries()),
-                     '[Set Iterator] { [ 1, 1 ], [ 3, 3 ] }');
+  assert.strictEqual(util.inspect(aSet.entries()), '[Set Iterator] { 1, 3 }');


I think this is more a kind of bugfix than a breaking change.

TimothyGu

Node.js parts LGTM.

devsnek · 2018-05-14T19:07:31Z

lib/console.js

@@ -29,7 +29,7 @@ const {
    ERR_INVALID_ARG_VALUE,
  },
 } = require('internal/errors');
-const { previewMapIterator, previewSetIterator } = require('internal/v8');
+const { previewEntries } = process.binding('util');


can we put them on an internalBinding instead?

Any concrete preferences? The most stable part of process.binding('util') (type checking) is runtime-deprecated in Node 10, so we might be able to move the entire util binding to internalBinding('util') in Node 11 anyway…

whatever gets it done is fine with me, i just want to see process.binding stop existing :)

mcollina

LGTM on the node parts

bnoordhuis

Apropos the V8 changes, I'd probably add
WeakMap and WeakSet to v8.h that map to JSWeakMap and JSWeakSet. They have GetEntries() methods that let you limit the number of returned elements. That seems too useful to pass up.

Likewise, MapIterator and SetIterator, mapping to JSMapIterator and JSSetIterator. Cloning the iterator like we do now is pretty elegant and straightforward: just call NewJSMapIterator() or NewJSSetIterator() with the iterator's table and index.

(I can chime in on the CL if you want but I figured most of the discussion so far happened here.)

addaleax · 2018-05-15T20:00:57Z

@bnoordhuis Yeah, that sounds fine to me as well. It sounds like a non-trivial amount of extra work though, so I’d prefer to wait for feedback from somebody on the V8 team. :)

Original commit message: [api] Expose PreviewEntries as public API Turn `debug::EntriesPreview` into a public API. This is a straightforward approach to addressing nodejs#20409 (not relying on functionality behind `--allow-natives-syntax`) in Node.js. Refs: v8/v8@ff0a979

Use a new public V8 API for inspecting weak collections and collection iterators, rather than using V8-internal functions to achieve this. This currently comes with a slight modification of the output for inspecting iterators generated by `Set().entries()`. Fixes: nodejs#20409

addaleax · 2018-05-18T16:53:37Z

The upstream patch landed as it was in v8/v8@ff0a979, so I guess this is no longer blocked.

CI: https://ci.nodejs.org/job/node-test-commit/18595/

BridgeAR

LGTM even though I would really love a follow up PR on V8 that adds a limit option.

BridgeAR · 2018-05-18T17:04:48Z

src/node_util.cc

+    return args.GetReturnValue().Set(entries);
+
+  uint32_t length = entries->Length();
+  CHECK_EQ(length % 2, 0);


This check should actually be obsolete. We should realize it one way or the other if the V8 implementation is broken.

Right now, if there’s a single extra element at the end, we wouldn’t notice that. Like, yes, obviously that would be a bug in V8, but I don’t think there’s much harm in the extra check?

We should notice it because this code will only run in case we have a test for this code path and in those tests we actually strictly check the outcome.

devsnek · 2018-05-18T17:39:16Z

src/node_util.cc

+  Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
+
+  Local<Array> pairs = Array::New(env->isolate(), length / 2);


do we know the speed impact of this loop being in js vs c++?

@devsnek At least I don’t, but I guess the answer is that doing it in JS is a bit faster. But if we’re looking for optimizations, avoiding the pairs array altogether is probably a much better (but requires some more restructuring of existing code on the JS side) and like I said, I don’t think any of these code paths are typically hot paths anyway.

Thinking about it: that would not be complicated. It was actually implemented in WeakMap like that before. The output would not be as nice but it is definitely faster, so I am +1 on that.

See https://github.com/nodejs/node/pull/20719/files#diff-3deb3f32958bb937ae05c6f3e4abbdf5L938 for the former implementation. It does not seem to be much difference using this approach, especially since the C++ function here would be smaller.

That’s definitely a place where it’s easy to switch – but all other uses expected the array-of-tuples layout.

so I am +1 on that.

Yeah, I’d like that as well. Not as a blocker for this PR, though – you can feel freel to hack on this yourself, if you have concrete ideas about how this is going to look in the end?

addaleax · 2018-05-18T23:44:46Z

Landed in 143a2f8, 70cc5da

Original commit message: [api] Expose PreviewEntries as public API Turn `debug::EntriesPreview` into a public API. This is a straightforward approach to addressing #20409 (not relying on functionality behind `--allow-natives-syntax`) in Node.js. Refs: v8/v8@ff0a979 PR-URL: #20719 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Use a new public V8 API for inspecting weak collections and collection iterators, rather than using V8-internal functions to achieve this. This currently comes with a slight modification of the output for inspecting iterators generated by `Set().entries()`. Fixes: #20409 PR-URL: #20719 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Use a plain `[key, value, key, value]`-style list instead of an array of pairs for inspecting collections. This also fixes a bug with `console.table()` where inspecting a non-key-value `MapIterator` would have led to odd results. Refs: nodejs#20719 (comment)

Original commit message: [api] Expose PreviewEntries as public API Turn `debug::EntriesPreview` into a public API. This is a straightforward approach to addressing nodejs#20409 (not relying on functionality behind `--allow-natives-syntax`) in Node.js. Refs: v8/v8@ff0a979 PR-URL: nodejs#20719 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Original commit message: [api] Expose PreviewEntries as public API Turn `debug::EntriesPreview` into a public API. This is a straightforward approach to addressing #20409 (not relying on functionality behind `--allow-natives-syntax`) in Node.js. Refs: v8/v8@ff0a979 PR-URL: #20719 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Use a plain `[key, value, key, value]`-style list instead of an array of pairs for inspecting collections. This also fixes a bug with `console.table()` where inspecting a non-key-value `MapIterator` would have led to odd results. PR-URL: nodejs#20831 Refs: nodejs#20719 (comment) Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Use a plain `[key, value, key, value]`-style list instead of an array of pairs for inspecting collections. This also fixes a bug with `console.table()` where inspecting a non-key-value `MapIterator` would have led to odd results. PR-URL: #20831 Refs: #20719 (comment) Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2018-08-07T15:39:46Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

addaleax added util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency. blocked PRs that are blocked by other issues or PRs. labels May 14, 2018

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 14, 2018

addaleax commented May 14, 2018

View reviewed changes

BridgeAR reviewed May 14, 2018

View reviewed changes

TimothyGu approved these changes May 14, 2018

View reviewed changes

devsnek reviewed May 14, 2018

View reviewed changes

devsnek approved these changes May 14, 2018

View reviewed changes

jasnell approved these changes May 15, 2018

View reviewed changes

mcollina approved these changes May 15, 2018

View reviewed changes

bnoordhuis reviewed May 15, 2018

View reviewed changes

addaleax added 2 commits May 18, 2018 18:50

addaleax force-pushed the no-internal-v8 branch from c7646c9 to 1d660fe Compare May 18, 2018 16:51

addaleax removed the blocked PRs that are blocked by other issues or PRs. label May 18, 2018

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 18, 2018

BridgeAR approved these changes May 18, 2018

View reviewed changes

devsnek reviewed May 18, 2018

View reviewed changes

addaleax closed this May 18, 2018

addaleax deleted the no-internal-v8 branch May 18, 2018 23:44

addaleax mentioned this pull request May 18, 2018

console,util: avoid pair array generation in C++ #20831

Closed

3 tasks

helloshuangzi mentioned this pull request Jun 4, 2018

remove --allow-native-syntax flag during worker initialization microsoft/napajs#242

Merged

MylesBorins mentioned this pull request Jun 6, 2018

v10.4.0 proposal #21167

Merged

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps,lib,src: remove dependency on --allow-natives-syntax #20719

deps,lib,src: remove dependency on --allow-natives-syntax #20719

addaleax commented May 14, 2018

addaleax May 14, 2018 •

edited

Loading

BridgeAR May 14, 2018

BridgeAR left a comment

BridgeAR May 14, 2018

addaleax May 14, 2018 •

edited

Loading

BridgeAR May 14, 2018

TimothyGu left a comment

devsnek May 14, 2018

addaleax May 14, 2018

devsnek May 14, 2018

mcollina left a comment

bnoordhuis left a comment

addaleax commented May 15, 2018

addaleax commented May 18, 2018

BridgeAR left a comment

BridgeAR May 18, 2018

addaleax May 18, 2018

BridgeAR May 18, 2018

devsnek May 18, 2018

addaleax May 18, 2018

BridgeAR May 18, 2018

addaleax May 18, 2018

addaleax commented May 18, 2018

MylesBorins commented Aug 7, 2018

deps,lib,src: remove dependency on --allow-natives-syntax #20719

deps,lib,src: remove dependency on --allow-natives-syntax #20719

Conversation

addaleax commented May 14, 2018

Checklist

addaleax May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a 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

mcollina left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

addaleax commented May 15, 2018

addaleax commented May 18, 2018

BridgeAR left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented May 18, 2018

MylesBorins commented Aug 7, 2018

addaleax May 14, 2018 •

edited

Loading

addaleax May 14, 2018 •

edited

Loading