-
Notifications
You must be signed in to change notification settings - Fork 98
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
ClusterMap benchmarks #856
Conversation
We just need to add an exception in the cargo-deny for xxhash's
I can observe it now with #855, it was just lost compared to massive cloning we were doing in that. |
Just making a note for myself - if you add the exception, this will add this notice to our licence.html document - but I do need to do the work to include it in our release .zip (which is easy enough to do) -- I'll write a ticket for this now so I remember to do it before the next release. The licence.html file is already in the Docker image. |
Is all of that necessary? I can just take it out, I was only using it for testing purposes, and it's a dev only dependency so not part of any release artifacts. |
We already do 95% of the work already to stay in compliance with OSS licencing - so it's not a big step to add this extra piece. We should probably ship the licence.html anyway in the zip to be honest - so it's work I'll do before our next release anyway whether you want to keep the dependency or not. Up to you 👍🏻 |
Sorry, I was eager to bench with these improvements, so I pushed a commit to add the exception 😄 |
b3baaca
to
cea5c98
Compare
Build Succeeded 🥳 Build Id: 15b0d34e-f7b0-400f-8d69-95ce92deaa2a The following development images have been built, and will exist for the next 30 days: To build this version:
|
During investigation of #837 (which fortunately? ended with the original issue not being observed) I added several benchmarks.
While getting familiar with the code, I noticed an easy win of
Locality
using 3 unnecessary heap allocations, now it uses 0, and no longer heap allocates during parsing. This also simplifies how it is serialized to/from json which gives slight speedups on both ends since it's not serializing as many fields. Really, locality is essentially an opaque identifier as far as the codebase is concerned, and it could probably be even more simplified to a single hash value, but that can be done separately, this was mainly just to see numbers change in the benchmarks.