Smaller and faster murmur

[v1rtl]

Uses a more compact murmur hash implementation by timepp and removes an untyped and heavier murmurhash3js-revisited dependency.

And it's faster than the previous implementation:

summary
  vendored - 332
   1.31x faster than mur3-revisited - 332
   3.49x faster than vendored - 3128
   4.74x faster than vendored - 364
   24.9x faster than mur3-revisited - 3128
   25.1x faster than mur3-revisited - 364

[rvagg]

so is this a vendor of that code? is there a good reason not to put it in the dependencies list and install it properly? and if not, how about we make a vendor/ directory and put it in there so we're clear about what's going on, we have that pattern elsewhere, such as in js-multiformats.

[v1rtl]

so is this a vendor of that code? is there a good reason not to put it in the dependencies list and install it properly? and if not, how about we make a vendor/ directory and put it in there so we're clear about what's going on, we have that pattern elsewhere, such as in js-multiformats.

The reason is because the library is on JSR and not on NPM, which requires a custom registry to be set up in
.npmrc.

I can move it to a vendor/ dir though

[v1rtl]

Added performance benchmarks, the vendored impl is a little slower

Will investigate

[v1rtl]

Turns out BigInt's make the perf much worse. Will rewrite to use plain numbers.

(and address other perf issues)

[v1rtl]

Performance issues fixed, new benchmarks:

clk: ~3.69 GHz
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: bun 1.3.5 (x64-linux)

benchmark                   avg (min … max) p75 / p99    (min … top 1%)
------------------------------------------- -------------------------------
vendored - 332               128.23 ns/iter 128.31 ns  █
                    (115.77 ns … 285.96 ns) 221.73 ns  █
                    (  0.00  b … 520.00  b)   4.61  b ▇█▇▂▄▃▂▂▂▂▁▁▁▁▁▁▁▁▁▁▁

vendored - 3128              448.10 ns/iter 471.36 ns █
                    (401.96 ns … 930.53 ns) 733.47 ns █
                    (  0.00  b …   1.41 kb)  13.30  b █▃▃▃▄▃▂▂▃▂▂▁▁▁▁▁▁▁▁▁▁

vendored - 364               608.31 ns/iter 608.17 ns █
                      (525.83 ns … 1.50 µs)   1.44 µs █
                    (  0.00  b … 440.00  b)  14.49  b ██▇▄▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁

mur3-revisited - 332         168.30 ns/iter 170.69 ns   █
                    (155.34 ns … 338.41 ns) 231.03 ns   █
                    (  0.00  b … 160.00  b)   2.63  b ▃▂█▅▂▃▅▃▁▁▁▁▁▁▁▁▁▁▁▁▁

mur3-revisited - 3128          3.19 µs/iter   3.22 µs  █
                      (2.34 µs … 667.36 µs)   8.05 µs  █▃
                    (  0.00  b … 192.00 kb) 109.15  b ▆██▆▄▃▂▂▂▂▂▁▁▁▁▁▁▁▁▁▁

mur3-revisited - 364           3.22 µs/iter   3.29 µs      ▄  ▄   █
                        (3.01 µs … 3.45 µs)   3.45 µs  ▅▅  █  █ ▅▅█▅
                    (  0.00  b … 800.00  b)  48.00  b ▅██▅██▅▅██████▅██▁█▅█

summary
  vendored - 332
   1.31x faster than mur3-revisited - 332
   3.49x faster than vendored - 3128
   4.74x faster than vendored - 364
   24.9x faster than mur3-revisited - 3128
   25.1x faster than mur3-revisited - 364

[rvagg]

@v1rtl before we go ahead with this we need increased confidence that our vendored and modified code actually produces stable results. This has been a consistent problem with murmur3 (IIRC one of the reasons we use -revisited, see https://cimi.io/murmurhash3js-revisited/ for some of those details). Can you chase down some stable test vectors we could put in here, we only have 2 at the moment, leaving it mostly up to the dependency to get it right. We need to beef up the test suite here if we're going to own the code that does the hashing.

Perhaps we can just import test fixtures from murmurhash3js-revisited? If we match that and there are good edge cases in it then this should be OK I hope.

[v1rtl]

@rvagg thank you for taking time to review and suggesting to add a more comprehensive test suite. I borrowed the tests from revisited and adapted them to the vendored impl (they all pass). Lmk anything else is needed

[rvagg]

@v1rtl can you rebase on master please, I've updated some of the CI components here and want to get a clean run

[v1rtl]

@rvagg done

[rvagg]

great, but now we have new lint errors from the updated toolcahin:

> aegir lint

[23:42:19] eslint [started]
(node:2246) ESLintEnvWarning: /* eslint-env */ comments are no longer recognized when linting with flat config and will be reported as errors as of v10.0.0. Replace them with /* global */ comments or define globals in your config file. See https://eslint.org/docs/latest/use/configure/migration-guide#eslint-env-configuration-comments for details. Found in /home/runner/work/js-murmur3/js-murmur3/test/test-basics.spec.js at line 1.
(Use `node --trace-warnings ...` to show where the warning was created)
(node:2246) ESLintEnvWarning: /* eslint-env */ comments are no longer recognized when linting with flat config and will be reported as errors as of v10.0.0. Replace them with /* global */ comments or define globals in your config file. See https://eslint.org/docs/latest/use/configure/migration-guide#eslint-env-configuration-comments for details. Found in /home/runner/work/js-murmur3/js-murmur3/test/test-impl.spec.js at line 2.
/home/runner/work/js-murmur3/js-murmur3/src/vendor/murmur.js:72:16: Expected { after 'if' condition. [Error/curly]
/home/runner/work/js-murmur3/js-murmur3/src/vendor/murmur.js:73:17: Expected { after 'if' condition. [Error/curly]
/home/runner/work/js-murmur3/js-murmur3/src/vendor/murmur.js:100:16: Expected { after 'if' condition. [Error/curly]
/home/runner/work/js-murmur3/js-murmur3/src/vendor/murmur.js:186:1: Defaults are not permitted on @param. [Warning/jsdoc/no-defaults]
/home/runner/work/js-murmur3/js-murmur3/src/vendor/murmur.js:293:1: Defaults are not permitted on @param. [Warning/jsdoc/no-defaults]
/home/runner/work/js-murmur3/js-murmur3/src/vendor/murmur.js:428:1: Defaults are not permitted on @param. [Warning/jsdoc/no-defaults]

[v1rtl]

will fix

[v1rtl]

fixed

[rvagg]

nice 👌 thanks @v1rtl

[github-actions]

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀