Skip to content

Reimplement Unpack in terms of jsonparser rather than jsoniter #8739

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
Merged
merged 1 commit into from
Mar 8, 2023
Merged

Reimplement Unpack in terms of jsonparser rather than jsoniter #8739

merged 1 commit into from
Mar 8, 2023

Conversation

MasslessParticle
Copy link
Contributor

This PR reimplements the functionality of Unpack using the jsonparser library rather than jsoniter

Benchmarks:

name                                            old time/op    new time/op    delta
_Parser/unpack/no_labels_hints-8                   900ns ± 4%     309ns ± 3%  -65.62%  (p=0.000 n=10+10)
_Parser/unpack/labels_hints-8                     1.06µs ±12%    0.32µs ± 2%  -69.72%  (p=0.000 n=9+10)
_Parser/unpack-not_json_line/no_labels_hints-8    37.5ns ± 8%    10.6ns ± 3%  -71.70%  (p=0.000 n=9+9)
_Parser/unpack-not_json_line/labels_hints-8       39.6ns ±11%    10.3ns ± 2%  -74.00%  (p=0.000 n=10+9)

name                                            old alloc/op   new alloc/op   delta
_Parser/unpack/no_labels_hints-8                    536B ± 0%       80B ± 0%  -85.07%  (p=0.000 n=10+10)
_Parser/unpack/labels_hints-8                       536B ± 0%       80B ± 0%  -85.07%  (p=0.000 n=10+10)
_Parser/unpack-not_json_line/no_labels_hints-8     0.00B          0.00B          ~     (all equal)
_Parser/unpack-not_json_line/labels_hints-8        0.00B          0.00B          ~     (all equal)

name                                            old allocs/op  new allocs/op  delta
_Parser/unpack/no_labels_hints-8                    17.0 ± 0%       4.0 ± 0%  -76.47%  (p=0.000 n=10+10)
_Parser/unpack/labels_hints-8                       17.0 ± 0%       4.0 ± 0%  -76.47%  (p=0.000 n=10+10)
_Parser/unpack-not_json_line/no_labels_hints-8      0.00           0.00          ~     (all equal)
_Parser/unpack-not_json_line/labels_hints-8         0.00           0.00          ~     (all equal)
@MasslessParticle MasslessParticle requested a review from a team as a code owner March 7, 2023 23:19
MasslessParticle
MasslessParticle commented Mar 7, 2023
View reviewed changes
pkg/logql/log/parser_test.go
require.Equal(t, string(tt.wantLine), string(l))
require.Equal(t, tt.wantLine, l)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change means that you get human-readable strings when this fails rather than a pile of bytes.
MichelHollands
MichelHollands approved these changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@MichelHollands MichelHollands left a comment

Choose a reason for hiding this comment

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

Very nice
@MasslessParticle MasslessParticle merged commit 9ac33e2 into main Mar 8, 2023
@MasslessParticle MasslessParticle deleted the tpatterson/jsonparser-for-unpack branch March 8, 2023 15:53
MasslessParticle added a commit that referenced this pull request Mar 21, 2023
@MasslessParticle
Revert "Reimplement Unpack in terms of jsonparser rather than jsoniter (
c829f79 
#8739)"

This reverts commit 9ac33e2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M
2 participants
@MasslessParticle @MichelHollands