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: https://github.com/nodejs/node/pull/20831
Refs: https://github.com/nodejs/node/pull/20719#discussion_r189342513
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>

5 files changed, 67 insertions, 38 deletions

diff --git a/lib/console.js b/lib/console.js
index 65d7e81f15..92ac64173a 100644
--- a/lib/console.js
+++ b/lib/console.js
@@ -363,17 +363,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
@@ -386,9 +396,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;
@@ -407,7 +417,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
index afd3cae724..5413cc0a33 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -981,7 +981,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)
@@ -998,14 +998,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).
@@ -1017,9 +1019,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
index 724bb3603c..ef7dc8a818 100644
--- 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
index 9ff4641d65..844d2e01e4 100644
--- 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
index 1f05f4987e..f97ab95cde 100644
--- 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.