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

Remove Murmurhash in favor of crypto.subtle.digest or just a Map #454

Closed
Snapstromegon opened this issue Jan 9, 2024 · 3 comments
Closed
Assignees
Labels
improvement Request a change of an existing feature

Comments

@Snapstromegon
Copy link

Snapstromegon commented Jan 9, 2024

Description

Right now murmurhash is used for grouping items in the batchExecutor by the item's metadata.
This is the only place where this dependency is used and there are other hashing algorithms like sha implemented in all modern browsers using the crypto.subtle API.
Even more so, all browsers have a Map implementation that's better than O(n), some (like Chromium) even use a hashtable themselves. In these cases the faro-web-sdk is hashing a value, so the hash can be hashed.

Proposed solution

Remove the dependency for murmurhash-js and instead either use Map directly or use the crypto.subtle.digest() for hashing instead.

Context

This would reduce the bundle size by ~1.2kb or 2% (5.6% of faro-core) and improve performance.

@Snapstromegon Snapstromegon added the improvement Request a change of an existing feature label Jan 9, 2024
@eskirk eskirk self-assigned this Jan 9, 2024
@Snapstromegon
Copy link
Author

As far as I can tell Spidermonkey (the JS engine of Firefox) also handles Map objects as a hashmap internally too: https://searchfox.org/mozilla-central/source/js/src/builtin/MapObject.cpp
I don't know how things are on the JavaScriptCore side (engine of Safari/WebKit).

@eskirk
Copy link
Collaborator

eskirk commented Jan 10, 2024

@Snapstromegon - thanks for the suggestion. I am doing a bit of research to see if we can fall back to using the builtin Map. I definitely love to remove dependencies when possible so I am hoping you are right. going to discuss with the team and see what our best approach should be.

@kpelelis kpelelis assigned kpelelis and unassigned eskirk Jan 11, 2024
@eskirk eskirk self-assigned this Jan 17, 2024
@eskirk
Copy link
Collaborator

eskirk commented Jan 17, 2024

after doing a bit of research into WebKit, we have decided to use plain Maps and remove the murmurhash dependency.

here is the PR - thanks for the suggestion 👍🏼

@eskirk eskirk closed this as completed Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Request a change of an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants