hyperloglog.rs icon indicating copy to clipboard operation
hyperloglog.rs copied to clipboard

Added mem-dbg across crate as optional featuew

Open LucaCappelletti94 opened this issue 1 year ago • 3 comments

Mem-dbg is a crate that allows to compute the size of a struct. I have added the derives through the crate as an optional feature, so as to use it to compare this implementation with others easily.

Cheers!

LucaCappelletti94 avatar Aug 08 '24 09:08 LucaCappelletti94

@tabac I have fixed a rather important error in the library in commit a137c97 - the estimates array were NOT sorted, but a binary search was used nevertheless. I have now sorted estimates alongside their biases so that the binary search actually makes sense.

LucaCappelletti94 avatar Aug 11 '24 08:08 LucaCappelletti94

@LucaCappelletti94 thanks a lot for the PR. I will look into it as soon as possible (probably ~end-of-month).

tabac avatar Aug 13 '24 17:08 tabac

@LucaCappelletti94 PR LGTM.

Three questions I have:

  • I would like to keep the requirements of the library to the minimum. Would it be possible to compute the struct sizes using mem-dbg in another crate by just wrapping the original structs?

  • Would you mind reverting the constants.rs reformatting, it is hard to see the diff this way. Also, since you change the order of the estimates you should change the corresponding biases too.

  • Finally, could you add a comment here that the data are not copied verbatim but instead sorted.

Thanks again for the PR!

tabac avatar Aug 26 '24 19:08 tabac