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

buffer: inspect extra properties #25150

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 20, 2018

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
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. labels Dec 20, 2018
@mscdex
Copy link
Contributor

mscdex commented Dec 20, 2018

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.

@BridgeAR
Copy link
Member Author

@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).

@mscdex
Copy link
Contributor

mscdex commented Dec 20, 2018

@BridgeAR I updated my comment with the implementation I benchmarked with.

@BridgeAR
Copy link
Member Author

@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.

@mscdex
Copy link
Contributor

mscdex commented Dec 20, 2018

@BridgeAR How many properties are we talking about here?

@BridgeAR
Copy link
Member Author

@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.

@mscdex
Copy link
Contributor

mscdex commented Dec 20, 2018

@BridgeAR FWIW here is the complete benchmark code I used (comment out the case not being tested):

Benchmark code
const 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.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 20, 2018

@mscdex this function is used for array types (in this case Buffer / Uint8Array), not for regular objects.

@BridgeAR
Copy link
Member Author

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

@nodejs/buffer @nodejs/util PTAL

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 23, 2018
@Trott
Copy link
Member

Trott commented Dec 23, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19767/ ✔️

This makes sure extra properties on buffers are not ignored anymore
when inspecting the buffer.
@BridgeAR BridgeAR force-pushed the inspect-extra-buffer-properties branch from 60a5f3d to 1be4412 Compare December 24, 2018 10:12
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 24, 2018

Rebased due to conflicts.

CI https://ci.nodejs.org/job/node-test-pull-request/19787/ ✔️

@BridgeAR
Copy link
Member Author

@nodejs/buffer @nodejs/util PTAL.

This needs another review.

pull bot pushed a commit to shakir-abdo/node that referenced this pull request Dec 27, 2018
This makes sure extra properties on buffers are not ignored anymore
when inspecting the buffer.

PR-URL: nodejs#25150
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member Author

Landed in d385e2c 🎉

@addaleax
Copy link
Member

addaleax commented Jan 5, 2019

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.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 9, 2019
This makes sure extra properties on buffers are not ignored anymore
when inspecting the buffer.

PR-URL: nodejs#25150
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit that referenced this pull request Jan 10, 2019
This makes sure extra properties on buffers are not ignored anymore
when inspecting the buffer.

PR-URL: #25150
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
This makes sure extra properties on buffers are not ignored anymore
when inspecting the buffer.

PR-URL: #25150
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This makes sure extra properties on buffers are not ignored anymore
when inspecting the buffer.

PR-URL: nodejs#25150
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
This makes sure extra properties on buffers are not ignored anymore
when inspecting the buffer.

PR-URL: nodejs#25150
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
@BridgeAR BridgeAR deleted the inspect-extra-buffer-properties branch January 20, 2020 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants