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.

Sign up for GitHub

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

Jump to bottom

Optimise JSON marshalling for sparse histograms #440

Merged
merged 7 commits into from
Feb 2, 2023
Merged

Optimise JSON marshalling for sparse histograms #440

merged 7 commits into from
Feb 2, 2023

Conversation

zenador
Copy link
Contributor

@zenador zenador commented Jan 27, 2023 •
edited

Follow up to #417 according to suggestion in #417 (comment)

Benchmarking results:

name                                         old time/op    new time/op    delta
JSONMarshallingSamplePairMatrix-16             6.96µs ± 8%    3.44µs ± 4%  -50.52%  (p=0.000 n=20+19)
JSONMarshallingSampleHistogramPairMatrix-16    69.2µs ± 4%    18.9µs ± 4%  -72.65%  (p=0.000 n=20+19)

name                                         old alloc/op   new alloc/op   delta
JSONMarshallingSamplePairMatrix-16             1.63kB ± 0%    0.84kB ± 0%  -48.31%  (p=0.000 n=20+20)
JSONMarshallingSampleHistogramPairMatrix-16    20.3kB ± 0%     4.6kB ± 0%  -77.17%  (p=0.000 n=20+20)

name                                         old allocs/op  new allocs/op  delta
JSONMarshallingSamplePairMatrix-16               74.0 ± 0%      22.0 ± 0%  -70.27%  (p=0.000 n=20+20)
JSONMarshallingSampleHistogramPairMatrix-16       658 ± 0%        22 ± 0%  -96.66%  (p=0.000 n=20+20)

@zenador
Merge branch 'main' into sparsehistograms
0e0dee6 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador
Remove sampleHistogramPairPtr
14a8016 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador zenador force-pushed the sparsehistograms branch 5 times, most recently from 09084ca to 0ed9457 Compare January 27, 2023 10:46
codesome
codesome reviewed Jan 31, 2023
View reviewed changes
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I only have comments about comments and removing a check in benchmarks

model/value_marshal.go Outdated Show resolved Hide resolved
model/value_marshal.go Outdated Show resolved Hide resolved
model/value_marshal.go Outdated Show resolved Hide resolved
model/value_marshal.go Outdated Show resolved Hide resolved
model/value_histogram_test.go Outdated Show resolved Hide resolved
model/value_float_test.go Outdated Show resolved Hide resolved
@zenador
Add benchmarks for marshalling
01880dc 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador
Start to use json-iterator to improve marshalling of HistogramBucket
eda7de1 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador
Continue to use json-iterator to improve marshalling of SampleHistogr…
f71df30 
…amPair

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador
Continue to use json-iterator to improve marshalling of SamplePair
4e9e3f5 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador
Update comments according to code review
18a4214 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador
Copy link
Contributor Author

zenador commented Feb 1, 2023

Thanks for the review! Updated benchmark results:

name                                         old time/op    new time/op    delta
JSONMarshallingSamplePairMatrix-16             6.66µs ± 4%    3.36µs ± 1%  -49.58%  (p=0.000 n=20+19)
JSONMarshallingSampleHistogramPairMatrix-16    68.2µs ± 4%    18.2µs ± 1%  -73.27%  (p=0.000 n=19+20)

name                                         old alloc/op   new alloc/op   delta
JSONMarshallingSamplePairMatrix-16             1.63kB ± 0%    0.84kB ± 0%  -48.31%  (p=0.000 n=18+20)
JSONMarshallingSampleHistogramPairMatrix-16    20.3kB ± 0%     4.6kB ± 0%  -77.17%  (p=0.000 n=20+20)

name                                         old allocs/op  new allocs/op  delta
JSONMarshallingSamplePairMatrix-16               74.0 ± 0%      22.0 ± 0%  -70.27%  (p=0.000 n=20+20)
JSONMarshallingSampleHistogramPairMatrix-16       658 ± 0%        22 ± 0%  -96.66%  (p=0.000 n=20+20)

@codesome codesome merged commit f9c1994 into prometheus:main Feb 2, 2023
krajorama added a commit to grafana/mimir that referenced this pull request Feb 20, 2023
@krajorama
Vendor update prometheus/common
9487447 
At prometheus/common@f9c1994

For prometheus/common#440
Optimise JSON marshalling for sparse histograms and floats

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama mentioned this pull request Feb 20, 2023
Merged
krajorama added a commit to grafana/mimir that referenced this pull request Feb 21, 2023
@krajorama
Vendor update prometheus/common (#4253)
0b53073 
At prometheus/common@f9c1994

For prometheus/common#440
Optimise JSON marshalling for sparse histograms and floats

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
beorn7 added a commit that referenced this pull request Feb 23, 2023
@beorn7
model: Remove json-iterator usage (again)
20d4829 
This seemingly undoes #440, but all the json-iterator usage was
actually pulled up into client_golang in
prometheus/client_golang#1225 . Detailed
explanation there. In short, we would like to keep heavy dependencies
like json-iterator out of prometheus/common/model since many importers
of this package aren't even interested in the JSON marshaling.

Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7 beorn7 mentioned this pull request Feb 23, 2023
Merged
beorn7 added a commit that referenced this pull request Feb 28, 2023
@beorn7
model: Remove json-iterator usage (again)
456cf05 
This seemingly undoes #440, but all the json-iterator usage was
actually pulled up into client_golang in
prometheus/client_golang#1225 . Detailed
explanation there. In short, we would like to keep heavy dependencies
like json-iterator out of prometheus/common/model since many importers
of this package aren't even interested in the JSON marshaling.

Signed-off-by: beorn7 <beorn@grafana.com>
radek-ryckowski pushed a commit to goldmansachs/common that referenced this pull request May 18, 2023
@beorn7 @radek-ryckowski
model: Remove json-iterator usage (again)
7b7b794 
This seemingly undoes prometheus#440, but all the json-iterator usage was
actually pulled up into client_golang in
prometheus/client_golang#1225 . Detailed
explanation there. In short, we would like to keep heavy dependencies
like json-iterator out of prometheus/common/model since many importers
of this package aren't even interested in the JSON marshaling.

Signed-off-by: beorn7 <beorn@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codesome codesome codesome approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@zenador @codesome