Issue-228: Add unorderedSnapshot method to MpscArrayQueue and BaseMps…#229
Conversation
…cLinkedArrayQueue
|
See comment on main ticket on API. |
| { | ||
| final long offset = nextArrayOffset(mask); | ||
| final E[] nextBuffer = (E[]) lvElement(buffer, offset); | ||
| soElement(buffer, offset, null); |
There was a problem hiding this comment.
This will cause GC nepotism
There was a problem hiding this comment.
Thanks, wasn't aware of this issue. Will try to come up with a solution
There was a problem hiding this comment.
I replaced this with a marker object that will cause the iterator to jump to the current consumerBuffer.
| private volatile long consumerIndex; | ||
| protected long consumerMask; | ||
| protected E[] consumerBuffer; | ||
| private volatile E[] volatileConsumerBuffer; |
There was a problem hiding this comment.
why do we need 2 consumer buffer refs?
There was a problem hiding this comment.
Should I make consumerBuffer volatile and add lp/lv/svConsumerBuffer methods instead?
There was a problem hiding this comment.
why does it need to be volatile?
There was a problem hiding this comment.
I wasn't aware that non-volatile reference writes/reads were always atomic. I guess it doesn't need to be volatile. Thanks.
| public List<E> unorderedSnapshot() { | ||
| int length = capacity(); | ||
| List<E> elements = new ArrayList<E>(); | ||
| for (int i = 0; i < length; i++) { |
There was a problem hiding this comment.
You iterate through the whole array, very likely to be many thousands of elements, to find the elements you need
There was a problem hiding this comment.
I'll try to make it less wasteful.
|
If we're going to add this access we will need some very clearly worded java doc on it's limitations and guarantees. |
…MED to ensure iterator doesn't jump to old buffer
|
I've updated to provide iterators instead, and believe the GC nepotism issue should be avoided. I made a minor change to the order of writes when replacing the consumer buffer. I would like to write the consumerBuffer field before the BUFFER_CONSUMED marker is inserted, to ensure the iterator doesn't see the BUFFER_CONSUMED flag along with the old consumerBuffer value, which would cause it to scan the same buffer again. Let me know if this isn't okay, and I'll be happy to revert. I couldn't figure out a way to avoid scanning the entire buffer in the linked queues. I'd like to avoid skipping elements that are still queued, and as far as I can tell we will risk doing so if we follow the consumer index. If the iterator sees a mismatch between the consumerIndex and the current buffer, e.g. because the consumer just replaced consumerBuffer, it may start at the wrong position and end up skipping elements. |
|
Sorry for taking long to re-review, this looks good :-) |
…cLinkedArrayQueue
Resolves #228