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.

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

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

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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: #20719 (comment)

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

@addaleax addaleax added util Issues and PRs related to the built-in util module. console Issues and PRs related to the console subsystem. labels May 18, 2018
@addaleax addaleax requested review from devsnek and BridgeAR May 18, 2018 23:44
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 18, 2018
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)
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?

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

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

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.


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

@BridgeAR BridgeAR May 18, 2018

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.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM in general. Some minor comments.

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

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

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

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

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

i couldn't find anything to nitpick so with the other comments handled this lgtm

@addaleax
Copy link
Member Author

@BridgeAR I wanted to wait for you to push changes you’d like to see before I continue with this PR … where are you at regarding that? Otherwise I’d probably address some of the newer comments here first

@BridgeAR
Copy link
Member

@addaleax I have my local change ready but it became somewhat more involved, so I decided to just open a PR on top of this one. I am going to rebase when this PR lands.

@BridgeAR
Copy link
Member

@addaleax did you have a look at my follow up PR? :-)

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 14, 2018
@BridgeAR
Copy link
Member

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jul 16, 2018
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]>
@BridgeAR
Copy link
Member

Landed in db49589.

I fixed the eslint errors while landing.

@BridgeAR BridgeAR closed this Jul 16, 2018
targos pushed a commit that referenced this pull request Jul 16, 2018
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]>
@targos targos mentioned this pull request Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants