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

perf: improving performance #528

Closed
wants to merge 3 commits into from
Closed

perf: improving performance #528

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca185a0
perf: improving perf of parsing options
H4ad Mar 31, 2023
8a20c28
perf: faster memoOpts for range
H4ad Mar 31, 2023
665eee4
perf: freeze parse options to avoid object allocation
H4ad Mar 31, 2023
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
35 changes: 32 additions & 3 deletions classes/range.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class Range {
this.set = range
.split('||')
// map the range to a 2d array of comparators
.map(r => this.parseRange(r.trim()))
.map(r => this.parseRange(r))
// throw out any comparator lists that are empty
// this generally means that it was not a valid range, which is allowed
// in loose mode, but will still throw if the WHOLE range is invalid.
Expand Down Expand Up @@ -81,8 +81,8 @@ class Range {

// memoize range parsing for performance.
// this is a very hot path, and fully deterministic.
const memoOpts = Object.keys(this.options).join(',')
const memoKey = `parseRange:${memoOpts}:${range}`
const memoOpts = buildMemoKeyFromOptions(this.options)
const memoKey = memoOpts + range
const cached = cache.get(memoKey)
if (cached) {
return cached
Expand Down Expand Up @@ -190,6 +190,35 @@ class Range {
return false
}
}

function buildMemoKeyFromOptions(options) {
Copy link

Choose a reason for hiding this comment

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

You may also want to try a handwritten key such as:

`${options.includePrerelease ? 1 : 0},${options.loose ? 1 : 0}...

Which is what we do in the TypeScript compiler. Not sure if it's faster than here but could be interesting to test.

Copy link
Contributor Author

@H4ad H4ad Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't hurt the performance:

buildMemoKeyFromOptions({"includePrelease":true}) x 1,121,468,719 ops/sec ±0.63% (96 runs sampled)
buildMemoKeyFromOptions({"includePrelease":true,"loose":true}) x 1,132,354,133 ops/sec ±0.67% (95 runs sampled)
buildMemoKeyFromOptions({"includePrelease":true,"loose":true,"rtl":true}) x 1,122,788,698 ops/sec ±0.53% (96 runs sampled)

After 1B ops/s, it almost has no effect choosing one or another.
The only drawback is increasing the lru-cache key by 6 characters instead of having it as 1.

If NPM team prefer readability over a tiny cache key, I can change the implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a good use case for a bit flag. It's a bunch of booleans, and bitwise operations are super fast. And while bit ops are often frowned upon as being dense or obscure, if organized properly, they're much more maintainable than a series of if/else statements and hoping that you captured each case.

let s = 1
const FLAG_includePrerelease = s
s<<=1
const FLAG_loose = s
s<<=1
const FLAG_rtl = s
s<<=1
// ...

function buildMemoKeyFromOptions(options) {
  // now just bitwise-OR them all together as appropriate
  return (
    options.loose ? FLAG_loose : 0
  ) | (
    options.rtl ? FLAG_rtl : 0
  ) | (
    options.includePrerelease ? FLAG_includePrerelease : 0
  )
}

Copy link

Choose a reason for hiding this comment

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

I don't think the suggestion is to change Options, but to use a different way of constructing a key.

That being said, I think it should be possible to internally store flags for these and then recustruct the options bag when asked on whichever methods that request it. Then, internally, things can use the fast check and pass number around. Spitballing, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I realized a few seconds after writing that, and yes, using the option internally and then exposing makes sense, I'll try using the getter to compute the options and internally using just the bit flag.

Copy link

Choose a reason for hiding this comment

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

Thinking further, the user already has to pass the object in, so you probably can just keep that one and not reconstruct it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakebailey I tried, but it would break a lot of tests if I did.

I put the version of parseOptions with bit flags here: h4ad-forks@6c7a68a

@isaacs Take a look and see if this PR is worth pushing, I made a lot of changes.

Also, I introduce 1 small breaking change, now the numbers passed to parseOptions are considered valid options, so instead of re-parsing it by passing the object, I use many of the options as bit flags to construct new objects that are inside the functions.

About performance, no performance penalties and I assume it got a bit faster because I now parse the options as an object once and then just parse the options again as a number which saves some comparisons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only downside I notice when using bitmask is the memory usage (ref: #528 (comment)) is the same as allocating the objects at runtime (8.7mb).

I don't know why this behavior is happening, but I just want to point out that it's not as memory efficient as Object.freeze.

if (options.includePrerelease === true) {
if (options.loose === true && options.rtl === true) {
return '1';
}

if (options.loose === true) {
return '2';
}

if (options.rtl === true) {
return '3';
}

return '4';
} else if (options.loose === true) {
if (options.rtl === true) {
return '5';
}

return '6';
} else if (options.rtl === true) {
return '7';
} else {
return '8';
}
}

module.exports = Range

const LRU = require('lru-cache')
Expand Down
28 changes: 17 additions & 11 deletions internal/parse-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
// parse out just the options we care about so we always get a consistent
// obj with keys in a consistent order.
const opts = ['includePrerelease', 'loose', 'rtl']
const parseOptions = options =>
!options ? {}
: typeof options !== 'object' ? { loose: true }
: opts.filter(k => options[k]).reduce((o, k) => {
o[k] = true
return o
}, {})
module.exports = parseOptions
const parseOptions = options => {
if (!options) return {};

if (typeof options !== 'object') return { loose: true };

const parsedOptions = {};

// parse out just the options we care about so we always get a consistent
// obj with keys in a consistent order.

if (options.includePrerelease) parsedOptions.includePrerelease = true;
if (options.loose) parsedOptions.loose = true;
if (options.rtl) parsedOptions.rtl = true;

return parsedOptions;
};
module.exports = parseOptions;