Timeseries: reuse raw data used to unmarshal Timeseries when marshaling them again #5137

pstibrany · 2023-06-02T07:11:03Z

What this PR does

This PR adds an optimization to PreallocTimeseries: When PreallocTimeseries.Unmarshal(...) is called, we keep passed bytes buffer, and return it again for PreallocTimeseries.Marshal* methods.

This is useful in distributor, which receives write request with many Timeseries in it. Distributor then recombines timeseries into new write requests for ingesters, and marshals them again. Distributor usually (!) doesn't need to modify timeseries in any way, so it could reuse original marshalled version of the timeseries, instead of creating new one.

There are some cases when distributor does in fact modify the timeseries:

When using HA deduplication, distributor removes replica label
Distributor also removes empty labels, and makes sure that labels are sorted
Distributor can optionally drop some labels or run relabelling process

In these cases, we simply "clear" the marshalled version of timeseries, and let marshalling run again.

Is keeping the original raw unmarshalled buffer safe? Can the buffer be modified? Yes, but Timeseries already relies on that buffer being unchanged during the lifecycle of write request, because LabelAdapter type used inside Timeseries also creates its strings by using the same buffer.

In our test, doing this optimisation can save ~25% of CPU time when marshalling write request (when not using HA deduplication, label dropping or relabelling). How much it really helps depends on whether any of above-mentioned features are used.

Due to a risk that we have missed some place where this optimization is not applicable, it's possible to disable it for now by using -timeseries-unmarshal-caching-optimization-enabled=false.

Checklist

Tests updated
[na] Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

56quarters

Changes seem reasonable to me with a few comments, thanks! Are there any benchmark results we can share for this change?

56quarters · 2023-06-02T15:08:53Z

integration/distributor_test.go

I'm not sure why this needs to be an integration test. The existing unit tests seem to make similar assertions without needing to start consul/minio/ingester/etc.

Existing unit tests make assertions on intermediate objects or messages, not on final bytes sent to ingester. I was looking at the test from different perspective — In a way I don’t care what happens in distributor, as long as ingester gets the correct data. That’s why I opted for integration test — it seemed to express that idea in a cleaner way.

If you feel strongly about this, I can try to write a unit test for this instead.

My primary reason for integration test was that when using unit test I could only mock ingester client to get not-yet marshaled WriteRequest. But I wanted to include marshaling in the test too.

pkg/distributor/distributor.go

flxbk · 2023-06-05T08:50:54Z

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

pstibrany · 2023-06-06T10:28:55Z

Q: shall we keep this functionality behind a CLI option until it's proven to work fine?

A: Added CLI option -timeseries-unmarshal-caching-optimization-enabled.

pracucci

This is a pretty interesting optimization. The code changes are clean, but unfortunately it's tricky to find if any usage we make of the Timeseries is safe or not. I agree the integration test gives an higher confidence than unit testing, but I would love to see an integration test with high number of concurrent writes too.

pkg/mimirpb/timeseries.go

pracucci · 2023-06-06T13:20:39Z

pkg/mimirpb/timeseries.go

+
+func (p *PreallocTimeseries) MarshalTo(dAtA []byte) (int, error) {
+	if p.unmarshalData != nil {
+		return p.MarshalToSizedBuffer(dAtA)


MarshalToSizedBuffer() expects the input slice to be sized, while MarshalTo() doesn't. Here we call MarshalToSizedBuffer() assuming it's sized but it may be not. I think we should resize it here, before calling MarshalToSizedBuffer().

I only called p.MarshalToSizedBuffer to reuse the same copying logic. I'll just copy that code instead.

pkg/mimirpb/timeseries_test.go

pkg/mimirpb/timeseries.go

pracucci · 2023-06-06T13:46:06Z

pkg/mimirpb/timeseries.go

+}
+
+// clearUnmarshalData removes cached unmarshalled version of the message.
+func (p *PreallocTimeseries) clearUnmarshalData() {


Distributor.validateSeries() can manipulate ts.Exemplars. I think we need to clear it in that case too.

Would be nice to have this case covered by the integration test too.

Distributor.validateSeries() can manipulate ts.Exemplars. I think we need to clear it in that case too.

Good catch, thank you!

Added handling of exemplars and changed integration test to cover those cases.