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

Merged
merged 1 commit into from
Nov 14, 2015
Merged

util: improve typed array formatting #3793

Show file tree
Hide file tree
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 68 additions & 9 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,10 +284,17 @@ function formatValue(ctx, value, recurseTimes) {
formatted = formatPrimitiveNoColor(ctx, raw);
return ctx.stylize('[Boolean: ' + formatted + ']', 'boolean');
}
// Fast path for ArrayBuffer. Can't do the same for DataView because it
// has a non-primitive .buffer property that we need to recurse for.
if (value instanceof ArrayBuffer) {
return `${getConstructorOf(value).name}` +
` { byteLength: ${formatNumber(ctx, value.byteLength)} }`;
}
}

var constructor = getConstructorOf(value);
var base = '', empty = false, braces, formatter;
var base = '', empty = false, braces;
var formatter = formatObject;

if (Array.isArray(value)) {
// We can't use `constructor === Array` because this could
Expand All @@ -314,6 +321,28 @@ function formatValue(ctx, value, recurseTimes) {
keys.unshift('size');
empty = value.size === 0;
formatter = formatMap;
} else if (value instanceof ArrayBuffer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

nm. overlooked the other keys additions.

Copy link
Contributor

Choose a reason for hiding this comment

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

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;

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 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a fast path for ArrayBuffer, PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it.

braces = ['{', '}'];
keys.unshift('byteLength');
visibleKeys.byteLength = true;
} else if (value instanceof DataView) {
braces = ['{', '}'];
// .buffer goes last, it's not a primitive like the others.
keys.unshift('byteLength', 'byteOffset', 'buffer');
visibleKeys.byteLength = true;
visibleKeys.byteOffset = true;
visibleKeys.buffer = true;
} else if (isTypedArray(value)) {
braces = ['[', ']'];
formatter = formatTypedArray;
if (ctx.showHidden) {
// .buffer goes last, it's not a primitive like the others.
keys.unshift('BYTES_PER_ELEMENT',
'length',
'byteLength',
'byteOffset',
'buffer');
}
} else {
// Only create a mirror if the object superficially looks like a Promise.
var promiseInternals = value instanceof Promise && inspectPromise(value);
Expand All @@ -336,7 +365,6 @@ function formatValue(ctx, value, recurseTimes) {
constructor = null;
braces = ['{', '}'];
empty = true; // No other data than keys.
formatter = formatObject;
}
}
}
Expand Down Expand Up @@ -408,6 +436,15 @@ function formatValue(ctx, value, recurseTimes) {
}


function formatNumber(ctx, value) {
// Format -0 as '-0'. Strict equality won't distinguish 0 from -0,
// so instead we use the fact that 1 / -0 < 0 whereas 1 / 0 > 0 .
if (value === 0 && 1 / value < 0)
return ctx.stylize('-0', 'number');
return ctx.stylize('' + value, 'number');
}


function formatPrimitive(ctx, value) {
if (value === undefined)
return ctx.stylize('undefined', 'undefined');
Expand All @@ -424,13 +461,8 @@ function formatPrimitive(ctx, value) {
.replace(/\\"/g, '"') + '\'';
return ctx.stylize(simple, 'string');
}
if (type === 'number') {
// Format -0 as '-0'. Strict equality won't distinguish 0 from -0,
// so instead we use the fact that 1 / -0 < 0 whereas 1 / 0 > 0 .
if (value === 0 && 1 / value < 0)
return ctx.stylize('-0', 'number');
return ctx.stylize('' + value, 'number');
}
if (type === 'number')
return formatNumber(ctx, value);
if (type === 'boolean')
return ctx.stylize('' + value, 'boolean');
// es6 symbol primitive
Expand Down Expand Up @@ -480,6 +512,20 @@ function formatArray(ctx, value, recurseTimes, visibleKeys, keys) {
}


function formatTypedArray(ctx, value, recurseTimes, visibleKeys, keys) {
var output = new Array(value.length);
for (var i = 0, l = value.length; i < l; ++i)
output[i] = formatNumber(ctx, value[i]);
for (const key of keys) {
if (typeof key === 'symbol' || !key.match(/^\d+$/)) {
output.push(
formatProperty(ctx, value, recurseTimes, visibleKeys, key, true));
}
}
return output;
}


function formatSet(ctx, value, recurseTimes, visibleKeys, keys) {
var output = [];
value.forEach(function(v) {
Expand Down Expand Up @@ -626,6 +672,19 @@ function reduceToSingleString(output, base, braces) {
}


function isTypedArray(value) {
return value instanceof Float32Array ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

value instanceof Float64Array ||
value instanceof Int16Array ||
value instanceof Int32Array ||
value instanceof Int8Array ||
value instanceof Uint16Array ||
value instanceof Uint32Array ||
value instanceof Uint8Array ||
value instanceof Uint8ClampedArray;
}


// NOTE: These type checking functions intentionally don't use `instanceof`
// because it is fragile and can be easily faked with `Object.create()`.
exports.isArray = Array.isArray;
Expand Down
53 changes: 53 additions & 0 deletions test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,59 @@ assert.equal(util.inspect(Object.create({},
'{ visible: 1 }'
);

for (const showHidden of [true, false]) {
const ab = new ArrayBuffer(4);
const dv = new DataView(ab, 1, 2);
assert.equal(util.inspect(ab, showHidden), 'ArrayBuffer { byteLength: 4 }');
assert.equal(util.inspect(new DataView(ab, 1, 2), showHidden),
'DataView {\n' +
' byteLength: 2,\n' +
' byteOffset: 1,\n' +
' buffer: ArrayBuffer { byteLength: 4 } }');
assert.equal(util.inspect(ab, showHidden), 'ArrayBuffer { byteLength: 4 }');
assert.equal(util.inspect(dv, showHidden),
'DataView {\n' +
' byteLength: 2,\n' +
' byteOffset: 1,\n' +
' buffer: ArrayBuffer { byteLength: 4 } }');
ab.x = 42;
dv.y = 1337;
assert.equal(util.inspect(ab, showHidden),
'ArrayBuffer { byteLength: 4, x: 42 }');
assert.equal(util.inspect(dv, showHidden),
'DataView {\n' +
' byteLength: 2,\n' +
' byteOffset: 1,\n' +
' buffer: ArrayBuffer { byteLength: 4, x: 42 },\n' +
' y: 1337 }');
}

[ Float32Array,
Float64Array,
Int16Array,
Int32Array,
Int8Array,
Uint16Array,
Uint32Array,
Uint8Array,
Uint8ClampedArray ].forEach(constructor => {
const length = 2;
const byteLength = length * constructor.BYTES_PER_ELEMENT;
const array = new constructor(new ArrayBuffer(byteLength), 0, length);
array[0] = 65;
array[1] = 97;
assert.equal(util.inspect(array, true),
`${constructor.name} [\n` +
` 65,\n` +
` 97,\n` +
` [BYTES_PER_ELEMENT]: ${constructor.BYTES_PER_ELEMENT},\n` +
` [length]: ${length},\n` +
` [byteLength]: ${byteLength},\n` +
` [byteOffset]: 0,\n` +
` [buffer]: ArrayBuffer { byteLength: ${byteLength} } ]`);
assert.equal(util.inspect(array, false), `${constructor.name} [ 65, 97 ]`);
});

// Due to the hash seed randomization it's not deterministic the order that
// the following ways this hash is displayed.
// See http://codereview.chromium.org/9124004/
Expand Down