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

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>
nodejs · May 18, 2018 · 73cafd853cc338a34c8dcfe043feb37ec9d32b98 · 73cafd8
1 parent 4e60ce8
commit 73cafd853cc338a34c8dcfe043feb37ec9d32b98
Unified Split

Showing with 67 additions and 38 deletions.

+19 −9 lib/console.js

+20 −8 lib/util.js

+6 −19 src/node_util.cc

+20 −0 test/parallel/test-console-table.js

+2 −2 test/parallel/test-util-inspect.js
diff --git a/lib/console.js b/lib/console.js
@@ -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
@@ -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;
@@ -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');

diff --git a/lib/util.js b/lib/util.js
@@ -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)
@@ -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).
@@ -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;

diff --git a/src/node_util.cc b/src/node_util.cc
@@ -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.

diff --git a/test/parallel/test-console-table.js b/test/parallel/test-console-table.js
@@ -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 │

diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
@@ -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.