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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 17 additions & 8 deletions lib/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,17 +344,26 @@ Console.prototype.table = function(tabularData, properties) {
const getIndexArray = (length) => ArrayFrom({ length }, (_, i) => inspect(i));

const mapIter = isMapIterator(tabularData);
let isKeyValue = false;
if (mapIter)
tabularData = previewEntries(tabularData);
[ tabularData, isKeyValue ] = previewEntries(tabularData);
Copy link
Member

Choose a reason for hiding this comment

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

The syntax makes me go 😬

Let's make it

if (mapIter) {
  ;([tabularData, isKeyValue] = previewEntries(tabularData));
}

Copy link
Member

@devsnek devsnek May 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

that leading semi isn't really node style, but i agree to brackets + wrapping in parens


if (mapIter || isMap(tabularData)) {
if (isKeyValue || isMap(tabularData)) {
const keys = [];
const values = [];
let length = 0;
for (const [k, v] of tabularData) {
keys.push(inspect(k));
values.push(inspect(v));
length++;
if (mapIter) {
for (let i = 0; i < tabularData.length / 2; ++i) {
keys.push(inspect(tabularData[i * 2]));
values.push(inspect(tabularData[i * 2 + 1]));
length++;
}
} else {
for (const [k, v] of tabularData) {
keys.push(inspect(k));
values.push(inspect(v));
length++;
}
}
return final([
iterKey, keyKey, valuesKey
Expand All @@ -367,9 +376,9 @@ Console.prototype.table = function(tabularData, properties) {

const setIter = isSetIterator(tabularData);
if (setIter)
tabularData = previewEntries(tabularData);
[ tabularData ] = previewEntries(tabularData);
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 make it previewEntries(tabularData)[0] instead?


const setlike = setIter || isSet(tabularData);
const setlike = setIter || (mapIter && !isKeyValue) || isSet(tabularData);
Copy link
Member

@BridgeAR BridgeAR May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I am not a fan of this solution. What we should do is to detect the actual type of the tabularData before ever overriding that variable. I would just use a second variable to make this easier. As in:

let data = tabularData;

if (mapIter) {
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not sure I understand this comment… Second argument to which function exactly?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, second variable (not argument).

The reason for the issue is that the type can not be properly detected anymore after overriding the original input. So we need a reference to the original input that we never override and one that could potentially be changed. That way the implementation should be easier. I guess I should not have accepted the PR to be landed originally :/

If you want, I can take a stab at this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, we did override tabularData before too, so this isn’t really a step back on that (if that’s the variable you’re referring to).

Tbh, I’m still not really getting what you’re trying to say, but it might just be easier to talk about concrete code, yes. :)

That way the implementation should be easier. I guess I should not have accepted the PR to be landed originally :/

Are you talking about #20719 or the one that introduced the table code in the first place? The former one did not make any changes to this code that go beyond trivial…

Copy link
Member

Choose a reason for hiding this comment

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

I mean the original table code in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

I am to tired tonight but I am going to have a look at it tomorrow. Is it fine to just push to your branch directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, feel free to do that. :)

Copy link
Member

Choose a reason for hiding this comment

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

Great :)

Copy link
Member

Choose a reason for hiding this comment

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

After looking into this closer it does indeed require some deeper changes for what I thought about.

if (setlike) {
const values = [];
let length = 0;
Expand Down
28 changes: 20 additions & 8 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ function formatMap(ctx, value, recurseTimes, keys) {

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

function formatWeakMap(ctx, value, recurseTimes, keys) {
const maxArrayLength = Math.max(ctx.maxArrayLength, 0);
const entries = previewEntries(value).slice(0, maxArrayLength + 1);
const remainder = entries.length > maxArrayLength;
const len = entries.length - (remainder ? 1 : 0);
const [ entries ] = previewEntries(value).slice(0, (maxArrayLength + 1) * 2);
Copy link
Member

Choose a reason for hiding this comment

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

nit: we don't usually put spaces inside of []. [entries] is fine.

Copy link
Member

Choose a reason for hiding this comment

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

The slice is actually unnecessary overhead. The limitation is done below and this has no effect (there are just two entries returned).

// Entries exist as [key1, val1, key2, val2, ...]
const remainder = entries.length / 2 > maxArrayLength;
const len = entries.length / 2 - (remainder ? 1 : 0);
const maxLength = Math.min(maxArrayLength, len);
let output = new Array(maxLength);
for (var i = 0; i < len; i++) {
output[i] = `${formatValue(ctx, entries[i][0], recurseTimes)} => ` +
formatValue(ctx, entries[i][1], recurseTimes);
for (var i = 0; i < maxLength; i++) {
const pos = i * 2;
output[i] = `${formatValue(ctx, entries[pos], recurseTimes)} => ` +
formatValue(ctx, entries[pos + 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 @@ -942,9 +944,19 @@ function formatWeakMap(ctx, value, recurseTimes, keys) {
return output;
}

function zip2(list) {
const ret = Array(list.length / 2);
for (let i = 0; i < ret.length; ++i)
ret[i] = [list[2 * i], list[2 * i + 1]];
return ret;
}
Copy link
Member

Choose a reason for hiding this comment

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

We could use the same pattern as in weak maps instead of first zipping this. This step is just overhead (even though the result is nicer to read).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it gets pretty tricky to do that, at least for me… we are passing the individual pair arrays to formatValue, and it’s not obvious how/whether to short-circuit around that

(Extreme edge case: Calling formatValue() on the individual keys/values rather than the pairs even makes a difference in depth…)

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure in what way this should have any influence on the depth?


function formatCollectionIterator(ctx, value, recurseTimes, keys) {
const output = [];
for (const entry of previewEntries(value)) {
let [ entries, isKeyValue ] = previewEntries(value);
if (isKeyValue)
entries = zip2(entries);
for (const entry of entries) {
if (ctx.maxArrayLength === output.length) {
output.push('... more items');
break;
Expand Down
25 changes: 6 additions & 19 deletions src/node_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,29 +53,16 @@ static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
if (!args[0]->IsObject())
return;

Environment* env = Environment::GetCurrent(args);
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);

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

Local<Array> pairs = Array::New(env->isolate(), length / 2);
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);
Local<Array> ret = Array::New(env->isolate(), 2);
ret->Set(env->context(), 0, entries).FromJust();
ret->Set(env->context(), 1, v8::Boolean::New(env->isolate(), is_key_value))
.FromJust();
return args.GetReturnValue().Set(ret);
Copy link
Member

Choose a reason for hiding this comment

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

We do not need the information about the key value pairs as we already have that when calling this function. The JS side has to be adjusted to the way it worked before to accomplish that.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not need the information about the key value pairs as we already have that when calling this function.

Not for Map iterators, and I’m not aware of how we could know that in advance.

Copy link
Member

Choose a reason for hiding this comment

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

The way this was done before in JS allowed this. We just pretty much have to revert the JS changes from the former PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@BridgeAR Not really? When we had the V8 builtins available, we could only tell them apart because some Map iterators gave us pairs and others didn’t, but if we want to avoid pair creation, then we need this.

Concrete example: This is necessary to distinguish new Map([[1,2],[3,4]]).entries() from new Map([['a',1],['b',2],['c',3],['d',4]]).values()

Copy link
Member

@BridgeAR BridgeAR May 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! I did not think about new Map().entries() anymore. However, this is only necessary in case of map iterators. So we could tell C++ if we want the return value to be wrapped in a extra array for the map iterators and just directly return the array for all set iterators.

Copy link
Member

@BridgeAR BridgeAR May 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, I just checked https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/entries and now I remember why the return value for entries with a set iterator was actually so weird. That was indeed by design. I am a bit on the edge to follow that or not... As far as I know, we can not detect that anymore, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I know, we can not detect that anymore, right?

Yes. If it helps, our behaviour is aligned with Chromium’s now (for that reason), and I think that either can be seen as valid.

}

// Side effect-free stringification that will never throw exceptions.
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-console-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,26 @@ test(new Map([[1, 1], [2, 2], [3, 3]]).entries(), `
└───────────────────┴─────┴────────┘
`);

test(new Map([[1, 1], [2, 2], [3, 3]]).values(), `
┌───────────────────┬────────┐
│ (iteration index) │ Values │
├───────────────────┼────────┤
│ 0 │ 1 │
│ 1 │ 2 │
│ 2 │ 3 │
└───────────────────┴────────┘
`);

test(new Map([[1, 1], [2, 2], [3, 3]]).keys(), `
┌───────────────────┬────────┐
│ (iteration index) │ Values │
├───────────────────┼────────┤
│ 0 │ 1 │
│ 1 │ 2 │
│ 2 │ 3 │
└───────────────────┴────────┘
`);

test(new Set([1, 2, 3]).values(), `
┌───────────────────┬────────┐
│ (iteration index) │ Values │
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,13 +447,13 @@ assert.strictEqual(util.inspect(-5e-324), '-5e-324');
{
const map = new Map();
map.set(1, 2);
const vals = previewEntries(map.entries());
const [ vals ] = previewEntries(map.entries());
Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea for us to test isKeyValue as well.

const valsOutput = [];
for (const o of vals) {
valsOutput.push(o);
}

assert.strictEqual(util.inspect(valsOutput), '[ [ 1, 2 ] ]');
assert.strictEqual(util.inspect(valsOutput), '[ 1, 2 ]');
}

// Test for other constructors in different context.
Expand Down