Optimize mergeStream

[codesome]

In my quest of optimizing query performance, I found that result.append was taking a significant amount of time in mergeStream (2s+ of around 4s of mergeStream).

The query and data in action is described here prometheus/prometheus#5707 (comment)

In this PR I have inlined the appending logic with the merging logic. This saves anywhere from 1s-2.5s for the above data.

[bboreham]

suggests this comment wasn't true?

// append, reset, hasNext, next, atTime etc are all inlined in go1.11.	

[bboreham]

I'd like some more information on why the repeated code cannot be a function, before approving.

[codesome]

Ah yup, then I guess the improvement was by efficient use of result slice I believe. There are fewer accesses to the result slice (only when going to next batch as opposed to accessing it for every sample), and direct assignment of values to the batch.

Also avoiding result = append(result, promchunk.Batch{}) for the first few Batch (till capacity) and just setting Index and Length to 0 also seemed to help.

[codesome]

I'd like some more information on why the repeated code cannot be a function, before approving.

I will club repeated code into functions and see if there is any difference in perf. If Go is inlining them, I believe there shouldn't be much difference.

[bboreham]

This comment that was there before:

// Caller must guarantee result is big enough.  Return value will always be a	

Can this comment be relied on? Seems the code would be a lot simpler if it were.

[codesome]

I considered that, but here are the reasons why I infer result can fall short.

1. mergeStreams is also called here

   cortex/pkg/querier/batch/merge.go

   Line 117 in 795dd59

   c.batchesBuf = mergeStreams(c.batches, nextBatch, c.batchesBuf, size)

   so the guarantee asked by mergeBatches need not hold true here.

2. Tests failed if I assume that, because it was thought result length would be taken care of?

[bboreham]

At least, the comment should be changed.
Right now I feel the code is too complicated for me to approve. Possibly if I spent a few hours poking at it I would understand what is going on.

[codesome]

I will try to simplify and add more comments to make it more readable.

At least, the comment should be changed.

I was thinking we can remove mergeBatch totally as we are not using it anywhere.

➜  cortex git:(merge-stream) ✗ grep -rni mergeBatches
Binary file .pkg/linux_amd64/github.com/cortexproject/cortex/pkg/querier/batch.a matches
pkg/querier/batch/stream.go:50:// mergeBatches assumes the contents of batches are overlapping and unsorted.
pkg/querier/batch/stream.go:53:// slice pointing to the same underly array as result, allowing mergeBatches
pkg/querier/batch/stream.go:55:func mergeBatches(batches batchStream, result batchStream, size int) batchStream {
pkg/querier/batch/stream.go:66:		left := mergeBatches(batches[n:], result[n:], size)
pkg/querier/batch/stream.go:67:		right := mergeBatches(batches[:n], result[:n], size)
pkg/querier/batch/stream_test.go:48:			result = mergeBatches(tc.input, result, promchunk.BatchSize)

[codesome]

I have simplified some part of mergeStreams (the initilization at the beginning) and also added some comments. Additionally, removed mergeBatchs as it was not used anywhere.

[tomwilkie]

We're had this running for ~2 weeks now, no problems observed.