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

fix: reuse comparators on subset #537

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Apr 6, 2023

As I comment here: #528 (comment)

The method of subset was creating comparators without needing for a specific ranges, perf:

subset(1.2.3, *) x 208,363 ops/sec ±1.01% (93 runs sampled)
subset(^1.2.3, *) x 238,678 ops/sec ±1.08% (91 runs sampled)
subset(^1.2.3-pre.0, *) x 235,344 ops/sec ±1.12% (92 runs sampled)
subset(^1.2.3-pre.0, *) x 238,530 ops/sec ±0.97% (96 runs sampled)
subset(1 || 2 || 3, *) x 117,125 ops/sec ±0.83% (93 runs sampled)

After caching those comparators:

subset(1.2.3, *) x 241,957 ops/sec ±0.78% (93 runs sampled)
subset(^1.2.3, *) x 277,689 ops/sec ±1.02% (93 runs sampled)
subset(^1.2.3-pre.0, *) x 278,444 ops/sec ±1.29% (95 runs sampled)
subset(^1.2.3-pre.0, *) x 278,750 ops/sec ±0.99% (94 runs sampled)
subset(1 || 2 || 3, *) x 145,633 ops/sec ±1.66% (92 runs sampled)

It's an improvement of about 13~14% for these cases.

benchmark.js
const Benchmark = require('benchmark')
const subset = require('./ranges/subset')
const suite = new Benchmark.Suite()

// taken from tests
const cases = [
  // everything is a subset of *
  ['1.2.3', '*', true],
  ['^1.2.3', '*', true],
  ['^1.2.3-pre.0', '*', false],
  ['^1.2.3-pre.0', '*', true, { includePrerelease: true }],
  ['1 || 2 || 3', '*', true],
]

for (const [sub, dom] of cases) {
  suite.add(`subset(${sub}, ${dom})`, function () {
    subset(sub, dom)
  })
}

suite
  .on('cycle', function (event) {
    console.log(String(event.target))
  })
  .run({ async: false })

About memory

To test the memory usage, I used isitfast which still in beta but provide a useful way of measuring memory (ignore op/s):

subset(1.2.3, *) 71,597 op/s (13,967 ns) ±1% x2,500 | 3,640 kB ±0.1% x25
subset(^1.2.3, *) 100,210 op/s (9,979 ns) ±1% x2,500 | 3,648 kB ±0.1% x25
subset(^1.2.3-pre.0, *) 127,796 op/s (7,825 ns) ±1% x2,500 | 4,088 kB ±0.1% x25
subset(1 || 2 || 3, *) 81,340 op/s (12,294 ns) ±1% x2,500 | 8,976 kB ±0.06% x25

After:

subset(1.2.3, *) 88,324 op/s (11,322 ns) ±1% x2,500 | 2,856 kB ±0.2% x25
subset(^1.2.3, *) 129,433 op/s (7,726 ns) ±1% x2,500 | 2,864 kB ±0.2% x25
subset(^1.2.3-pre.0, *) 169,176 op/s (5,911 ns) ±1% x2,500 | 2,872 kB ±0.2% x25
subset(1 || 2 || 3, *) 96,899 op/s (10,320 ns) ±1% x2,500 | 6,624 kB ±0.08% x25

Is a reduction of 21%~26% in memory usage.

isitfast.js
const { suite, useTerminalCompact } = require('isitfast')

const subset = require('./ranges/subset')

const benchmark = suite('Test', {
  gc: {
    allow: true,
  },
})

// taken from tests
const cases = [
  // everything is a subset of *
  ['1.2.3', '*', true],
  ['^1.2.3', '*', true],
  ['^1.2.3-pre.0', '*', false],
  ['^1.2.3-pre.0', '*', true, { includePrerelease: true }],
  ['1 || 2 || 3', '*', true],
]

for (const [sub, dom] of cases) {
  benchmark.add(`subset(${sub}, ${dom})`, function () {
    subset(sub, dom)
  })
}

useTerminalCompact();

benchmark.run();

References

@H4ad H4ad requested a review from a team as a code owner April 6, 2023 23:50
@H4ad H4ad requested review from fritzy and removed request for a team April 6, 2023 23:50
@wraithgar wraithgar requested review from wraithgar and removed request for fritzy April 7, 2023 00:00
@wraithgar
Copy link
Member

I don't think this overlaps with #538 in any way but am making a note to myself to triple check.

@H4ad H4ad force-pushed the fix/reuse-subset-comparators branch from f7f3f4b to 7ecc0db Compare April 7, 2023 00:07
ranges/subset.js Outdated Show resolved Hide resolved
@H4ad H4ad force-pushed the fix/reuse-subset-comparators branch from 7ecc0db to ec94d56 Compare April 7, 2023 17:33
@wraithgar
Copy link
Member

This is a tiny startup tradeoff for a larger runtime improvement.

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

quick and easy win, lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants