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.

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: improve typed array formatting #3793

Merged
merged 1 commit into from Nov 14, 2015

Conversation

Projects
None yet
3 participants
@bnoordhuis
Copy link
Member

commented Nov 12, 2015

Pretty-print typed arrays like regular arrays. Speeds up formatting by
almost 300% because it no longer stringifies the array indices.

Pretty-print ArrayBuffer and DataView as well by including byteLength,
byteOffset and buffer properties in the stringified representation.

Taggging this semver-major for now. Maybe overcautious?

CI: https://ci.nodejs.org/job/node-test-pull-request/711/

@@ -314,6 +315,28 @@ function formatValue(ctx, value, recurseTimes) {
keys.unshift('size');
empty = value.size === 0;
formatter = formatMap;
} else if (value instanceof ArrayBuffer) {

This comment has been minimized.

Copy link
@trevnorris

trevnorris Nov 12, 2015

Contributor

Object.keys((new ArrayBuffer(5))).length === 0. Should this be placed in the shortcut path above?

This comment has been minimized.

Copy link
@trevnorris

trevnorris Nov 12, 2015

Contributor

nm. overlooked the other keys additions.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Nov 12, 2015

Contributor

hah. so I wasn't loosing it:

Object.keys(new ArrayBuffer(5)).length === 0;
Object.getOwnPropertyNames(new ArrayBuffer(5)).length === 0;
Object.getOwnPropertySymbols(new ArrayBuffer(5)) === 0;

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Nov 13, 2015

Author Member

I put it here because it adds a key for .byteLength but I can probably move it up and call formatProperty() directly, likewise for DataView. Good idea / bad idea?

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Nov 13, 2015

Author Member

Added a fast path for ArrayBuffer, PTAL.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Nov 13, 2015

Contributor

I like it.

@@ -626,6 +666,19 @@ function reduceToSingleString(output, base, braces) {
}


function isTypedArray(value) {
return value instanceof Float32Array ||

This comment has been minimized.

Copy link
@evanlucas

evanlucas Nov 12, 2015

Member

I wonder, would V8::Value::IsTypedArray be faster here?

This comment has been minimized.

Copy link
@evanlucas

evanlucas Nov 12, 2015

Member

nope, not for a buffer anyways...nevermind me

@trevnorris

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2015

LGTM

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2015

util: improve typed array formatting
Pretty-print typed arrays like regular arrays.  Speeds up formatting by
almost 300% because it no longer stringifies the array indices.

Pretty-print ArrayBuffer and DataView as well by including byteLength,
byteOffset and buffer properties in the stringified representation.

PR-URL: #3793
Reviewed-By: Rod Vagg <rod@vagg.org>

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:pretty-typed-arrays branch to 6cf2cff Nov 14, 2015

@bnoordhuis bnoordhuis closed this Nov 14, 2015

@bnoordhuis bnoordhuis deleted the bnoordhuis:pretty-typed-arrays branch Nov 14, 2015

@bnoordhuis bnoordhuis merged commit 6cf2cff into nodejs:master Nov 14, 2015

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Nov 14, 2015

util: improve typed array formatting
Pretty-print typed arrays like regular arrays.  Speeds up formatting by
almost 300% because it no longer stringifies the array indices.

Pretty-print ArrayBuffer and DataView as well by including byteLength,
byteOffset and buffer properties in the stringified representation.

PR-URL: nodejs#3793
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2015

Thanks for review, landed in 34a3591. Apologies, I initially landed it with the wrong Reviewed-By tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.