Skip to content

Commit

Permalink
fix: optimize Range parsing and formatting (#726)
Browse files Browse the repository at this point in the history
<!-- What / Why -->
<!-- Describe the request in detail. What it does and why it's being
changed. -->

This pull request optimizes the Range class in the following ways:

1. Produce fewer intermediate objects when reducing a range's space
characters to single spaces. This seems to improve bench-subset scores
by up to 5%, and bench-satisfies scores to a lesser degree.
2. Optimize Range formatting with explicit for loops instead, avoiding
an intermediate array creation. This seems to improve bench-satisfies
and bench-subset scores by up to 20%.
3. Calculate Range's `.range` string (used by `.format()` and
`.toString()`) lazily. This seems to improve bench-satisfies and
bench-subset scores by up to 9%.

The external interface for the class stays the same, except for the new
internal `.formatted` property used to cache its lazily calculated
string. `Range#range` property is now also read-only.

There is a new test lazy formatting to ensure full test coverage.

The benchmarks bench-satisfies and bench-subset benefit from these
changes, sometimes by up to 40%. Other benchmark results seem to stay
the same. Here are the affected benchmarks before:

```
$ node benchmarks/bench-satisfies.js
satisfies(1.0.6, 1.0.3||^2.0.0) x 695,094 ops/sec ±0.68% (97 runs sampled)
satisfies(1.0.6, 2.2.2||~3.0.0) x 764,115 ops/sec ±0.40% (99 runs sampled)
satisfies(1.0.6, 2.3.0||<4.0.0) x 805,593 ops/sec ±0.62% (97 runs sampled)
satisfies(1.0.6, 1.0.3||^2.0.0, {"includePrelease":true}) x 695,045 ops/sec ±0.73% (95 runs sampled)
satisfies(1.0.6, 2.2.2||~3.0.0, {"includePrelease":true}) x 750,433 ops/sec ±0.66% (99 runs sampled)
satisfies(1.0.6, 2.3.0||<4.0.0, {"includePrelease":true}) x 787,903 ops/sec ±0.39% (99 runs sampled)
satisfies(1.0.6, 1.0.3||^2.0.0, {"includePrelease":true,"loose":true}) x 652,166 ops/sec ±0.34% (99 runs sampled)
satisfies(1.0.6, 2.2.2||~3.0.0, {"includePrelease":true,"loose":true}) x 696,377 ops/sec ±0.36% (96 runs sampled)
satisfies(1.0.6, 2.3.0||<4.0.0, {"includePrelease":true,"loose":true}) x 721,729 ops/sec ±0.35% (98 runs sampled)
satisfies(1.0.6, 1.0.3||^2.0.0, {"includePrelease":true,"loose":true,"rtl":true}) x 585,692 ops/sec ±0.75% (95 runs sampled)
satisfies(1.0.6, 2.2.2||~3.0.0, {"includePrelease":true,"loose":true,"rtl":true}) x 631,653 ops/sec ±0.33% (96 runs sampled)
satisfies(1.0.6, 2.3.0||<4.0.0, {"includePrelease":true,"loose":true,"rtl":true}) x 650,110 ops/sec ±0.64% (95 runs sampled)

$ node benchmarks/bench-subset.js
subset(1.2.3, *) x 633,342 ops/sec ±0.61% (95 runs sampled)
subset(^1.2.3, *) x 743,036 ops/sec ±0.47% (97 runs sampled)
subset(^1.2.3-pre.0, *) x 680,087 ops/sec ±0.76% (98 runs sampled)
subset(^1.2.3-pre.0, *) x 680,948 ops/sec ±0.46% (96 runs sampled)
subset(1 || 2 || 3, *) x 330,669 ops/sec ±0.53% (98 runs sampled)
```

And after:

```
$ node benchmarks/bench-satisfies.js
satisfies(1.0.6, 1.0.3||^2.0.0) x 896,936 ops/sec ±0.53% (94 runs sampled)
satisfies(1.0.6, 2.2.2||~3.0.0) x 998,214 ops/sec ±0.40% (95 runs sampled)
satisfies(1.0.6, 2.3.0||<4.0.0) x 1,000,593 ops/sec ±0.43% (97 runs sampled)
satisfies(1.0.6, 1.0.3||^2.0.0, {"includePrelease":true}) x 890,369 ops/sec ±0.41% (100 runs sampled)
satisfies(1.0.6, 2.2.2||~3.0.0, {"includePrelease":true}) x 977,239 ops/sec ±0.48% (97 runs sampled)
satisfies(1.0.6, 2.3.0||<4.0.0, {"includePrelease":true}) x 983,682 ops/sec ±0.95% (96 runs sampled)
satisfies(1.0.6, 1.0.3||^2.0.0, {"includePrelease":true,"loose":true}) x 805,330 ops/sec ±0.84% (98 runs sampled)
satisfies(1.0.6, 2.2.2||~3.0.0, {"includePrelease":true,"loose":true}) x 894,117 ops/sec ±0.43% (99 runs sampled)
satisfies(1.0.6, 2.3.0||<4.0.0, {"includePrelease":true,"loose":true}) x 911,742 ops/sec ±0.42% (96 runs sampled)
satisfies(1.0.6, 1.0.3||^2.0.0, {"includePrelease":true,"loose":true,"rtl":true}) x 741,254 ops/sec ±0.35% (97 runs sampled)
satisfies(1.0.6, 2.2.2||~3.0.0, {"includePrelease":true,"loose":true,"rtl":true}) x 807,380 ops/sec ±0.42% (99 runs sampled)
satisfies(1.0.6, 2.3.0||<4.0.0, {"includePrelease":true,"loose":true,"rtl":true}) x 820,390 ops/sec ±0.37% (99 runs sampled)

$ node benchmarks/bench-subset.js
subset(1.2.3, *) x 905,030 ops/sec ±0.63% (96 runs sampled)
subset(^1.2.3, *) x 1,026,457 ops/sec ±0.63% (95 runs sampled)
subset(^1.2.3-pre.0, *) x 923,789 ops/sec ±0.41% (97 runs sampled)
subset(^1.2.3-pre.0, *) x 923,136 ops/sec ±0.44% (96 runs sampled)
subset(1 || 2 || 3, *) x 432,037 ops/sec ±0.67% (96 runs sampled)
```
  • Loading branch information
jviide authored Jul 16, 2024
1 parent 2975ece commit 73a3d79
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 10 deletions.
34 changes: 24 additions & 10 deletions classes/range.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const SPACE_CHARACTERS = /\s+/g

// hoisted class for cyclic dependency
class Range {
constructor (range, options) {
Expand All @@ -18,7 +20,7 @@ class Range {
// just put it in the set and return
this.raw = range.value
this.set = [[range]]
this.format()
this.formatted = undefined
return this
}

Expand All @@ -29,10 +31,7 @@ class Range {
// First reduce all whitespace as much as possible so we do not have to rely
// on potentially slow regexes like \s*. This is then stored and used for
// future error messages as well.
this.raw = range
.trim()
.split(/\s+/)
.join(' ')
this.raw = range.trim().replace(SPACE_CHARACTERS, ' ')

// First, split on ||
this.set = this.raw
Expand Down Expand Up @@ -66,14 +65,29 @@ class Range {
}
}

this.format()
this.formatted = undefined
}

get range () {
if (this.formatted === undefined) {
this.formatted = ''
for (let i = 0; i < this.set.length; i++) {
if (i > 0) {
this.formatted += '||'
}
const comps = this.set[i]
for (let k = 0; k < comps.length; k++) {
if (k > 0) {
this.formatted += ' '
}
this.formatted += comps[k].toString().trim()
}
}
}
return this.formatted
}

format () {
this.range = this.set
.map((comps) => comps.join(' ').trim())
.join('||')
.trim()
return this.range
}

Expand Down
9 changes: 9 additions & 0 deletions test/classes/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,15 @@ test('tostrings', (t) => {
t.end()
})

test('formatted value is calculated lazily and cached', (t) => {
const r = new Range('>= v1.2.3')
t.equal(r.formatted, undefined)
t.equal(r.format(), '>=1.2.3')
t.equal(r.formatted, '>=1.2.3')
t.equal(r.format(), '>=1.2.3')
t.end()
})

test('ranges intersect', (t) => {
rangeIntersection.forEach(([r0, r1, expect]) => {
t.test(`${r0} <~> ${r1}`, t => {
Expand Down

0 comments on commit 73a3d79

Please sign in to comment.