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: faster toString for integrity #75

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Apr 3, 2023

Performance before:

parsed.toString() x 1,595,186 ops/sec ±1.35% (90 runs sampled)
parsedStrict.toString() x 1,553,016 ops/sec ±1.82% (90 runs sampled)

After:

parsed.toString() x 8,128,007 ops/sec ±1.82% (90 runs sampled)
parsedStrict.toString() x 8,322,286 ops/sec ±1.30% (93 runs sampled)
benchmark.js
const Benchmark = require('benchmark')
const ssri = require('./lib/index');
const suite = new Benchmark.Suite;

const integrity = `sha512-WrLorGiX4iEWOOOaJSiCrmDIamA47exH+Bz7tVwIPb4sCU8w4iNqGCqYuspMMeU5pgz/sU7koP5u8W3RCUojGw== sha256-Qhx213Vjr6GRSEawEL0WTzlb00whAuXpngy5zxc8HYc=`;
const parsed = ssri.parse(integrity);
const parsedStrict = ssri.parse(integrity, { strict: true });

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

When parsing is strict, I introduce a small break change because I know the possible hashes, so I call it directly.
Instead of the toString being stringified in the order in which the hashes were added, it will always be stringified in this order: sha512, sha384 and sha256.

If you think that is problematic, we can keep the loop, even if is a little bit slower.

References

Related to #71

@H4ad H4ad requested a review from a team as a code owner April 3, 2023 15:43
@H4ad H4ad requested a review from wraithgar April 3, 2023 15:43
lib/index.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

npm run lintfix should shore up the linting errors (it usually fixes everything but line length warnings)

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

Reading the readme again we do not make any guarantees about the order that the algorithms will be in toString. The order doesn't matter to things like parse either so I think this can remain a patch release.

@wraithgar wraithgar merged commit a316b12 into npm:main Apr 3, 2023
@github-actions github-actions bot mentioned this pull request Apr 3, 2023
@H4ad H4ad deleted the fix/faster-to-string branch April 6, 2023 17:00
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.

2 participants