Skip to content

fix: remove spread of defaultOpts#72

Merged
wraithgar merged 1 commit intonpm:mainfrom
h4ad-forks:perf/default-options
Apr 3, 2023
Merged

fix: remove spread of defaultOpts#72
wraithgar merged 1 commit intonpm:mainfrom
h4ad-forks:perf/default-options

Conversation

@H4ad
Copy link
Contributor

@H4ad H4ad commented Apr 1, 2023

I was looking the CPU profiler of pnpm and I saw this call:

https://github.com/pnpm/pnpm/blob/ef6c22e129dc3d76998cee33647b70a66d1f36bf/store/cafs/src/getFilePathInCafs.ts#L29-L30

I thought about what I could do to optimize and then I found good performance improvements.

Removing spread of opts

The first thing I notice was the spread of defaultOpts in every method, sometimes being called twice without needing.
So remove all the calls, before this change:

ssri.parse(base64, { single: true }) x 2,194,090 ops/sec ±0.58% (93 runs sampled)
ssri.parse(base64, { single: true, strict: true }) x 1,389,626 ops/sec ±1.09% (94 runs sampled)
ssri.parse(parsed, { single: true }) x 694,087 ops/sec ±1.60% (87 runs sampled)
ssri.parse(parsed, { single: true, strict: true }) x 469,885 ops/sec ±0.96% (95 runs sampled)

With the deletion of opts:

ssri.parse(base64, { single: true }) x 5,011,696 ops/sec ±1.84% (91 runs sampled)
ssri.parse(base64, { single: true, strict: true }) x 2,468,691 ops/sec ±0.66% (95 runs sampled)
ssri.parse(parsed, { single: true }) x 1,726,705 ops/sec ±1.68% (94 runs sampled)
ssri.parse(parsed, { single: true, strict: true }) x 770,549 ops/sec ±2.00% (93 runs sampled)
benchmark.js 
const Benchmark = require('benchmark')
const ssri = require('./lib/index');
const suite = new Benchmark.Suite;
const fs = require('fs');
const crypto = require('crypto');

const TEST_DATA = fs.readFileSync(__filename)

function hash (data, algorithm) {
  return crypto.createHash(algorithm).update(data).digest('base64')
}

const sha = hash(TEST_DATA, 'sha512')
const integrity = `sha512-${sha}`;
const parsed = ssri.parse(integrity, { single: true });

suite
.add('ssri.parse(base64, { single: true })', function () {
  ssri.parse(integrity, { single: true })
})
.add('ssri.parse(base64, { single: true, strict: true })', function () {
  ssri.parse(integrity, { single: true, strict: true })
})
.add('ssri.parse(parsed, { single: true })', function () {
  ssri.parse(parsed, { single: true })
})
.add('ssri.parse(parsed, { single: true, strict: true })', function () {
  ssri.parse(parsed, { single: true, strict: true })
})
.on('cycle', function(event) {
  console.log(String(event.target))
})
.run({ 'async': false });

@H4ad H4ad requested a review from a team as a code owner April 1, 2023 19:29
@H4ad H4ad requested a review from lukekarrys April 1, 2023 19:29
@H4ad H4ad mentioned this pull request Apr 1, 2023
Closed
@wraithgar
Copy link
Member

wraithgar commented Apr 2, 2023 •
edited
Loading

Just a heads up for this and future PRs, we only allow a very limited subset of conventional commit prefixes: fix, feat, chore, docs, deps. These will all be fix commits (unless we decide something here requires a breaking change, which I'm not ruling out as we work through these default options).

@wraithgar wraithgar changed the title perf: remove spread of defaultOpts fix: remove spread of defaultOpts Apr 2, 2023
@H4ad H4ad force-pushed the perf/default-options branch from 61dccf8 to 028b9b8 Compare April 2, 2023 00:29
@wraithgar
Copy link
Member

wraithgar commented Apr 2, 2023

I think this is really heading in a good direction. Let's take a very critical look at defaulOpts now. At a cursory glance it looks like error, options, single, sep, and strict are all superfluous now? That would leave just two things and at that point do we really need them abstracted into an object? The default algorithms can live next to SPEC_ALGORITHMS as its own declaration, and the single reference to defaultOpts.pickAlgorithm can be replaced w/ a direct reference to getPrioritizedHash.

@wraithgar
Copy link
Member

wraithgar commented Apr 2, 2023

Just a note for either you if you pick it up or for future me if you don't. The getOptString logic appears to be duplicated in the toString method. The logic there is inverted from that of getOptString and is also easier to follow.

Here's a handy diff of my notes so far.
diff.txt

@wraithgar wraithgar self-assigned this Apr 2, 2023
@wraithgar wraithgar requested review from wraithgar and removed request for lukekarrys April 2, 2023 02:16
@wraithgar
Copy link
Member

wraithgar commented Apr 2, 2023

A note for folks who may be following along in the future, one thing that spread gives us that our new opts?.sep does not is this subtle difference:

> defaults
{ sep: ' ' }
> opts
{ sep: '' }
> { ...defaults, ...opts }
{ sep: '' }

Truthiness is distinct from attribute presence. HOWEVER we already have tests that show that even if you provide your own default, if it is falsey then it is coerced to a space.

test('use " " as sep when opts.sep is falsey', t => {
  const parsed = ssri.parse('yum-somehash foo-barbaz')
  t.equal(parsed.toString({ sep: false }), 'yum-somehash foo-barbaz')
  t.equal(parsed.toString({ sep: '\t' }), 'yum-somehash\tfoo-barbaz')
  t.end()
})

@H4ad H4ad force-pushed the perf/default-options branch from 028b9b8 to 8034d1b Compare April 2, 2023 05:08
@wraithgar wraithgar merged commit 6e6877d into npm:main Apr 3, 2023
@github-actions github-actions bot mentioned this pull request Apr 3, 2023
Merged
@H4ad H4ad deleted the perf/default-options branch April 3, 2023 14:15
@wraithgar
Copy link
Member

wraithgar commented Apr 3, 2023

It wasn't worth keeping this PR going for another day but heads up we'll probably move Object.create(null) back to a bare {}. Prototype pollutioning that comes from outside this module isn't something we typically guard against. If that's already happened there's not much this module can do about it.

@H4ad
Copy link
Contributor Author

H4ad commented Apr 3, 2023

@wraithgar No problem, from what I remember, I use that to protect against prototype pollution and also that way is a little bit faster than the {}, but I'm not sure about the last one.

About the other perf improvements, I will try to push this week as soon I have more time.

@wraithgar wraithgar mentioned this pull request Mar 26, 2024
Merged
4 tasks
H4ad added a commit to h4ad-forks/ssri that referenced this pull request Dec 15, 2025
@H4ad
chore: add benchmarks
13d63d9 
Reference:
- npm#71
- npm#72
- npm#75
- npm#76
@H4ad H4ad mentioned this pull request Dec 15, 2025
Merged
wraithgar pushed a commit that referenced this pull request Dec 16, 2025
@H4ad
chore: add benchmarks (#161)
5ca3f4a 
Reference:
- #71
- #72
- #75
- #76

Started by #160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wraithgar wraithgar wraithgar approved these changes

Assignees

@wraithgar wraithgar

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@H4ad @wraithgar