Skip to content
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.

Sign up for GitHub

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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0babe5a
deps: cherry-pick ff0a9793334 from upstream V8
addaleax May 18, 2018
1d660fe
lib,src: use V8 API for collection inspection
addaleax May 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions deps/v8/include/v8.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3503,6 +3503,17 @@ class V8_EXPORT Object : public Value {
*/
Isolate* GetIsolate();

/**
* If this object is a Set, Map, WeakSet or WeakMap, this returns a
* representation of the elements of this object as an array.
* If this object is a SetIterator or MapIterator, this returns all
* elements of the underlying collection, starting at the iterator's current
* position.
* For other types, this will return an empty MaybeLocal<Array> (without
* scheduling an exception).
*/
MaybeLocal<Array> PreviewEntries(bool* is_key_value);

static Local<Object> New(Isolate* isolate);

V8_INLINE static Object* Cast(Value* obj);
Expand Down
19 changes: 9 additions & 10 deletions deps/v8/src/api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9526,21 +9526,20 @@ int debug::EstimatedValueSize(Isolate* v8_isolate, v8::Local<v8::Value> value) {
return i::Handle<i::HeapObject>::cast(object)->Size();
}

v8::MaybeLocal<v8::Array> debug::EntriesPreview(Isolate* v8_isolate,
v8::Local<v8::Value> value,
bool* is_key_value) {
i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
if (value->IsMap()) {
v8::MaybeLocal<v8::Array> v8::Object::PreviewEntries(bool* is_key_value) {
if (IsMap()) {
*is_key_value = true;
return value.As<Map>()->AsArray();
return Map::Cast(this)->AsArray();
}
if (value->IsSet()) {
if (IsSet()) {
*is_key_value = false;
return value.As<Set>()->AsArray();
return Set::Cast(this)->AsArray();
}

i::Handle<i::Object> object = Utils::OpenHandle(*value);
i::Handle<i::JSReceiver> object = Utils::OpenHandle(this);
i::Isolate* isolate = object->GetIsolate();
Isolate* v8_isolate = reinterpret_cast<Isolate*>(isolate);
ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
if (object->IsJSWeakCollection()) {
*is_key_value = object->IsJSWeakMap();
return Utils::ToLocal(i::JSWeakCollection::GetEntries(
Expand Down
4 changes: 0 additions & 4 deletions deps/v8/src/debug/debug-interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,6 @@ void ResetBlackboxedStateCache(Isolate* isolate,

int EstimatedValueSize(Isolate* isolate, v8::Local<v8::Value> value);

v8::MaybeLocal<v8::Array> EntriesPreview(Isolate* isolate,
v8::Local<v8::Value> value,
bool* is_key_value);

enum Builtin {
kObjectKeys,
kObjectGetPrototypeOf,
Expand Down
4 changes: 3 additions & 1 deletion deps/v8/src/inspector/v8-debugger.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@ v8::MaybeLocal<v8::Array> collectionsEntries(v8::Local<v8::Context> context,
v8::Isolate* isolate = context->GetIsolate();
v8::Local<v8::Array> entries;
bool isKeyValue = false;
if (!v8::debug::EntriesPreview(isolate, value, &isKeyValue).ToLocal(&entries))
if (!value->IsObject() ||
!value.As<v8::Object>()->PreviewEntries(&isKeyValue).ToLocal(&entries)) {
return v8::MaybeLocal<v8::Array>();
}

v8::Local<v8::Array> wrappedEntries = v8::Array::New(isolate);
CHECK(!isKeyValue || wrappedEntries->Length() % 2 == 0);
Expand Down
6 changes: 3 additions & 3 deletions lib/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const {
ERR_INVALID_ARG_VALUE,
},
} = require('internal/errors');
const { previewMapIterator, previewSetIterator } = require('internal/v8');
const { previewEntries } = process.binding('util');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we put them on an internalBinding instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { Buffer: { isBuffer } } = require('buffer');
const cliTable = require('internal/cli_table');
const util = require('util');
Expand Down Expand Up @@ -343,7 +343,7 @@ Console.prototype.table = function(tabularData, properties) {

const mapIter = isMapIterator(tabularData);
if (mapIter)
tabularData = previewMapIterator(tabularData);
tabularData = previewEntries(tabularData);

if (mapIter || isMap(tabularData)) {
const keys = [];
Expand All @@ -365,7 +365,7 @@ Console.prototype.table = function(tabularData, properties) {

const setIter = isSetIterator(tabularData);
if (setIter)
tabularData = previewSetIterator(tabularData);
tabularData = previewEntries(tabularData);

const setlike = setIter || isSet(tabularData);
if (setlike) {
Expand Down
17 changes: 0 additions & 17 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
// do this good and early, since it handles errors.
setupProcessFatal();

setupV8();
setupProcessICUVersions();

setupGlobalVariables();
Expand Down Expand Up @@ -471,22 +470,6 @@
};
}

function setupV8() {
// Warm up the map and set iterator preview functions. V8 compiles
// functions lazily (unless --nolazy is set) so we need to do this
// before we turn off --allow_natives_syntax again.
const v8 = NativeModule.require('internal/v8');
v8.previewMapIterator(new Map().entries());
v8.previewSetIterator(new Set().entries());
v8.previewWeakMap(new WeakMap(), 1);
v8.previewWeakSet(new WeakSet(), 1);
// Disable --allow_natives_syntax again unless it was explicitly
// specified on the command line.
const re = /^--allow[-_]natives[-_]syntax$/;
if (!process.execArgv.some((s) => re.test(s)))
process.binding('v8').setFlagsFromString('--noallow_natives_syntax');
}

function setupProcessICUVersions() {
const icu = process.binding('config').hasIntl ?
process.binding('icu') : undefined;
Expand Down
39 changes: 0 additions & 39 deletions lib/internal/v8.js
View file
Open in desktop

This file was deleted.

32 changes: 11 additions & 21 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,12 @@ const {
const { TextDecoder, TextEncoder } = require('internal/encoding');
const { isBuffer } = require('buffer').Buffer;

const {
previewMapIterator,
previewSetIterator,
previewWeakMap,
previewWeakSet
} = require('internal/v8');

const {
getPromiseDetails,
getProxyDetails,
kPending,
kRejected,
previewEntries
} = process.binding('util');

const { internalBinding } = require('internal/bootstrap/loaders');
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@addaleax addaleax May 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

const maxLength = Math.min(maxArrayLength, entries.length);
let output = new Array(maxLength);
for (var i = 0; i < maxLength; ++i)
Expand All @@ -932,16 +926,14 @@ function formatWeakSet(ctx, value, recurseTimes, keys) {

function formatWeakMap(ctx, value, recurseTimes, keys) {
const maxArrayLength = Math.max(ctx.maxArrayLength, 0);
const entries = previewWeakMap(value, maxArrayLength + 1);
// Entries exist as [key1, val1, key2, val2, ...]
const remainder = entries.length / 2 > maxArrayLength;
const len = entries.length / 2 - (remainder ? 1 : 0);
const entries = previewEntries(value).slice(0, maxArrayLength + 1);
const remainder = entries.length > maxArrayLength;
const len = entries.length - (remainder ? 1 : 0);
const maxLength = Math.min(maxArrayLength, len);
let output = new Array(maxLength);
for (var i = 0; i < len; i++) {
const pos = i * 2;
output[i] = `${formatValue(ctx, entries[pos], recurseTimes)} => ` +
formatValue(ctx, entries[pos + 1], recurseTimes);
output[i] = `${formatValue(ctx, entries[i][0], recurseTimes)} => ` +
formatValue(ctx, entries[i][1], recurseTimes);
}
// Sort all entries to have a halfway reliable output (if more entries than
// retrieved ones exist, we can not reliably return the same output).
Expand All @@ -953,9 +945,9 @@ function formatWeakMap(ctx, value, recurseTimes, keys) {
return output;
}

function formatCollectionIterator(preview, ctx, value, recurseTimes, keys) {
function formatCollectionIterator(ctx, value, recurseTimes, keys) {
const output = [];
for (const entry of preview(value)) {
for (const entry of previewEntries(value)) {
if (ctx.maxArrayLength === output.length) {
output.push('... more items');
break;
Expand All @@ -969,13 +961,11 @@ function formatCollectionIterator(preview, ctx, value, recurseTimes, keys) {
}

function formatMapIterator(ctx, value, recurseTimes, keys) {
return formatCollectionIterator(previewMapIterator, ctx, value, recurseTimes,
keys);
return formatCollectionIterator(ctx, value, recurseTimes, keys);
}

function formatSetIterator(ctx, value, recurseTimes, keys) {
return formatCollectionIterator(previewSetIterator, ctx, value, recurseTimes,
keys);
return formatCollectionIterator(ctx, value, recurseTimes, keys);
}

function formatPromise(ctx, value, recurseTimes, keys) {
Expand Down
1 change: 0 additions & 1 deletion node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@
'lib/internal/http2/core.js',
'lib/internal/http2/compat.js',
'lib/internal/http2/util.js',
'lib/internal/v8.js',
'lib/internal/v8_prof_polyfill.js',
'lib/internal/v8_prof_processor.js',
'lib/internal/validators.js',
Expand Down
6 changes: 0 additions & 6 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4355,12 +4355,6 @@ void Init(int* argc,
}
#endif

// Needed for access to V8 intrinsics. Disabled again during bootstrapping,
// see lib/internal/bootstrap/node.js.
const char allow_natives_syntax[] = "--allow_natives_syntax";
V8::SetFlagsFromString(allow_natives_syntax,
sizeof(allow_natives_syntax) - 1);

// We should set node_is_initialized here instead of in node::Start,
// otherwise embedders using node::Init to initialize everything will not be
// able to set it and native modules will not load for them.
Expand Down
30 changes: 30 additions & 0 deletions src/node_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,35 @@ static void GetProxyDetails(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(ret);
}

static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
if (!args[0]->IsObject())
return;

bool is_key_value;
Local<Array> entries;
if (!args[0].As<Object>()->PreviewEntries(&is_key_value).ToLocal(&entries))
return;
if (!is_key_value)
return args.GetReturnValue().Set(entries);

uint32_t length = entries->Length();
CHECK_EQ(length % 2, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Environment* env = Environment::GetCurrent(args);
Local<Context> context = env->context();

Local<Array> pairs = Array::New(env->isolate(), length / 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

for (uint32_t i = 0; i < length / 2; i++) {
Local<Array> pair = Array::New(env->isolate(), 2);
pair->Set(context, 0, entries->Get(context, i * 2).ToLocalChecked())
.FromJust();
pair->Set(context, 1, entries->Get(context, i * 2 + 1).ToLocalChecked())
.FromJust();
pairs->Set(context, i, pair).FromJust();
}
args.GetReturnValue().Set(pairs);
}

// Side effect-free stringification that will never throw exceptions.
static void SafeToString(const FunctionCallbackInfo<Value>& args) {
auto context = args.GetIsolate()->GetCurrentContext();
Expand Down Expand Up @@ -188,6 +217,7 @@ void Initialize(Local<Object> target,
env->SetMethod(target, "getPromiseDetails", GetPromiseDetails);
env->SetMethod(target, "getProxyDetails", GetProxyDetails);
env->SetMethod(target, "safeToString", SafeToString);
env->SetMethod(target, "previewEntries", PreviewEntries);

env->SetMethod(target, "startSigintWatchdog", StartSigintWatchdog);
env->SetMethod(target, "stopSigintWatchdog", StopSigintWatchdog);
Expand Down
8 changes: 3 additions & 5 deletions test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,13 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

// Flags: --expose_internals
'use strict';
const common = require('../common');
const assert = require('assert');
const JSStream = process.binding('js_stream').JSStream;
const util = require('util');
const vm = require('vm');
const { previewMapIterator } = require('internal/v8');
const { previewEntries } = process.binding('util');

assert.strictEqual(util.inspect(1), '1');
assert.strictEqual(util.inspect(false), 'false');
Expand Down Expand Up @@ -448,7 +447,7 @@ assert.strictEqual(util.inspect(-5e-324), '-5e-324');
{
const map = new Map();
map.set(1, 2);
const vals = previewMapIterator(map.entries());
const vals = previewEntries(map.entries());
const valsOutput = [];
for (const o of vals) {
valsOutput.push(o);
Expand Down Expand Up @@ -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 }');
Copy link
Member Author

@addaleax addaleax May 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Make sure the iterator doesn't get consumed.
const keys = aSet.keys();
assert.strictEqual(util.inspect(keys), '[Set Iterator] { 1, 3 }');
Expand Down