Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8772004:
Improve GamepadSeqLock impl
|
Created:
9 years ago by dvyukov Modified:
8 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, dpranke-watch+content_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionLock-free GamepadSeqLock
The change
- provides an improved lock-free SeqLock implementation which eliminates any potential blocking of readers.
- provides a higher-level and simpler API as was suggested by Darin.
- ThreadSanitizer report suppressions are replaced with correct synchronization.
- eliminates nasty kMaximumContentionCount and associated histogram.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143903
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 29
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 6
Patch Set 8 : '' #
Total comments: 2
Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 6
Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #
Total comments: 12
Patch Set 15 : #
Total comments: 3
Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Patch Set 20 : #Patch Set 21 : #Messages
Total messages: 44 (0 generated)
PTAL
It really feels like the SeqLock class belongs in src/base/synchronization/ :-) I know there was a discussion about not doing so until we have more than one consumer though! http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:21: class CONTENT_EXPORT GamepadSeqLock { nit: since this is all implemented inline, there is nothing to export. remove CONTENT_EXPORT.
I like the basic idea of the template and the simple ReadTo API. I'm uncomfortable about the additional complexity of the lock free code, because that often leads to rare bugs. It seems that we could use Scott's original implementation inside the ReadTo with an annotation of the T obj to simplify this. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:53: Entry* ent = &entries_[cur % 2]; I'm not sure this additional complexity is necessary now that we have a templated class with T. Why not just internalize the loop from Scott's original seqlock code and keep the ANNOTATE_BENIGN_RACE_SIZED here? Then there's no chance of the end user getting an inconsistent copy of obj because this loop doesn't return until it gets a good version. This would also keep the lock free code much simpler (which is helpful to avoid unpredictable and rare bugs). http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:57: continue; Without some kind of yield or sleep, this can cause livelock via starvation on certain OSes. Even a yield is not enough for some OSes when thread priorities vary. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:61: tmp[i] = NoBarrier_Load(&ent->data_[i]); Why not store a volatile T in ent, and change this loop to: *obj = ent->data; NoBarrier_Load is just a volatile load on all of our platforms. MemoryBarriers already guarantee ordering. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:66: continue; Less likely to cause problems assuming the writer is not updating frequently, but still safer to yield here as well. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:74: void GamepadSeqLock<T>::Write(T const* obj) { nit: consider changing this to const T&? http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:80: memcpy(tmp, reinterpret_cast<char const*>(obj), sizeof(*obj)); Why the extra memcpy? http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:84: NoBarrier_Store(&ent->sequence_, ent->sequence_ + 1); If the writer thread calls Write multiple times while the reader thread is reading, is there still a race on ent->data_? It should be rare enough that it doesn't matter with the gamepad use case. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:88: NoBarrier_Store(&ent->data_[i], tmp[i]); Why not store a volatile T in ent, and change this loop to: ent->obj_ = obj; NoBarrier_Store is also just a volatile store on all of our platforms, and the MemoryBarriers already guarantee ordering.
http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:21: class CONTENT_EXPORT GamepadSeqLock { On 2011/12/01 17:45:16, darin wrote: > nit: since this is all implemented inline, there is nothing to export. remove > CONTENT_EXPORT. Will do. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:53: Entry* ent = &entries_[cur % 2]; On 2011/12/01 19:31:12, jbates wrote: > I'm not sure this additional complexity is necessary now that we have a > templated class with T. Why not just internalize the loop from Scott's original > seqlock code and keep the ANNOTATE_BENIGN_RACE_SIZED here? Then there's no > chance of the end user getting an inconsistent copy of obj because this loop > doesn't return until it gets a good version. This would also keep the lock free > code much simpler (which is helpful to avoid unpredictable and rare bugs). Well, the current algo already has quite substantial complexity, so I do not see significant difference... and in the end nobody proved that it is correct. This algo just has better properties. Annotations must not be used in application code. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:57: continue; On 2011/12/01 19:31:12, jbates wrote: > Without some kind of yield or sleep, this can cause livelock via starvation on > certain OSes. Even a yield is not enough for some OSes when thread priorities > vary. This algorithm does not require yield, it is completely lock-free for readers, as opposed to the current algo which contains a spin-mutex. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:61: tmp[i] = NoBarrier_Load(&ent->data_[i]); On 2011/12/01 19:31:12, jbates wrote: > Why not store a volatile T in ent, and change this loop to: > *obj = ent->data; Because it leads to undefined behavior. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:66: continue; On 2011/12/01 19:31:12, jbates wrote: > Less likely to cause problems assuming the writer is not updating frequently, > but still safer to yield here as well. yield is not required here. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:74: void GamepadSeqLock<T>::Write(T const* obj) { On 2011/12/01 19:31:12, jbates wrote: > nit: consider changing this to const T&? Will do. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:80: memcpy(tmp, reinterpret_cast<char const*>(obj), sizeof(*obj)); On 2011/12/01 19:31:12, jbates wrote: > Why the extra memcpy? I don't know how to implement w/o it. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:84: NoBarrier_Store(&ent->sequence_, ent->sequence_ + 1); On 2011/12/01 19:31:12, jbates wrote: > If the writer thread calls Write multiple times while the reader thread is > reading, is there still a race on ent->data_? Yes. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:88: NoBarrier_Store(&ent->data_[i], tmp[i]); On 2011/12/01 19:31:12, jbates wrote: > Why not store a volatile T in ent, and change this loop to: > ent->obj_ = obj; It leads to undefined behavior, complicates manual verification, leads to poorly documented code, confuses tools and does not allow them to do proper verification.
Thank you for taking this on Dmitriy, appreciated. I agree that it should live in base/synchronization (maybe with an IsPOD<> on T if we have one?) http://codereview.chromium.org/8772004/diff/3002/content/browser/gamepad/game... File content/browser/gamepad/gamepad_provider_unittest.cc (right): http://codereview.chromium.org/8772004/diff/3002/content/browser/gamepad/game... content/browser/gamepad/gamepad_provider_unittest.cc:79: main_message_loop_.RunAllPending(); The intention was to test the collision between reading and writing, which is why it blocked here to ensure the test data had been read. http://codereview.chromium.org/8772004/diff/3002/content/browser/gamepad/game... content/browser/gamepad/gamepad_provider_unittest.cc:91: output.length = 0; this loop seems unnecessary with the ReadTo api. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:51: for (;;) { Could you explain (perhaps in a comment here) why this loop is lock-free & cannot cause live-lock? http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:53: Entry* ent = &entries_[cur % 2]; 2 should be kEntries? (and below) http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:60: for (size_t i = 0; i < kSize; ++i) This seems very subtle. Could you explain (again in a comment perhaps) why it is required that the data are stored as an array of Atomic32s? Is it alright for the buffer to contain qwords? bytes? unaligned data? http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:64: // If the counter is changed, then we've read inconsistend data -> retry. nit: 'inconsistent'
http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:57: continue; On 2011/12/01 21:27:28, dvyukov wrote: > On 2011/12/01 19:31:12, jbates wrote: > > Without some kind of yield or sleep, this can cause livelock via starvation on > > certain OSes. Even a yield is not enough for some OSes when thread priorities > > vary. > > This algorithm does not require yield, it is completely lock-free for readers, > as opposed to the current algo which contains a spin-mutex. After rereading the code 10 times or so :), I think I understand that this can't spin unless the writer thread is spamming Writes. If that's the case, it would be helpful to explain in the comments. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:61: tmp[i] = NoBarrier_Load(&ent->data_[i]); On 2011/12/01 21:27:28, dvyukov wrote: > On 2011/12/01 19:31:12, jbates wrote: > > Why not store a volatile T in ent, and change this loop to: > > *obj = ent->data; > > Because it leads to undefined behavior. I'd be interested in learning how you came to that conclusion. Based on the implementation of NoBarrier_Load/Store, the behavior is identical (just volatile loads and stores). I agree the T::operator= could be bad though because we don't want user code executing here, so allow me to suggest a memcpy instead. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:66: continue; On 2011/12/01 21:27:28, dvyukov wrote: > On 2011/12/01 19:31:12, jbates wrote: > > Less likely to cause problems assuming the writer is not updating frequently, > > but still safer to yield here as well. > > yield is not required here. Is that because it can only happen once per call to Write? Again, would be helpful to see that in the comments. http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:80: memcpy(tmp, reinterpret_cast<char const*>(obj), sizeof(*obj)); On 2011/12/01 21:27:28, dvyukov wrote: > On 2011/12/01 19:31:12, jbates wrote: > > Why the extra memcpy? > > I don't know how to implement w/o it. It appears this is to prevent the compiler from reordering the NoBarrier_Stores (line 88) with the user's stores to obj (before calling Write) (based on strict aliasing rules, which btw we don't compile with but that's another issue). The MemoryBarrier() below (line 85) should be enough to order those, no? I thought GCC does not reorder around assembly ops such as that.
On 2011/12/01 22:36:39, scottmg wrote: > Thank you for taking this on Dmitriy, appreciated. > > I agree that it should live in base/synchronization Well, we can find another place where the component will be useful, use it there and then legally move to it to base/ :) > (maybe with an IsPOD<> on T if we have one?) Humm... I only see one in third_party/WebKit/Source/JavaScriptCore/wtf/TypeTraits.h, I can't use it in content/common, right?
http://codereview.chromium.org/8772004/diff/3002/content/browser/gamepad/game... File content/browser/gamepad/gamepad_provider_unittest.cc (right): http://codereview.chromium.org/8772004/diff/3002/content/browser/gamepad/game... content/browser/gamepad/gamepad_provider_unittest.cc:91: output.length = 0; On 2011/12/01 22:36:39, scottmg wrote: > this loop seems unnecessary with the ReadTo api. The situation is more involved that it appears on first sight. Currently, if the writer is in GetGamepadData(), it means that it started writing into SeqLock but not yet finished. If the readers start reading at this point, it blocks on SeqLock's internal spinlock waiting for the write to complete. When the write completes, the reader reads the new version. With this change, if the writer is in GetGamepadData(), it is not yet started writing into SeqLock, so at this point the reader will just read the old version (all zeroes). Even if we somehow detect when the writer is in process of writing into SeqLock, and start reading at that point, the reader will still read the old version (the write is not yet completed). So I am not sure what to do with this test. We can (1) remove the loop and verify that the reader get either consistent initial version or consistent new version (however in tests we will test only 1 option most of the time), or (2) block the reader till the writer finishes write (return from GamepadSeqLock::Write()) and verify that the reader observes the new version, or (3) leave it as-is - currently it verifies that the new consistent data eventually becomes available to readers.
On 2011/12/01 19:31:12, jbates wrote: > http://codereview.chromium.org/8772004/diff/3002/content/common/gamepad_seqlo... > content/common/gamepad_seqlock.h:88: NoBarrier_Store(&ent->data_[i], tmp[i]); > Why not store a volatile T in ent, and change this loop to: > ent->obj_ = obj; > > NoBarrier_Store is also just a volatile store on all of our platforms, and the > MemoryBarriers already guarantee ordering. Well, the very same way I can conclude that signed overflow has defined semantics by looking at compiler sources. Or I can conclude that accessing an object after freeing it OK under some circumstances by examining memory manager sources. It is not the way to reason about guaranteed semantics. I reason about concurrent accesses and atomic operations by applying C1x/C++11 rules, the standards have documented semantics, seem to capture pre-standard intentions about all these things in a quite precise way and provide a ground for developers and tools to understand each other.
PTAL I've resolved most of the remarks. Does the comments make sense to you? I've removed the memcpy's by copying directly from/to the object: 83 // Breaks strict-aliasing rules. 84 Atomic32* dst = reinterpret_cast<Atomic32*>(obj); 85 // Copy out the entry. 86 for (size_t i = 0; i < kSize; ++i) 87 dst[i] = NoBarrier_Load(&ent->data_[i]); However it requires the object to be multiple of Atomic32: typedef char static_assert_size[sizeof(T) % sizeof(Atomic32) ? -1 : 1]; otherwise I can overwrite random data after the object.
On 2011/12/02 21:40:14, jbates wrote: > After rereading the code 10 times or so :), I think I understand that this can't > spin unless the writer thread is spamming Writes. If that's the case, it would > be helpful to explain in the comments. I hope it was at least an interesting journey :) Sorry that I did not explain it initially.
looks good. http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:77: Entry* ent = &entries_[cur % kSize]; kEntries, not kSize? http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:104: Entry* ent = &entries_[(current_ + 1) % kSize]; kEntries, not kSize? http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... File content/common/gamepad_seqlock_unittest.cc (right): http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... content/common/gamepad_seqlock_unittest.cc:36: for (unsigned i = 0; i < 10000; ++i) { 10000 was causing timeouts on (slower?) tsan bots before. it's possible less contention here will mean it won't not, but consider reducing this a bit (1000?).
PTAL http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:77: Entry* ent = &entries_[cur % kSize]; On 2011/12/05 18:45:06, scottmg wrote: > kEntries, not kSize? Done. http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... content/common/gamepad_seqlock.h:104: Entry* ent = &entries_[(current_ + 1) % kSize]; On 2011/12/05 18:45:06, scottmg wrote: > kEntries, not kSize? Done. http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... File content/common/gamepad_seqlock_unittest.cc (right): http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... content/common/gamepad_seqlock_unittest.cc:36: for (unsigned i = 0; i < 10000; ++i) { On 2011/12/05 18:45:06, scottmg wrote: > 10000 was causing timeouts on (slower?) tsan bots before. it's possible less > contention here will mean it won't not, but consider reducing this a bit > (1000?). Done.
On 2011/12/05 18:45:05, scottmg wrote: > http://codereview.chromium.org/8772004/diff/9005/content/common/gamepad_seqlo... > content/common/gamepad_seqlock_unittest.cc:36: for (unsigned i = 0; i < 10000; > ++i) { > 10000 was causing timeouts on (slower?) tsan bots before. it's possible less > contention here will mean it won't not, but consider reducing this a bit > (1000?). Btw, if you ever run tests under ThreadSanitizer you may try compiler-based version: http://www.chromium.org/developers/how-tos/using-valgrind/threadsanitizer/gcc... It's currently in sort of beta version and we are in process of setting up bots that will potentially replace Valgrind-based. On my machine compiler-based ThreadSanitizer executes GamepadSeqLockTest.ManyThreads with N=10000 in just 180 ms.
On 2011/12/07 09:35:50, dvyukov wrote: > PTAL lgtm, but darin and/or jbates will need to review for commit.
lgtm http://codereview.chromium.org/8772004/diff/16001/content/common/gamepad_seql... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/16001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:115: // Switch the current object. The reason for having both of these stores with a memory barrier is a bit subtle. After thinking it through, it looks like it's necessary to avoid spinning in the reader thread on line 82. (I was thinking it would be nice to reduce the number of memory barriers for simplicity and performance, such as store current_; store ent->sequence_; barrier; but it looks like that could lead to spinning in the reader thread if the writer was swapped out between the stores.)
http://codereview.chromium.org/8772004/diff/16001/content/common/gamepad_seql... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/16001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:115: // Switch the current object. On 2011/12/08 01:02:36, jbates wrote: > The reason for having both of these stores with a memory barrier is a bit > subtle. After thinking it through, it looks like it's necessary to avoid > spinning in the reader thread on line 82. (I was thinking it would be nice to > reduce the number of memory barriers for simplicity and performance, such as > store current_; store ent->sequence_; barrier; but it looks like that could lead > to spinning in the reader thread if the writer was swapped out between the > stores.) Missed memory fences usually only increase complexity :) It's much easier to reason when reorderings are restricted. As for performance, store-release is completely free on x86. The fences that we want to remove for performance reasons are on lines 88 and 107. The former can easily increase reading cost 10x. However it is unsolvable with current atomic API (to solve that we need either load-load/store-store fences or ReleaseLoads_Load and AcquireStores_Store) - all are no-ops on x86).
I've resynched my client and added back 'common/gamepad_seqlock_unittest.cc' to content/content_tests.gypi.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvyukov@chromium.org/8772004/31002
Presubmit check for 8772004-31002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/common/gamepad_seqlock_unittest.cc,content/browser/gamepad/gamepad_provider_unittest.cc,content/browser/gamepad/gamepad_provider.cc,content/renderer/gamepad_shared_memory_reader.cc,content/common/gamepad_hardware_buffer.h,content/common/gamepad_seqlock.cc,content/common/gamepad_seqlock.h Presubmit checks took 2.2s to calculate.
http://codereview.chromium.org/8772004/diff/32004/content/browser/gamepad/gam... File content/browser/gamepad/gamepad_provider_unittest.cc (right): http://codereview.chromium.org/8772004/diff/32004/content/browser/gamepad/gam... content/browser/gamepad/gamepad_provider_unittest.cc:22: explicit MockDataFetcher(WebGamepads const& test_data) { nit: please stick with the more canonical "const Foo&" syntax. http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seql... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seql... content/common/gamepad_seqlock.h:70: void GamepadSeqLock<T>::ReadTo(T* obj) { nit: Can't this be a non-template helper function that just takes an Atomic32* pointer? It would probably be good to make GamepadSeqLock<T> inherit from GamepadSeqLockBase that is non-templatized. Then, implement the GamepadSeqLockBase methods in a .cc file. I'd also declare the GamepadSeqLockBase class in the content::internal:: namespace. http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seql... content/common/gamepad_seqlock.h:99: void GamepadSeqLock<T>::Write(T const& obj) { Ditto.
PTAL http://codereview.chromium.org/8772004/diff/32004/content/browser/gamepad/gam... File content/browser/gamepad/gamepad_provider_unittest.cc (right): http://codereview.chromium.org/8772004/diff/32004/content/browser/gamepad/gam... content/browser/gamepad/gamepad_provider_unittest.cc:22: explicit MockDataFetcher(WebGamepads const& test_data) { On 2011/12/08 22:10:48, darin wrote: > nit: please stick with the more canonical "const Foo&" syntax. Done. http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seql... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seql... content/common/gamepad_seqlock.h:70: void GamepadSeqLock<T>::ReadTo(T* obj) { On 2011/12/08 22:10:48, darin wrote: > nit: Can't this be a non-template helper function that just takes an Atomic32* > pointer? > > It would probably be good to make GamepadSeqLock<T> inherit from > GamepadSeqLockBase that is non-templatized. Then, implement the > GamepadSeqLockBase methods in a .cc file. I'd also declare the > GamepadSeqLockBase class in the content::internal:: namespace. Done. http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seql... content/common/gamepad_seqlock.h:99: void GamepadSeqLock<T>::Write(T const& obj) { On 2011/12/08 22:10:48, darin wrote: > Ditto. Done.
ping On Tue, Dec 13, 2011 at 4:46 PM, <dvyukov@chromium.org> wrote: > PTAL > > > > http://codereview.chromium.**org/8772004/diff/32004/** > content/browser/gamepad/**gamepad_provider_unittest.cc<http://codereview.chromium.org/8772004/diff/32004/content/browser/gamepad/gamepad_provider_unittest.cc> > File content/browser/gamepad/**gamepad_provider_unittest.cc (right): > > http://codereview.chromium.**org/8772004/diff/32004/** > content/browser/gamepad/**gamepad_provider_unittest.cc#**newcode22<http://codereview.chromium.org/8772004/diff/32004/content/browser/gamepad/gamepad_provider_unittest.cc#newcode22> > content/browser/gamepad/**gamepad_provider_unittest.cc:**22: explicit > MockDataFetcher(WebGamepads const& test_data) { > On 2011/12/08 22:10:48, darin wrote: > >> nit: please stick with the more canonical "const Foo&" syntax. >> > > Done. > > > http://codereview.chromium.**org/8772004/diff/32004/** > content/common/gamepad_**seqlock.h<http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seqlock.h> > File content/common/gamepad_**seqlock.h (right): > > http://codereview.chromium.**org/8772004/diff/32004/** > content/common/gamepad_**seqlock.h#newcode70<http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seqlock.h#newcode70> > content/common/gamepad_**seqlock.h:70: void GamepadSeqLock<T>::ReadTo(T* > obj) { > On 2011/12/08 22:10:48, darin wrote: > >> nit: Can't this be a non-template helper function that just takes an >> > Atomic32* > >> pointer? >> > > It would probably be good to make GamepadSeqLock<T> inherit from >> GamepadSeqLockBase that is non-templatized. Then, implement the >> GamepadSeqLockBase methods in a .cc file. I'd also declare the >> GamepadSeqLockBase class in the content::internal:: namespace. >> > > Done. > > > http://codereview.chromium.**org/8772004/diff/32004/** > content/common/gamepad_**seqlock.h#newcode99<http://codereview.chromium.org/8772004/diff/32004/content/common/gamepad_seqlock.h#newcode99> > content/common/gamepad_**seqlock.h:99: void GamepadSeqLock<T>::Write(T > const& obj) { > On 2011/12/08 22:10:48, darin wrote: > >> Ditto. >> > > Done. > > http://codereview.chromium.**org/8772004/<http://codereview.chromium.org/8772... >
I've resynced the client. PTAL.
High level, looking good. http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... File content/common/gamepad_seqlock.cc (right): http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.cc:16: entries_[i] = (Entry*)((char*)entries + i * entSize); guidelines prohibit c-style casts I think. http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:48: : Base(entries_, kSize) { hrmm, so I see you haven't tried to compile this yet :p http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:51: void ReadTo(T* obj) { Would be helpful to see the properties and guarantees of ReadTo in the comments. For example: - if ReadTo occurs in the middle of Write (on another thread), ReadTo gets the old data (not the new data being written by the other thread). - ReadTo never spins unless the writer thread is calling Write multiple times before ReadTo completes. http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:63: typedef char static_assert_size[sizeof(T) % sizeof(Atomic32) ? -1 : 1]; could use the macro for this in base/basictypes.h: COMPILE_ASSERT(sizeof(T) % sizeof(Atomic32) == 0) http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:65: struct Entry : Base::Entry{ I was confused by "Entry" since it is the same name as the derived Entry type. A different name here would help. http://codereview.chromium.org/8772004/diff/46001/tools/valgrind/tsan/ignores... File tools/valgrind/tsan/ignores.txt (right): http://codereview.chromium.org/8772004/diff/46001/tools/valgrind/tsan/ignores... tools/valgrind/tsan/ignores.txt:65: fun:*base*subtle*NoBarrier_Load* Looks like my trace_event code will need this.. hopefully it goes in soon!
All done. PTAL. http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... File content/common/gamepad_seqlock.cc (right): http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.cc:16: entries_[i] = (Entry*)((char*)entries + i * entSize); On 2012/02/17 23:29:52, jbates wrote: > guidelines prohibit c-style casts I think. Done. http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... File content/common/gamepad_seqlock.h (right): http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:48: : Base(entries_, kSize) { On 2012/02/17 23:29:52, jbates wrote: > hrmm, so I see you haven't tried to compile this yet :p Compile?.. What for? :) Done... hope I guessed correctly what error compilation produces on this code... http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:51: void ReadTo(T* obj) { On 2012/02/17 23:29:52, jbates wrote: > Would be helpful to see the properties and guarantees of ReadTo in the comments. > For example: > - if ReadTo occurs in the middle of Write (on another thread), ReadTo gets the > old data (not the new data being written by the other thread). > - ReadTo never spins unless the writer thread is calling Write multiple times > before ReadTo completes. Done. http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:63: typedef char static_assert_size[sizeof(T) % sizeof(Atomic32) ? -1 : 1]; On 2012/02/17 23:29:52, jbates wrote: > could use the macro for this in base/basictypes.h: > COMPILE_ASSERT(sizeof(T) % sizeof(Atomic32) == 0) Done. http://codereview.chromium.org/8772004/diff/46001/content/common/gamepad_seql... content/common/gamepad_seqlock.h:65: struct Entry : Base::Entry{ On 2012/02/17 23:29:52, jbates wrote: > I was confused by "Entry" since it is the same name as the derived Entry type. A > different name here would help. Done. http://codereview.chromium.org/8772004/diff/46001/tools/valgrind/tsan/ignores... File tools/valgrind/tsan/ignores.txt (right): http://codereview.chromium.org/8772004/diff/46001/tools/valgrind/tsan/ignores... tools/valgrind/tsan/ignores.txt:65: fun:*base*subtle*NoBarrier_Load* On 2012/02/17 23:29:52, jbates wrote: > Looks like my trace_event code will need this.. hopefully it goes in soon! Yes, we already see the reports: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28t...
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvyukov@chromium.org/8772004/53001
Presubmit check for 8772004-53001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/common/gamepad_seqlock_unittest.cc,content/browser/gamepad/gamepad_provider_unittest.cc,content/browser/gamepad/gamepad_provider.cc,content/renderer/gamepad_shared_memory_reader.cc,content/common/gamepad_hardware_buffer.h,content/common/gamepad_seqlock.cc,content/common/gamepad_seqlock.h Presubmit checks took 2.0s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
On 2012/02/21 17:36:01, I haz the power (commit-bot) wrote: > Presubmit check for 8772004-53001 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit Messages ** > If this change has an associated bug, add BUG=[bug number]. > > If this change requires manual test instructions to QA team, add > TEST=[instructions]. > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for: > content/common/gamepad_seqlock_unittest.cc,content/browser/gamepad/gamepad_provider_unittest.cc,content/browser/gamepad/gamepad_provider.cc,content/renderer/gamepad_shared_memory_reader.cc,content/common/gamepad_hardware_buffer.h,content/common/gamepad_seqlock.cc,content/common/gamepad_seqlock.h > > Presubmit checks took 2.0s to calculate. > > Was the presubmit check useful? Please send feedback & hate mail to > maruel@chromium.org! darin, may you please approve this CL, I need LGTM from the OWNER and you seem to be an owner for all the files.
On 2012/02/22 08:04:51, dvyukov wrote: > On 2012/02/21 17:36:01, I haz the power (commit-bot) wrote: > > Presubmit check for 8772004-53001 failed and returned exit status 1. > > > > Running presubmit commit checks ... > > > > ** Presubmit Messages ** > > If this change has an associated bug, add BUG=[bug number]. > > > > If this change requires manual test instructions to QA team, add > > TEST=[instructions]. > > > > ** Presubmit ERRORS ** > > Missing LGTM from an OWNER for: > > > content/common/gamepad_seqlock_unittest.cc,content/browser/gamepad/gamepad_provider_unittest.cc,content/browser/gamepad/gamepad_provider.cc,content/renderer/gamepad_shared_memory_reader.cc,content/common/gamepad_hardware_buffer.h,content/common/gamepad_seqlock.cc,content/common/gamepad_seqlock.h > > > > Presubmit checks took 2.0s to calculate. > > > > Was the presubmit check useful? Please send feedback & hate mail to > > maruel@chromium.org! > > > darin, > > may you please approve this CL, I need LGTM from the OWNER and you seem to be an > owner for all the files. Hey, May somebody from the following list approve this CL as OWNER, plz? avi@ ben@ brettw@ darin@ jam@
Please pick one person and ask them rather than spamming all of the most already-overloaded reviewers on the team.
Hi, Could I dredge this up from the ancient past to try to get it landed? I believe there are timeouts being caused on the WebKit bots due to the current implementation starving in test code. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=pla...
On 2012/06/19 19:31:48, scottmg wrote: > Hi, Could I dredge this up from the ancient past to try to get it landed? I > believe there are timeouts being caused on the WebKit bots due to the current > implementation starving in test code. It's waiting for darin review. > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=pla...
LGTM https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... File content/common/gamepad_seqlock.cc (right): https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... content/common/gamepad_seqlock.cc:13: const size_t entSize = sizeof(BaseEntry) + sizeof(Atomic32) * size; nit: entSize -> ent_size https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... File content/common/gamepad_seqlock.h (right): https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... content/common/gamepad_seqlock.h:49: : internal::GamepadSeqLockBase(entries_, kSize) { nit: indent by 4 spaces https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... content/common/gamepad_seqlock.h:74: struct Entry : Base::BaseEntry{ nit: insert space before "{"
On 2012/06/19 21:14:35, darin wrote: > LGTM > > https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... > File content/common/gamepad_seqlock.cc (right): > > https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... > content/common/gamepad_seqlock.cc:13: const size_t entSize = sizeof(BaseEntry) + > sizeof(Atomic32) * size; > nit: entSize -> ent_size > > https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... > File content/common/gamepad_seqlock.h (right): > > https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... > content/common/gamepad_seqlock.h:49: : internal::GamepadSeqLockBase(entries_, > kSize) { > nit: indent by 4 spaces > > https://chromiumcodereview.appspot.com/8772004/diff/53001/content/common/game... > content/common/gamepad_seqlock.h:74: struct Entry : Base::BaseEntry{ > nit: insert space before "{" All done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvyukov@chromium.org/8772004/82002
Try job failure for 8772004-82002 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvyukov@chromium.org/8772004/82004
Change committed as 143903
On 2012/06/19 19:31:48, scottmg wrote: > Hi, Could I dredge this up from the ancient past to try to get it landed? I > believe there are timeouts being caused on the WebKit bots due to the current > implementation starving in test code. > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=pla... Landed.
Thanks Dmitriy! On Mon, Jun 25, 2012 at 6:51 AM, <dvyukov@google.com> wrote: > On 2012/06/19 19:31:48, scottmg wrote: >> >> Hi, Could I dredge this up from the ancient past to try to get it landed? >> I >> believe there are timeouts being caused on the WebKit bots due to the >> current >> implementation starving in test code. > > > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=pla... > > Landed. > > > https://chromiumcodereview.appspot.com/8772004/
On 2012/06/25 16:54:25, scottmg wrote: > Thanks Dmitriy! > > On Mon, Jun 25, 2012 at 6:51 AM, <mailto:dvyukov@google.com> wrote: > > On 2012/06/19 19:31:48, scottmg wrote: > >> > >> Hi, Could I dredge this up from the ancient past to try to get it landed? > >> I > >> believe there are timeouts being caused on the WebKit bots due to the > >> current > >> implementation starving in test code. > > > > > > > > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=pla... > > > > Landed. > > > > > > https://chromiumcodereview.appspot.com/8772004/ FTR, This patch was reverted. Here is another reincarnation: https://chromiumcodereview.appspot.com/10704041/ |
