Introduce count_min_sketch::serialize_to#509
Open
cv4g wants to merge 5 commits into
Open
Conversation
de5ec55 to
91f6b19
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi,
thank you so much for considering the other two pull requests. I would like to propose one more change to
count_min_sketchfocussing on the serialization.Currently there is a
std::vectorand astd::ostream&based API. ClickHouse is callingserialize()to obtain a vector and is then copying it's contents into another buffer that is then written to disk.I would now like to remove this additional copy. The ClickHouse project is trying to avoid using standard library stream mechanisms which is why I would like to propose to add a sink-based API as an additional option.
Would you be okay with adding this? An advantage is that it could also be used internally to back both
serialize()overloads and thestd::ostream&overload is benefiting from the change as well.The change set consists of four commits:
4e66c11 introduces a new member function
serialize_to()and updates bothserialize()andserialize(std::ostream&)to call it. 63e7f75 improves the implementation ofserialize_to()such that it is passing all sketch cells as a consecutive memory buffer to the sink instead of each one individually.I've added a microbenchmark once more to ensure it's not pessimising the existing API:
6029803:
764cdf1:
63e7f75:
Summary:
The vector serialization regressed badly in 764cdf1 but recovered in 63e7f75 when all sketch cells are written in a single call. It seems that the compiler was able to optimize the tight loop that copied the sketch cells very well.
The
ostream&overload benefits from 63e7f75 a lot.