-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
util: simplify constructor retrieval in inspect() #36466
Conversation
let fallback = ''; | ||
if (constructor === null) { | ||
const constr = findTypedConstructor(value); | ||
fallback = constr.name; | ||
fallback = TypedArrayPrototypeGetSymbolToStringTag(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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})`);
Benchmark results seem OK:
|
Instead of looping through a list of typed arrays, use TypedArrayPrototypeGetSymbolToStringTag. PR-URL: nodejs#36466 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
9ab439e
to
f43d1ca
Compare
Landed in f43d1ca |
Instead of looping through a list of typed arrays, use TypedArrayPrototypeGetSymbolToStringTag. PR-URL: #36466 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
Instead of looping through a list of typed arrays, use TypedArrayPrototypeGetSymbolToStringTag. PR-URL: #36466 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
Instead of looping through a list of typed arrays, use TypedArrayPrototypeGetSymbolToStringTag. PR-URL: #36466 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
Instead of looping through a list of typed arrays, use
TypedArrayPrototypeGetSymbolToStringTag.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes