fix: remove spread of defaultOpts

[H4ad]

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 });

[wraithgar]

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]

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]

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]

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()
})

[wraithgar]

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]

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