Optimise Ingester.QueryStream#1306
Conversation
|
Before:
After:
|
tomwilkie
force-pushed
the
query-stream-bench
branch
2 times, most recently
from
28c0797 to
a8a0a26
Compare
|
See the notes on I've run our other benchmarks and don't see any regressions. |
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
This change removes the use of LabelPair and wire.Bytes. We directly marshal and unmarshal from the bytes on the wire into labels.Labels, via LabelsAdapater. I've migrated everywhere I could find to these new types. We can almost completely remove wire.Bytes, execpt for its use in the caching index proto. So I moved the type there. I'd draw attention to the use of yoloString and the use of unsafe for casting between []LabelAdapater and labels.Labels. They should be safe as we take a copy of the label string in the inverted index. Also: - changed all occurances of `__name__` to model.MetricNameLabel. - removed a bunch of unused functions for converting between old types. - make the FromXXXToYYY naming consistent in compat.go. - fork stdlib fnv32 hashing like we have with fnv64. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
tomwilkie
force-pushed
the
query-stream-bench
branch
from
a8a0a26 to
922a3b9
Compare
|
Deployed this out to one of our (under provisioned) test envs, saw P99 QueryStream drop by ~50%. Also say a reduction in push latency, but the new image includes a bunch of optimisations so I wouldn't read too much into that.
|
gouthamve
left a comment
gouthamve
left a comment
There was a problem hiding this comment.
Looks good. If I didn't miss anything, the main change here is to move away from proto generated LabelPair to custom type LabelAdapter labels.Label and using unsafe to do away with any copying? Also reducing allocs by using yoloString in the unmarshalling?
| result = append(result, fps...) | ||
| } | ||
|
|
||
| sort.Sort(fingerprints(result)) |
There was a problem hiding this comment.
Hmm, why was this removed?
There was a problem hiding this comment.
"Remove the sort from the inverted index Lookup, its not actually needed."
I also add a comment in the place we call this discussion the locking. Previously we used to ensure we locked the series in order for a query, to prevent deadlocks. Now we only ever hold one series lock at once, so deadlocks can't happen. Hence we don't need the sort.
| type fingerprintLocker struct { | ||
| fpMtxs []paddedMutex | ||
| numFpMtxs uint | ||
| numFpMtxs uint32 |
There was a problem hiding this comment.
Does this really make enough difference? Just curious if it does.
There was a problem hiding this comment.
This makes no difference. its there as I pulled in the fnv32 implementation to avoid copying a string to a []byte, and reused it here.
Thats right; the commit comment on the last change goes into more detail. Removing the copying is pretty much the only change here, but it was quite involved.
We previously used LabelPair, which avoided the copy by storing the labels as []byte. As I've moved everything to labels.Labels (via LabelAdapater), we need a different technique to avoid the copy - |
|
I've got this deployed to our dev/test envs. Will leave it there for a day or so before I merge this. |
khaines
left a comment
khaines
left a comment
There was a problem hiding this comment.
LGTM. While you said there is more work to do, that relative drop in time is significant on its own.
|
Thanks Ken! That comments was out of date - I've plumbed this change through everywhere and had it in test for the last few days. |
4 participants
labels.Lables(warning: usesunsafe)For me, this reduces the QueryStream run time from ~1.7s to ~1s.
Still got work to do in other packages, and not happy with so many different representations of labels - need to tidy this up.