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

util: simplify constructor retrieval in inspect() #36466

Merged
merged 1 commit into from Dec 14, 2020
Merged

util: simplify constructor retrieval in inspect() #36466

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 3 additions & 44 deletions lib/internal/util/inspect.js
View file
Open in desktop
Expand Up @@ -3,21 +3,14 @@
const {
Array,
ArrayIsArray,
BigInt64Array,
BigIntPrototypeValueOf,
BigUint64Array,
BooleanPrototypeValueOf,
DatePrototypeGetTime,
DatePrototypeToISOString,
DatePrototypeToString,
ErrorPrototypeToString,
Float32Array,
Float64Array,
FunctionPrototypeCall,
FunctionPrototypeToString,
Int8Array,
Int16Array,
Int32Array,
JSONStringify,
Map,
MapPrototypeGetSize,
Expand Down Expand Up @@ -59,10 +52,8 @@ const {
SymbolIterator,
SymbolToStringTag,
TypedArrayPrototypeGetLength,
Uint16Array,
Uint32Array,
TypedArrayPrototypeGetSymbolToStringTag,
Uint8Array,
Uint8ClampedArray,
uncurryThis,
} = primordials;

Expand Down Expand Up @@ -120,17 +111,6 @@ const {
isNumberObject,
isBooleanObject,
isBigIntObject,
isUint8Array,
isUint8ClampedArray,
isUint16Array,
isUint32Array,
isInt8Array,
isInt16Array,
isInt32Array,
isFloat32Array,
isFloat64Array,
isBigInt64Array,
isBigUint64Array
} = require('internal/util/types');

const assert = require('internal/assert');
Expand Down Expand Up @@ -712,26 +692,6 @@ function formatProxy(ctx, proxy, recurseTimes) {
ctx, res, '', ['Proxy [', ']'], kArrayExtrasType, recurseTimes);
}

function findTypedConstructor(value) {
for (const [check, clazz] of [
[isUint8Array, Uint8Array],
[isUint8ClampedArray, Uint8ClampedArray],
[isUint16Array, Uint16Array],
[isUint32Array, Uint32Array],
[isInt8Array, Int8Array],
[isInt16Array, Int16Array],
[isInt32Array, Int32Array],
[isFloat32Array, Float32Array],
[isFloat64Array, Float64Array],
[isBigInt64Array, BigInt64Array],
[isBigUint64Array, BigUint64Array]
]) {
if (check(value)) {
return clazz;
}
}
}

// Note: using `formatValue` directly requires the indentation level to be
// corrected by setting `ctx.indentationLvL += diff` and then to decrease the
// value afterwards again.
Expand Down Expand Up @@ -879,10 +839,9 @@ function formatRaw(ctx, value, recurseTimes, typedArray) {
let bound = value;
let fallback = '';
if (constructor === null) {
const constr = findTypedConstructor(value);
fallback = constr.name;
fallback = TypedArrayPrototypeGetSymbolToStringTag(value);
Comment on lines 840 to +842
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably use:

Suggested change
let fallback = '';
if (constructor === null) {
const constr = findTypedConstructor(value);
fallback = constr.name;
fallback = TypedArrayPrototypeGetSymbolToStringTag(value);
const fallback = TypedArrayPrototypeGetSymbolToStringTag(value);
if (constructor === null) {

So that fallback is always non‑empty.

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 think the idea originally was to avoid the overhead of a function call where possible. Judging from our test coverage stats, constructor being null is the unusual case (616 times hitting the if but only 33 times evaluating to true).

The performance difference seems likely to be unmeasurable and the code simplification may very well be worth it. But I'd prefer that be handled in a separate pull request. I'd prefer that this pull request preserve the code shape and merely replace/eliminate the existing function.

This is reminding me to run benchmarks for this change.

Copy link
Contributor

@ExE-Boss ExE-Boss Dec 10, 2020 •
edited

Choose a reason for hiding this comment

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

Well, TypedArrayPrototypeGetSymbolToStringTag is already invoked by isTypedArray:

node/lib/internal/util/types.js

Lines 8 to 10 in 72b07e4

function isTypedArray(value) {
return TypedArrayPrototypeGetSymbolToStringTag(value) !== undefined;
}

It should be possible to reduce this to always perform only one TypedArrayPrototypeGetSymbolToStringTag call, even in the constructor === null case, but that is probably overkill:

   if (value[SymbolIterator] || constructor === null) {
     noIterator = false;
+    let typedArrayName;
     if (ArrayIsArray(value)) {
-    } else if (isTypedArray(name)) {
+    } else if ((typedArrayName = TypedArrayPrototypeGetSymbolToStringTag(value)) !== undefined) {
       keys = getOwnNonIndexProperties(value, filter);
       let bound = value;
-      let fallback = '';
       if (constructor === null) {
-        fallback = TypedArrayPrototypeGetSymbolToStringTag(value);
         // Reconstruct the array information.
-        bound = new primordials[primordials](value);
+        bound = new primordials[typedArrayName](value);
       }
       const size = TypedArrayPrototypeGetLength(value);
-      const prefix = getPrefix(constructor, tag, fallback, `(${size})`);
+      const prefix = getPrefix(constructor, tag, typedArrayName, `(${size})`);

// Reconstruct the array information.
bound = new constr(value);
bound = new primordials[fallback](value);
}
const size = TypedArrayPrototypeGetLength(value);
const prefix = getPrefix(constructor, tag, fallback, `(${size})`);
Expand Down