Skip to content

Commit

Permalink
console,util: avoid pair array generation in C++
Browse files Browse the repository at this point in the history 
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 <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
@addaleax @targos
addaleax authored and targos committed Jul 16, 2018
1 parent 4e60ce8 commit 73cafd8
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 38 deletions.
28 changes: 19 additions & 9 deletions lib/console.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,17 +356,27 @@ Console.prototype.table = function(tabularData, properties) {
const getIndexArray = (length) => ArrayFrom({ length }, (_, i) => inspect(i));

const mapIter = isMapIterator(tabularData);
let isKeyValue = false;
let i = 0;
if (mapIter)
tabularData = previewEntries(tabularData);
[ tabularData, isKeyValue ] = previewEntries(tabularData);

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 (; 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 @@ -379,9 +389,9 @@ Console.prototype.table = function(tabularData, properties) {

const setIter = isSetIterator(tabularData);
if (setIter)
tabularData = previewEntries(tabularData);
[ tabularData ] = previewEntries(tabularData);

const setlike = setIter || isSet(tabularData);
const setlike = setIter || (mapIter && !isKeyValue) || isSet(tabularData);
if (setlike) {
const values = [];
let length = 0;
Expand All @@ -400,7 +410,7 @@ Console.prototype.table = function(tabularData, properties) {
const valuesKeyArray = [];
const indexKeyArray = ObjectKeys(tabularData);

for (var i = 0; i < indexKeyArray.length; i++) {
for (; i < indexKeyArray.length; i++) {
const item = tabularData[indexKeyArray[i]];
const primitive = item === null ||
(typeof item !== 'function' && typeof item !== 'object');
Expand Down
28 changes: 20 additions & 8 deletions lib/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -984,7 +984,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 @@ -1001,14 +1001,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);
// 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 @@ -1020,9 +1022,19 @@ function formatWeakMap(ctx, value, recurseTimes, keys) {
return output;
}

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

function formatCollectionIterator(ctx, value, recurseTimes, keys) {
const output = [];
for (const entry of previewEntries(value)) {
var [ 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
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);
}

// 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
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,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
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());
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

0 comments on commit 73cafd8

Please sign in to comment.