refactor: reuse the slice to optimize

[YaoZengzeng]

ref: prometheus-junkyard/tsdb#676

@bwplotka @krasi-georgiev

Signed-off-by: YaoZengzeng [email protected]

[YaoZengzeng]

@krasi-georgiev Also optimize seriesHashmap.del() and baseChunkSeries.Next() with same way.

[YaoZengzeng]

@bwplotka @krasi-georgiev

After reading the code more comprehensively, I think the slice of MemPostings.Delete() should not be reused.

Because MemPostings.Get(name, value) will return the Postings for querying whose underlying slice share same memory with MemPostings.m[name][value].

If we reuse the slice in deleting, the Postings may return duplicate series ref.

seriesHashmap.del() and baseChunkSeries.Next() could reuse the slice safely.

[csmarchbanks]

Why did you remove the short circuit? The short circuit doesn't allocate anymore, but still avoids extra append calls and assignment.

[YaoZengzeng]

@csmarchbanks Actually the short circuit still need to iterate the p.m[n][l] one loop and if found==true, it need to allocate and iterate another loop to fullfill repl. But if we reuse the p.m[n][l], only one loop is needed and no allocation at all under any conditions.

However as I commented above, reuse the slice will result a sneaky bug. So I'll rollback this code change.

[csmarchbanks]

This code is inside of a Lock, so reusing the slice should not cause a problem.

I will rephrase my concern, the previous code did a loop with only gets in order to quickly see if anything changed. The new code only does one loop, but that loop is more expensive since it also does an append for every element that was not deleted. I am wondering if you have any benchmarks to show that doing the extra operations is ok.

[YaoZengzeng]

The lock only protect MemPostings.m, but not the memory underlying it.

For example, if MemPosting.m["foo"]["bar"] is {1, 2, 3, 4, 5} and through MemPosting.Get("foo", "bar") we will get a ListPostings whose list field share the same memory with MemPosting.m["foo"]["bar"].

Then MemPosting.Delete() delete some ref, 2, 4 for example. If we reuse the slice, the underlying memory will be {1, 3, 5, 4, 5}. Finally, the iterating of ListPostings will not be correct.

[csmarchbanks]

👍 That make sense. Also, that is an insidious potential bug that does not fail any tests.

[YaoZengzeng]

So I'll delete this part of slice reuse.

And I'd like to add a test for this insidious case if necessary :)

[csmarchbanks]

And I'd like to add a test for this insidious case if necessary :)

Please do, or else I will in a separate PR. :) It doesn't look like MemPostings.Delete has any package level testing right now.

[YaoZengzeng]

Another PR will be fine.

[YaoZengzeng]

@krasi-georgiev @bwplotka @csmarchbanks

It's proved that reuse the slice for memPostings.Delete() is not correct and the code change has been rollbacked.

But seriesHashmap.del() and baseChunkSeries.Next() could reuse the slice safely.

PTAL.

[krasi-georgiev]

@YaoZengzeng thanks,
I just run the BenchmarkQueries and it didn't show any difference in the allocs and total bytes.
Could you check if we have another func that shows the improvment or write one to see what are the real gains by this change.

[YaoZengzeng]

@krasi-georgiev Maybe the referenced functions are not called frequently, so the optimization is not obvious. But reusing the slice that way is appropriate and more efficient in theory, right? :)

[krasi-georgiev]

yep 100% correct, but sometime the theory is not what we see in practice.

It should be simple enough to write some benchmark that shows the difference and it should be useful for future tests.
Would you have the time to write it?
If yes please use only public API interfaces as these are less likely to change.

[YaoZengzeng]

Yes, I'd like to research it in more depth :)

[krasi-georgiev]

Thanks, Appreciated. waiting for the results.

[krasi-georgiev]

@YaoZengzeng any progress on this?

[krasi-georgiev]

ping @YaoZengzeng

[YaoZengzeng]

@krasi-georgiev Sorry for the delay, but really busy these days. 😂

I'll move on once I have time :)

[krasi-georgiev]

@YaoZengzeng are you still busy? any chance to look into this anytime soon?

[beorn7]

In the bug scrub, we decided to close this for lack of activity. If somebody wants to pick this up, we'll appreciate it. Next step, as discussed above, would be to verify the improvement with a benchmark.