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

[nhcb branch] Use single bit to differentiate between optimized bounds and floats #13828

Merged
merged 7 commits into from
Mar 27, 2024

Conversation

krajorama
Copy link
Member

Use one bit to decide what kind of data to read/write. This reduces storage need of floats from 72 bits to 65 bits and makes the integers store in 17 instead of 16.

Use one bit to decide what kind of data to read/write.
This reduces storage need of floats from 72 bits to 65 bits and makes the
integers store in 17 instead of 16.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@zenador
Copy link
Contributor

zenador commented Mar 22, 2024

LGTM, and passes all the unit tests of nhcb-tests-eval in my local branch.

@bboreham bboreham changed the title Use single bit to differentiate between optimized bounds and floats [nhcb branch] Use single bit to differentiate between optimized bounds and floats Mar 23, 2024
@bboreham
Copy link
Member

What is the nhcb branch? Is it ok that TestQueryStatistics failed in CI?

Doesn't this make a breaking change to the storage on disk?

@krajorama
Copy link
Member Author

What is the nhcb branch? Is it ok that TestQueryStatistics failed in CI?

Doesn't this make a breaking change to the storage on disk?

What it is:
https://github.com/prometheus/proposals/blob/main/proposals/2024-01-26_classic-histograms-stored-as-native-histograms.md
EPIC: #13485

It's not a breaking change, doesn't affect histogram chunk with the current schema for exponential histograms.

@beorn7
Copy link
Member

beorn7 commented Mar 25, 2024

Generally, I would say it helps a bit if we store things byte-aligned. Of course, it depends on the exact location in the bit stream if things are byte aligned at the position where we store the NHCB bounds. If generally not byte-aligned stuff has been in the stream before anyway, maybe we could also use the varbit (not varint) encoding for integers and be even a bit more compact.
(I preferred varint previously because I somehow assumed we are still byte-aligned, but that might actually not be true. Needs verification.)

@beorn7
Copy link
Member

beorn7 commented Mar 25, 2024

OK, I checked.

We are definitely not byte-aligned at the time putHistogramChunkLayoutCustomBounds is called.

So I would like to modify the algorithm once again:

  • If you get a positive integer number (or zero) after multiplying with 1000, write a 1 bit and then use putVarbitUint to write that integer.
  • Otherwise, do what's done in this PR (write a 0 bit and the 64 bit of the float64).

Update the limit under which we optimize since the new function is more
permissive.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@zenador
Copy link
Contributor

zenador commented Mar 26, 2024

Actually, looking at how varbit is implemented there, maybe this optimisation could work? zenador@1f97f35

And once the implementation is finalised, we'd need to update the doc comments.

@beorn7
Copy link
Member

beorn7 commented Mar 26, 2024

Actually, looking at how varbit is implemented there, maybe this optimisation could work? zenador@1f97f35

Yes, this should work.

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@krajorama
Copy link
Member Author

Actually, looking at how varbit is implemented there, maybe this optimisation could work? zenador@1f97f35

Yes, this should work.

Picked @zenador optimization, nice find!

krajorama and others added 3 commits March 27, 2024 17:52
Signed-off-by: George Krajcsovits <krajorama@users.noreply.github.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@zenador
Copy link
Contributor

zenador commented Mar 27, 2024

LGTM

@krajorama krajorama merged commit 4eab18a into prometheus:nhcb Mar 27, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants