| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2017/pipeline |
Sorry, something went wrong.
|
FWIW getOwnNonIndexProperties() can be replaced with a JS variant that is much faster, at least for the ONLY_ENUMERABLE case if I'm understanding the function's behavior correctly. Here's what I came up with for the JS implementation: function isAllDigits(s) {
if (s.length === 0)
return false;
for (var i = 0; i < s.length; ++i) {
const code = s.charCodeAt(i);
if (code < 48 || code > 57)
return false;
}
return true;
}
function getOwnNonIndexProperties(obj, filter) {
const props = [];
var p = 0;
if (filter === ONLY_ENUMERABLE) {
const keys = Object.keys(obj);
for (var i = 0; i < keys.length; ++i) {
const key = keys[i];
if (!isAllDigits(key))
props[p++] = key;
}
} else {
// TODO or defer to C++ implementation
}
return props;
}It's at least twice as fast for a variety of objects using node master. |
Sorry, something went wrong.
|
@mscdex how would that look like? I can't think of any way to access the keys like that in JS. The functions returns the keys of an object that are not numbers (keys are sorted by spec. First the numbers, then strings, then symbols). |
Sorry, something went wrong.
|
@BridgeAR I updated my comment with the implementation I benchmarked with. |
Sorry, something went wrong.
|
@mscdex that will be very slow for objects with lots of properties. The reason is that the array with all keys has to be allocated first and that would be very expensive. |
Sorry, something went wrong.
|
@BridgeAR How many properties are we talking about here? |
Sorry, something went wrong.
|
@mscdex in my direct comparison I can't get the JS function to be faster under any circumstances. The C++ function has a constant time while the JS function is over linear and even for small buffers, the C++ function wins when running it multiple times. |
Sorry, something went wrong.
|
@BridgeAR FWIW here is the complete benchmark code I used (comment out the case not being tested): Benchmark codeconst n = 1e6;
const vals = [
{ foo: 'bar', baz: 5, quux: true },
{ '1': 'bar', '2': 5, '3': 5.1 },
{ '-1': 'bar', '2': 5, '3a': 40000 },
{ '': () => {}, '1234567890-0987654321': false, quux: false }
];
const {
propertyFilter: {
ALL_PROPERTIES,
ONLY_ENUMERABLE
}
} = process.binding('util');
const filter = ONLY_ENUMERABLE;
var name;
// Case #1
//~ const {
//~ getOwnNonIndexProperties,
//~ } = process.binding('util');
//~ name = 'C++';
// Case #2
function isAllDigits(s) {
if (s.length === 0)
return false;
for (var i = 0; i < s.length; ++i) {
const code = s.charCodeAt(i);
if (code < 48 || code > 57)
return false;
}
return true;
}
function getOwnNonIndexProperties(obj, filter) {
const props = [];
var p = 0;
if (filter === ONLY_ENUMERABLE) {
const keys = Object.keys(obj);
for (var i = 0; i < keys.length; ++i) {
const key = keys[i];
if (!isAllDigits(key))
props[p++] = key;
}
} else {
}
return props;
}
name = 'JS';
// Run the benchmark
console.time(name);
for (var i = 0; i < n; ++i) {
for (var j = 0; j < vals.length; ++j)
getOwnNonIndexProperties(vals[j], filter);
}
console.timeEnd(name);Running this I get ~1100ms for C++ and ~510ms for JS on node master. EDIT: For mostly numeric keys the C++ implementation does seem to be a little faster, but not by a whole lot (~50ms), when using objects with the same number of properties as in the cases provided above. |
Sorry, something went wrong.
|
@mscdex this function is used for array types (in this case Buffer / Uint8Array), not for regular objects. |
Sorry, something went wrong.
|
CI https://ci.nodejs.org/job/node-test-pull-request/19763/ @nodejs/buffer @nodejs/util PTAL |
Sorry, something went wrong.
|
Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19767/ ✔️ |
Sorry, something went wrong.
This makes sure extra properties on buffers are not ignored anymore when inspecting the buffer.
|
Rebased due to conflicts. CI https://ci.nodejs.org/job/node-test-pull-request/19787/ ✔️ |
Sorry, something went wrong.
|
@nodejs/buffer @nodejs/util PTAL. This needs another review. |
Sorry, something went wrong.
This makes sure extra properties on buffers are not ignored anymore when inspecting the buffer. PR-URL: nodejs#25150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Should this be backported to v11.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label. |
Sorry, something went wrong.
This makes sure extra properties on buffers are not ignored anymore when inspecting the buffer. PR-URL: nodejs#25150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
This makes sure extra properties on buffers are not ignored anymore when inspecting the buffer. PR-URL: #25150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
This makes sure extra properties on buffers are not ignored anymore when inspecting the buffer. PR-URL: #25150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
This makes sure extra properties on buffers are not ignored anymore when inspecting the buffer. PR-URL: nodejs#25150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
This makes sure extra properties on buffers are not ignored anymore when inspecting the buffer. PR-URL: nodejs#25150 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
| Back | FazBrowse Home | New Git URL |
This makes sure extra properties on buffers are not ignored anymore
when inspecting the buffer.
I am not sure about this either being a patch or semver-major. We mainly consider changes to util.inspect as patch, so I guess this could be considered a similar thing.
The implementation uses some internal knowledge and it would require more overhead otherwise but this seemed the most straight forward way to do this.
Checklist