LGTM Please consider adding functions to platform.h for atomic operations to make this explicit. Chromium has some in base/atomicops_*.
http://codereview.chromium.org/1047002/diff/1/3 File src/circular-queue-inl.h (right): http://codereview.chromium.org/1047002/diff/1/3#newcode88 src/circular-queue-inl.h:88: Thread::SetThreadLocal(producer_key_, enqueue_pos + kRecordSize); Is the producer supposed to check for full queue if start of chink does not contain kClear. The producer should then use an atomic operation for that? http://codereview.chromium.org/1047002/diff/1/4 File src/circular-queue.cc (right): http://codereview.chromium.org/1047002/diff/1/4#newcode96 src/circular-queue.cc:96: } else { ASSERT(cp->dequeue_chunk_poll_pos % kChunkSize == 0)? Also We are assuming that reading *cp->dequeue_chunk_poll_pos is an atomic operation, right? http://codereview.chromium.org/1047002/diff/1/4#newcode114 src/circular-queue.cc:114: cp->dequeue_pos = NULL; We are assuming that this assignment (*cp->dequeue_chunk_pos = kClear) is an atomic operation, right? http://codereview.chromium.org/1047002/diff/1/5 File src/circular-queue.h (right): http://codereview.chromium.org/1047002/diff/1/5#newcode76 src/circular-queue.h:76: void SetUpProducer(); Please add a comment as to how Enqueue works. http://codereview.chromium.org/1047002/diff/1/5#newcode81 src/circular-queue.h:81: void SetUpConsumer(); Please add a comment to how StartDequeue/FinishDequeue work together. http://codereview.chromium.org/1047002/diff/1/5#newcode103 src/circular-queue.h:103: static const Cell kMagic = -1; // Marks the end of the buffer. kMagic -> kLastChunk? http://codereview.chromium.org/1047002/diff/1/5#newcode104 src/circular-queue.h:104: Please use only kXXX names for compile-time constants. http://codereview.chromium.org/1047002/diff/1/7 File test/cctest/test-circular-queue.cc (right): http://codereview.chromium.org/1047002/diff/1/7#newcode67 test/cctest/test-circular-queue.cc:67: // Fill up the first chunk. Please add a comment why these test loops start at 1 and not 0. http://codereview.chromium.org/1047002/diff/1/7#newcode104 test/cctest/test-circular-queue.cc:104: Please add a comment here on what FlushResidualRecords does. What happens if the producer tries to enqueue after FlushResidualRecords has been called? http://codereview.chromium.org/1047002 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
