WIP: Add memory-bound queue #2013

yurishkuro · 2020-01-12T22:22:11Z

As alternative to channel-based queue in #1985

jpkrohling

Looks promising! @pavolloffay suggested something similar, but his idea was to check the size directly in the linked list, IIRC. This wouldn't require changing the backing buffer, but would require sync access during the enqueue/dequeue operations, I believe.

In any case, would be nice to get his review as well.

jpkrohling · 2020-01-13T08:32:18Z

pkg/queue/mem_bounded_queue.go

+
+func (q *MemoryBoundedQueue) Enqueue(item Sizeable) bool {
+	size := item.Size()
+	for q.memSize+size > q.memCapacity {


This deserves a comment, it took me a while to realize that you are dropping items until space is freed.

jpkrohling · 2020-01-13T08:32:59Z

pkg/queue/mem_bounded_queue.go

+	}
+	// now we're good with memory, but may need to make the queue longer
+	if q.queue.Full() {
+		q.queue.EnsureCapacity(2 * q.queue.Capacity())


Debug statement?

jpkrohling · 2020-01-13T08:40:18Z

pkg/queue/mem_bounded_queue.go

+	Size() int
+}
+
+type MemoryBoundedQueue struct {


The queue isn't meant for concurrent access and this fact needs to be documented, even though there's an explicit "concurrent" version down there.

jpkrohling · 2020-01-13T08:48:53Z

pkg/queue/ring_buffer.go

+}
+
+func NewRingBufferQueue(size int) *RingBufferQueue {
+	return &RingBufferQueue{


Check the boundaries? The min size of the queue is probably 1.

jpkrohling · 2020-01-13T08:50:15Z

pkg/queue/ring_buffer.go

+package queue
+
+type RingBufferQueue struct {
+	size, head, tail int


Size could be uint

jpkrohling · 2020-01-13T08:51:10Z

pkg/queue/mem_bounded_queue.go

+type MemoryBoundedQueue struct {
+	queue         *RingBufferQueue
+	memSize       int
+	memCapacity   int


Both could be uint

jpkrohling · 2020-01-13T08:51:32Z

pkg/queue/mem_bounded_queue.go

+}
+
+func NewMemoryBoundedQueue(initLength int, memCapacity int, onDroppedItem func(item Sizeable)) *MemoryBoundedQueue {
+	return &MemoryBoundedQueue{


Check the boundaries for the given values

jpkrohling · 2020-01-13T08:52:17Z

pkg/queue/mem_bounded_queue.go

+	return ok
+}
+
+func (q *ConcurrentMemoryBoundedQueue) StartConsumers(workers int, consumeFn func(item Sizeable)) {


What happens when workers is 0, or negative?

jpkrohling · 2020-01-13T09:07:28Z

pkg/queue/ring_buffer_test.go

+	}{
+		{items: []int{1, 2}, newCap: 4, expCap: 4, expItems: []int{1, 2, 0, 0}},
+		{items: []int{1, 2, 3, -1}, newCap: 4, expCap: 4, expItems: []int{2, 3, 0, 0}},
+		{items: []int{1, 2, 3, -1, -2, 4, 5}, newCap: 4, expCap: 4, expItems: []int{3, 4, 5, 0}},


I'm getting something wrong... This is would I would expect:

1, 2, 3, 0 (initial state) 2, 3, 0, 0 (after -1) 0, 0, 0, 0 (after -2) 4, 5, 0, 0 (final state)

Unless -2 doesn't mean take two, but take one, for the second time

I think -2 means dequeue and expect the dequeued item 2.

Refer to the comment above: items []int // positive to enqueue, negative to dequeue
and the assertion below: assert.Equal(t, -item, itm)

jpkrohling · 2020-01-13T09:16:48Z

pkg/queue/ring_buffer.go

+
+package queue
+
+type RingBufferQueue struct {


This also require the consumer to take care of concurrency. Would also be good to have a documentation somewhere showing how the types relate to each other. Like:

ConcurrentMemoryBoundedQueue wraps MemoryBoundedQueue, which wraps RingBufferQueue

or we can make these package private and export just the concurrent version.

pavolloffay · 2020-01-13T10:02:51Z

pkg/queue/mem_bounded_queue.go

+	"github.com/uber/jaeger-lib/metrics"
+)
+
+type Sizeable interface {


Is there a core API to get the size? Could we use that instead?

https://golang.org/src/go/types/sizes.go

pavolloffay · 2020-01-13T10:43:03Z

pkg/queue/ring_buffer.go

+
+package queue
+
+type RingBufferQueue struct {


or we can make these package private and export just the concurrent version.

pavolloffay · 2020-01-13T10:48:08Z

pkg/queue/ring_buffer.go

+	if q.Full() {
+		return false
+	}
+	if q.head == -1 {


call to Empty is more idiomatic

yurishkuro · 2020-01-13T16:21:49Z

@jpkrohling this was a quick prototype, if we think it's worth pursuing I can polish it. The basic ring buffer is ready/tested, but the other two types are only written, I never ran them yet.

jpkrohling · 2020-01-13T18:44:37Z

if we think it's worth pursuing I can polish it

Is it possible to benchmark it against the bounded queue that was recently changed? If the performance is similar, I think it's well worth pursuing it!

yurishkuro · 2020-01-13T18:49:45Z

The new queue uses a single mutex, those are usually more efficient than Go channels. But the calls to Span.Size(), which are recursive, will introduce an extra overhead (although it will apply to both implementations). I am skeptical that the queue itself will be in any way a limiting factor in the collector's throughput, and micro-benchmarks won't reflect that anyway.

pavolloffay · 2020-01-14T09:14:59Z

Could we have a benchmark for the concurrent version and compare it against the current impl?

The POC looks good to me, if there is no perf degradation.

jpkrohling · 2020-01-15T13:21:38Z

@yurishkuro will you continue working on this one? I closed the PR that adds the flags (#1985), but just realized that this PR here only adds a new queue type. Should I reopen that and work on the review comments, eventually switching to this new queue, or would you rather have me wait for this one here to be merged first?

yurishkuro · 2020-01-15T15:03:08Z

If someone could take over this pr it would be great, as I will only have time to return to it over the weekend or next week.

The other PR you closed is still needed as it introduces the wiring for the new queue (cli flags, etc).

jpkrohling · 2020-01-16T09:38:07Z

The other PR you closed is still needed as it introduces the wiring for the new queue (cli flags, etc).

I'll reopen that and address the comments. If this here is merged before that one, I'll adapt that to use this new queue, otherwise, I'll send a follow-up PR once the new queue gets merged.

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro · 2020-01-20T17:48:59Z

Benchmark results are puzzling: the mutex+cond version is about x3 slower than channels.

BenchmarkChannelQueue-12    	 1227919	       968 ns/op
BenchmarkRingBuffer-12      	  477895	      2522 ns/op
BenchmarkChannel-12         	 1796756	       651 ns/op
BenchmarkCond-12            	  557828	      2170 ns/op

I added the last two benchmarks which are just raw channel and Cond, without any queueing logic. If I remove the calls to Goshed(), then Cond benchmark becomes slightly faster than channel, although the full queue is still slowed (custom data structure understandably slower than built-in channels).

BenchmarkChannelQueue-12    	14336493	        72.7 ns/op
BenchmarkRingBuffer-12      	 3808519	       318 ns/op
BenchmarkChannel-12         	11800252	        96.9 ns/op
BenchmarkCond-12            	14106967	        84.7 ns/op

The CPU profiles are very similar (with yield), spending most time in the same Cond wait, yet channel is much faster:

channel:

Cond:

Signed-off-by: Yuri Shkuro <ys@uber.com>

albertteoh

Thanks for putting together the benchmarks, that's an interesting insight. Forgive the ignorance in my question; does this mean the existing approach of using the ChannelQueue is preferred given the benchmark results?

albertteoh · 2020-01-22T05:24:05Z

pkg/queue/ring_buffer_test.go

+	}{
+		{items: []int{1, 2}, newCap: 4, expCap: 4, expItems: []int{1, 2, 0, 0}},
+		{items: []int{1, 2, 3, -1}, newCap: 4, expCap: 4, expItems: []int{2, 3, 0, 0}},
+		{items: []int{1, 2, 3, -1, -2, 4, 5}, newCap: 4, expCap: 4, expItems: []int{3, 4, 5, 0}},


I think -2 means dequeue and expect the dequeued item 2.

Refer to the comment above: items []int // positive to enqueue, negative to dequeue
and the assertion below: assert.Equal(t, -item, itm)

albertteoh · 2020-01-22T05:41:30Z

pkg/queue/ring_buffer_test.go

+		expCap   int
+		expItems []int // content of the buffer after EnsureCapacity
+	}{
+		{items: []int{1, 2}, newCap: 4, expCap: 4, expItems: []int{1, 2, 0, 0}},


I would use want instead of exp which I feel is a lot clearer as exp can be an abbreviation for many other words other than expect, or maybe I'm just being too pedantic :)

albertteoh · 2020-01-22T05:47:33Z

pkg/queue/ring_buffer.go

+		return false
+	}
+	if q.Empty() {
+		// empty queue


is this comment necessary?

albertteoh · 2020-01-22T06:43:28Z

pkg/queue/ring_buffer.go

+		return nil, false
+	}
+	var item interface{}
+	if q.head == q.tail {


I wonder if this logic could be simplified if we took the approach of Empty == head == tail, where the tail pointer points to the next "empty" index of the buffer. This means the buffer is never "full", but there is always an empty slot. This also implies that Full means tail == ((head - 1 + size) % size). I think this should do away with the need for conditional logic, but take all this with a grain of salt as I haven't thought this out thoroughly nor tested it.

yurishkuro · 2020-01-22T14:15:54Z

does this mean the existing approach of using the ChannelQueue is preferred given the benchmark results?

That is precisely the open question. I don't think these benchmarks are conclusive, the methodology is flawed. It makes sense to me that plain Cond is faster than channels, considering that profile shows that channels are using the same primitives underneath. I cannot explain why adding yield makes Cond test 3 times slower. I used yield as a standby for a syscall, which the real collector would be doing in both producer and consumer, but real syscalls are likely to dominate the benchmark.

yurishkuro requested a review from jpkrohling January 12, 2020 22:22

yurishkuro requested review from black-adder, objectiser, pavolloffay, tiffon and vprithvi as code owners January 12, 2020 22:22

yurishkuro mentioned this pull request Jan 12, 2020

Added support for dynamic queue sizes #1985

Merged

jpkrohling reviewed Jan 13, 2020

View reviewed changes

pavolloffay reviewed Jan 13, 2020

View reviewed changes

Yuri Shkuro added 2 commits January 19, 2020 20:44

WIP: Add memory-bound queue

2455f1a

Signed-off-by: Yuri Shkuro <ys@uber.com>

refactor

a8256f9

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro force-pushed the mem-bound-queue branch from 34756bf to a8256f9 Compare January 20, 2020 05:51

benchmarks

27a6bda

Signed-off-by: Yuri Shkuro <ys@uber.com>

albertteoh reviewed Jan 22, 2020

View reviewed changes

yurishkuro closed this Aug 14, 2020

yurishkuro deleted the mem-bound-queue branch August 14, 2020 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add memory-bound queue #2013

WIP: Add memory-bound queue #2013

yurishkuro commented Jan 12, 2020

jpkrohling left a comment

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

jpkrohling Jan 13, 2020

albertteoh Jan 22, 2020

jpkrohling Jan 13, 2020

pavolloffay Jan 13, 2020

pavolloffay Jan 13, 2020

pavolloffay Jan 13, 2020

pavolloffay Jan 13, 2020

yurishkuro commented Jan 13, 2020

jpkrohling commented Jan 13, 2020

yurishkuro commented Jan 13, 2020

pavolloffay commented Jan 14, 2020

jpkrohling commented Jan 15, 2020

yurishkuro commented Jan 15, 2020

jpkrohling commented Jan 16, 2020

yurishkuro commented Jan 20, 2020

albertteoh left a comment

albertteoh Jan 22, 2020

albertteoh Jan 22, 2020

albertteoh Jan 22, 2020

albertteoh Jan 22, 2020

yurishkuro commented Jan 22, 2020

WIP: Add memory-bound queue #2013

WIP: Add memory-bound queue #2013

Conversation

yurishkuro commented Jan 12, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jan 13, 2020

jpkrohling commented Jan 13, 2020

yurishkuro commented Jan 13, 2020

pavolloffay commented Jan 14, 2020

jpkrohling commented Jan 15, 2020

yurishkuro commented Jan 15, 2020

jpkrohling commented Jan 16, 2020

yurishkuro commented Jan 20, 2020

albertteoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jan 22, 2020