Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9605015:
Add a SharedMemSynchronizer class. (Closed)
|
Created:
8 years, 9 months ago by tommi (sloooow) - chröme Modified:
8 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a CrossProcessNotification class.
This class will be used to synchronize multiple audio objects with minimal thread and IPC socket usage.
TEST=Several tests included. Run media unittests.
BUG=114699
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129263
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129405
Patch Set 1 #
Total comments: 58
Patch Set 2 : Address review comments. I'll do the file name change later so that you can see the diffs clearer. #
Total comments: 24
Patch Set 3 : Address review comments #Patch Set 4 : Address review comments #Patch Set 5 : Change file names #Patch Set 6 : address review comments from Ami #
Total comments: 20
Patch Set 7 : Changed class and file names according to Ami's wishes. #
Total comments: 10
Patch Set 8 : Address review comments #
Total comments: 2
Patch Set 9 : Address comments #Patch Set 10 : Disabled two tests before relanding #
Messages
Total messages: 20 (0 generated)
nice! for this class we should follow base's convention for multi-OS impls: media/audio/shared_mem_synchronizer.h media/audio/shared_mem_synchronizer.cc media/audio/shared_mem_synchronizer_posix.cc media/audio/shared_mem_synchronizer_win.cc the media/audio/OS subfolders are there primarily because implementing the interfaces under media/audio/ require so much code :) https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... File media/audio/shared_mem_synchronizer.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:4: #include "media/audio/shared_mem_synchronizer.h" blank line above https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:30: SharedMemSynchronizer::SharedMemSynchronizer() { nit: typically collapse these one-liners {} https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:35: SharedMemSynchronizer::~SharedMemSynchronizer() { nit: typically collapse these one-liners {} https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:93: DCHECK(last_signaled < count); DCHECK_LE https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:94: #ifndef NDEBUG if we added curly braces around that for-loop I'd hope that w/o this a compiler would be smart enough not to spin for no reason! https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:102: memset(&sockets[0], 0, count * sizeof(struct pollfd)); // NOLINT is the NOLINT for sizeof(struct pollfd)? would sizeof(sockets[0]) work? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:104: for (size_t i = last_signaled + 1; i < count; ++i) { I'd have a smallish comment here describing what's going on (prevent starvation by constructing |sockets| such that it starts at |(last_signaled + 1) % count| https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:128: NOTREACHED() << "err: " << errno; something more descriptive? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... File media/audio/shared_mem_synchronizer.h (right): https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:24: // to know when the other end has called Signal(). I know I can look at unit tests, but I think we could use a tiny code snippet here to explain how I'm supposed to use this class for example we've got the following: * default ctor * ctor w/ IPCHandles * ShareToProcess() * Create() which functions do I call, when should I call them, and in which processes? for example base/synchronization/condition_variable.h has a nice intro on usage & gotchas https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:33: SharedMemSynchronizer(); docs on ctors considering they can be different https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:37: void Signal(); docs https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:48: // Initialized a pair of SharedMemSynchronizer instances. Note that this can typo? Initialized -> Initialize ? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:50: static bool Create(SharedMemSynchronizer* a, SharedMemSynchronizer* b); would it be better to rename this Initialize()? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:58: // TODO(tommi): Support a way to abort the wait. I guess starvation would still occur if a client is constantly recreating their WaitForMultiple object by adding/removing objects from the end of the array between calls to Wait() (last_ is always reset to zero) we could do something like shuffle the synchronizers_ pointer inside of Wait() but that seems rather tricky haha https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:73: protected: this class doesn't appear to have subclasses -- private? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... File media/audio/shared_mem_synchronizer_unittest.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer_unittest.cc:82: struct FlagArray { AFAIK std::vector<bool> is implemented as a bit array there's also std::bitset but not sure about the cross-platformability of that one! https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer_unittest.cc:249: ASSERT_TRUE(signals.size() >= kCount); assert_ge? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer_unittest.cc:256: size_t repeats = /*kCount * */ 1; commented out code? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... File media/audio/win/shared_mem_synchronizer_win.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:4: #include "media/audio/shared_mem_synchronizer.h" blank line above https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:22: DCHECK(::WaitForSingleObject(mine_, 0) == WAIT_TIMEOUT); can we DCHECK_EQ it up here + below? also is this a dcheck or are we doing something here? msdn says WAIT_TIMEOUT is returned if the object has been signaled, so how about we add some logging here so it's clearer what's going on if this were to fail (Signal() called multiple times -- i.e., aggressive chess player!) https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:23: ::SetEvent(mine_); FYI this could fail -- but not sure what we'd do to handle that case perhaps a CHECK() would suffice so we get some crash dumps to verify that it's possible to fail in the first place, WDYT? NOTE: I'm not a windows programming guru so let me know if the common practice is to ignore the BOOL/HRESULT values for functions like these ones :) https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:29: DCHECK(wait == WAIT_OBJECT_0); ditto https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:30: ::ResetEvent(other_); ditto on failing here + way below https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:43: if (::DuplicateHandle(our_process, mine_, process, handle_1, 0, FALSE, nit: return early on fail instead of nesting ifs and using |success| variable? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:171: DCHECK(last_signaled < count); ditto here+below https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:193: // Assuming equally busy events, this approach distributes the priority sanity check: aren't we using the same trick in posix land? https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:206: if (count <= MAXIMUM_WAIT_OBJECTS) { stepping back a bit is this something we want/need to support now? anyway, it seems like we'd hit quite the performance cliff once we reach MAXIMUM_WAIT_OBJECTS by thrashing thread creation+tear down if we assumed thread creation + hand off of the next 63+ events took N milliseconds, what if we called WFMO() w/ a wait time of <N on chunks of 64 events? If WFMO() times out we would rebuild the handles array using the next chunk of 64 events, etc... I have a feeling the resulting code would be easier to understand as well as avoid creating/stopping threads in what I believe will ultimately be a tight loop -- but again I don't think this is an area we need to optimize until proven necessary :) https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:218: // next thread. etc etc. bit confusing to read perhaps reword to include something along the lines of "recursively create threads so that each thread waits on at most 62 events including one event for when a child thread signals completion and one event for when a parent thread must stop a child thread" I would also include a mention to MAXIMUM_WAIT_OBJECTS and/or MSDN link for the non-windows-folk out there :) https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:253: NOTREACHED(); LOG? how severe is this? note that in release mode this does nothing as does your DCHECK below
https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... File media/audio/shared_mem_synchronizer.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:4: #include "media/audio/shared_mem_synchronizer.h" On 2012/03/08 02:37:08, scherkus wrote: > blank line above Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:30: SharedMemSynchronizer::SharedMemSynchronizer() { On 2012/03/08 02:37:08, scherkus wrote: > nit: typically collapse these one-liners {} Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:35: SharedMemSynchronizer::~SharedMemSynchronizer() { On 2012/03/08 02:37:08, scherkus wrote: > nit: typically collapse these one-liners {} Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:93: DCHECK(last_signaled < count); On 2012/03/08 02:37:08, scherkus wrote: > DCHECK_LE changed to DCHECK_LT https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:94: #ifndef NDEBUG On 2012/03/08 02:37:08, scherkus wrote: > if we added curly braces around that for-loop I'd hope that w/o this a compiler > would be smart enough not to spin for no reason! Done. Do we even need the curly braces? (the compiler should be smart enough to realize that ++i doesn't have any side effect) https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:102: memset(&sockets[0], 0, count * sizeof(struct pollfd)); // NOLINT On 2012/03/08 02:37:08, scherkus wrote: > is the NOLINT for sizeof(struct pollfd)? > > would sizeof(sockets[0]) work? Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:104: for (size_t i = last_signaled + 1; i < count; ++i) { On 2012/03/08 02:37:08, scherkus wrote: > I'd have a smallish comment here describing what's going on (prevent starvation > by constructing |sockets| such that it starts at |(last_signaled + 1) % count| Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.cc:128: NOTREACHED() << "err: " << errno; On 2012/03/08 02:37:08, scherkus wrote: > something more descriptive? Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... File media/audio/shared_mem_synchronizer.h (right): https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:24: // to know when the other end has called Signal(). On 2012/03/08 02:37:08, scherkus wrote: > I know I can look at unit tests, but I think we could use a tiny code snippet > here to explain how I'm supposed to use this class > > for example we've got the following: > * default ctor > * ctor w/ IPCHandles > * ShareToProcess() > * Create() > > which functions do I call, when should I call them, and in which processes? for > example base/synchronization/condition_variable.h has a nice intro on usage & > gotchas Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:33: SharedMemSynchronizer(); On 2012/03/08 02:37:08, scherkus wrote: > docs on ctors considering they can be different Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:37: void Signal(); On 2012/03/08 02:37:08, scherkus wrote: > docs Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:48: // Initialized a pair of SharedMemSynchronizer instances. Note that this can On 2012/03/08 02:37:08, scherkus wrote: > typo? > > Initialized -> Initialize ? Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:50: static bool Create(SharedMemSynchronizer* a, SharedMemSynchronizer* b); On 2012/03/08 02:37:08, scherkus wrote: > would it be better to rename this Initialize()? I kinda borrowed this name from SyncSocket except that I didn't call it CreatePair... I guess I could call this InitializePair to make it even clearer. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:58: // TODO(tommi): Support a way to abort the wait. On 2012/03/08 02:37:08, scherkus wrote: > I guess starvation would still occur if a client is constantly recreating their > WaitForMultiple object by adding/removing objects from the end of the array > between calls to Wait() (last_ is always reset to zero) Yes. However, adding/removing to/from the array should be a much rarer case than synchronizing the shared memory (audio buffers), so at least the problem is reduced in that case. To rectify that I guess we could offer a way to reset the synchronizer array instead of recreating the WaitForMultiple instance, but I don't think this is going to be a problem in practice (see below). > > we could do something like shuffle the synchronizers_ pointer inside of Wait() > but that seems rather tricky haha I wrote this to support the scenario of multiple SharedMemSynchronizer instances. But then came up with an alternate solution that is considerably faster and can use as few as a single pair of synchronizers for thousands of shared mem objects. See the "TwoSynchronizersTwoThreads1000Signals" test. So, in practice, the number of SharedMemSynchronizers will likely be 1 per process for audio in the most common case. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer.h:73: protected: On 2012/03/08 02:37:08, scherkus wrote: > this class doesn't appear to have subclasses -- private? Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... File media/audio/shared_mem_synchronizer_unittest.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer_unittest.cc:82: struct FlagArray { On 2012/03/08 02:37:08, scherkus wrote: > AFAIK std::vector<bool> is implemented as a bit array > > there's also std::bitset but not sure about the cross-platformability of that > one! I did consider that but the difference is this requirement: "you must be able to map this class to a piece of shared memory". I can't make assumptions about the internal structure of vector<bool>. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer_unittest.cc:249: ASSERT_TRUE(signals.size() >= kCount); On 2012/03/08 02:37:08, scherkus wrote: > assert_ge? Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/shared_mem_... media/audio/shared_mem_synchronizer_unittest.cc:256: size_t repeats = /*kCount * */ 1; On 2012/03/08 02:37:08, scherkus wrote: > commented out code? Removed. I thought this would help make the comment above clearer but I guess it just made it more confusing :) https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... File media/audio/win/shared_mem_synchronizer_win.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:4: #include "media/audio/shared_mem_synchronizer.h" On 2012/03/08 02:37:08, scherkus wrote: > blank line above Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:22: DCHECK(::WaitForSingleObject(mine_, 0) == WAIT_TIMEOUT); On 2012/03/08 02:37:08, scherkus wrote: > can we DCHECK_EQ it up here + below? Done. > > also is this a dcheck or are we doing something here? Only a DCHECK. > msdn says WAIT_TIMEOUT is > returned if the object has been signaled, I guess you mean 'not signaled'? That's what the DCHECK is for. > so how about we add some logging here > so it's clearer what's going on if this were to fail (Signal() called multiple > times -- i.e., aggressive chess player!) hehe, good mental picture! I added a little hint to the DCHECK about what the programmer might be doing wrong if we hit this. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:23: ::SetEvent(mine_); On 2012/03/08 02:37:08, scherkus wrote: > FYI this could fail -- but not sure what we'd do to handle that case > > perhaps a CHECK() would suffice so we get some crash dumps to verify that it's > possible to fail in the first place, WDYT? > > NOTE: I'm not a windows programming guru so let me know if the common practice > is to ignore the BOOL/HRESULT values for functions like these ones :) Paranoid is good in this case, so Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:29: DCHECK(wait == WAIT_OBJECT_0); On 2012/03/08 02:37:08, scherkus wrote: > ditto Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:30: ::ResetEvent(other_); On 2012/03/08 02:37:08, scherkus wrote: > ditto on failing here + way below Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:43: if (::DuplicateHandle(our_process, mine_, process, handle_1, 0, FALSE, On 2012/03/08 02:37:08, scherkus wrote: > nit: return early on fail instead of nesting ifs and using |success| variable? Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:171: DCHECK(last_signaled < count); On 2012/03/08 02:37:08, scherkus wrote: > ditto here+below Done. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:193: // Assuming equally busy events, this approach distributes the priority On 2012/03/08 02:37:08, scherkus wrote: > sanity check: aren't we using the same trick in posix land? Yes. There's one very subtle difference which is that on Windows, this problem belongs to WaitForMultipleObjects (OS provided implementation) but on posix, the loop I wrote has the same property as WaitForMultipleObjects - so, our code in that case. I don't know if the comments make that clear, but ... the short answer is yes. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:206: if (count <= MAXIMUM_WAIT_OBJECTS) { On 2012/03/08 02:37:08, scherkus wrote: > stepping back a bit is this something we want/need to support now? > > anyway, it seems like we'd hit quite the performance cliff once we reach > MAXIMUM_WAIT_OBJECTS by thrashing thread creation+tear down > > if we assumed thread creation + hand off of the next 63+ events took N > milliseconds, what if we called WFMO() w/ a wait time of <N on chunks of 64 > events? If WFMO() times out we would rebuild the handles array using the next > chunk of 64 events, etc... > > I have a feeling the resulting code would be easier to understand as well as > avoid creating/stopping threads in what I believe will ultimately be a tight > loop -- but again I don't think this is an area we need to optimize until proven > necessary :) Right! So, I thought quite a bit about all of this too and came up with a way to not have to rely on this in practice. I would still like to have this code in for now just so that we don't have this OS limitation hanging over our heads, but checkout the timings on the trybots for TwoSynchronizersTwoThreads1000Signals vs ThousandSynchronizersTwoThreads. They use two different approaches to accomplish the same thing. One is significantly faster and the other runs into a resource limitation on the bots so it can't actually do as many operations as the other but it is still significantly slower! So, moving forward, we'll be taking the faster, less resource heavy approach. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:218: // next thread. etc etc. On 2012/03/08 02:37:08, scherkus wrote: > bit confusing to read > > perhaps reword to include something along the lines of "recursively create > threads so that each thread waits on at most 62 events including one event for > when a child thread signals completion and one event for when a parent thread > must stop a child thread" > > I would also include a mention to MAXIMUM_WAIT_OBJECTS and/or MSDN link for the > non-windows-folk out there :) I realize now that this comment is a note I left to myself about how to go about implementing this. Changed to: // Create a list of threads so that each thread waits on at most 62 events // including one event for when a child thread signals completion and one // event for when all of the threads must be stopped (due to some event // being signaled). I didn't use 'recursively' since the process happens in parallel to the current thread and doesn't involve recursive calls. https://chromiumcodereview.appspot.com/9605015/diff/1/media/audio/win/shared_... media/audio/win/shared_mem_synchronizer_win.cc:253: NOTREACHED(); On 2012/03/08 02:37:08, scherkus wrote: > LOG? > > how severe is this? > > note that in release mode this does nothing as does your DCHECK below These are just sanity checks. If they fail it's because something is wrong with the implementation.
drive-by minor coding comments below, but a bigger philosophical point here. The new interface & impl don't have anything to do with SharedMemory, do they? This seems more like a version of base::WaitableEvent that can cross process boundaries on POSIX (on win it already can). Have you considered attempting to extend base::WaitableEvent to do that instead of introducing this new class? In particular the API around construction/initialization/sendtoprocess seems baroque to me. IWBN if instead one could construct a WaitableEvent and simply IPC Send() it to another process and have it "just work" (by extending WaitableEvent's impl to account for this, and teaching IPC how to serialize/deserialize WE). WDYT? https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... File media/audio/shared_mem_synchronizer.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:27: last_ = (ret >= 0) ? static_cast<size_t>(ret) : count_ - 1; When will the conditional be false, given the DCHECK_NE(ret, -1); at the bottom of WaitMultiple() ? https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:48: size_t bytes = socket_.Send(&signal, sizeof(signal)); Is this depending on the socket having a buffer so that this doesn't block even if there's no Wait() happening elsewhere? (what happens if many Signal()'s are sent and none are waited for?) https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:130: break; could just return ret; here and replace the remainder of the function with an unconditional NOTREACHED/return -1. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... File media/audio/shared_mem_synchronizer.h (right): https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:35: // synchronizer_->Wait(); // Wait for the other process to yield access. underscores here and two lines lower are superfluous. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:110: // handle that case. the sample code above will be copy/pasted, so it should handle such failure. (here and elsewhere) https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:128: WaitForMultiple(SharedMemSynchronizer* synchronizers, size_t count); vector to replace the ptr+size?
On 2012/03/08 17:04:36, Ami Fischman wrote: > drive-by minor coding comments below, but a bigger philosophical point here. > > The new interface & impl don't have anything to do with SharedMemory, do they? > This seems more like a version of base::WaitableEvent that can cross process > boundaries on POSIX (on win it already can). Have you considered attempting to > extend base::WaitableEvent to do that instead of introducing this new class? > In particular the API around construction/initialization/sendtoprocess seems > baroque to me. IWBN if instead one could construct a WaitableEvent and simply > IPC Send() it to another process and have it "just work" (by extending > WaitableEvent's impl to account for this, and teaching IPC how to > serialize/deserialize WE). WDYT? I agree the name should be something more WaitableEvent-ish that doesn't hint at it's intended primary use (shared memory). The biggest difference from the regular WaitableEvent and this API is that it's primary use is to be able to wait on 100s of events across processes. Whether that warrants a new class or extending an existing API is up to debate :) What I *am* curious about is that it seems you discovered that SynchronizersTwoThreads1000Signals is considerably faster after writing this API. If that's the case then I'm not sure we need an additional synchronization primitive when we already have WaitableEvent (which can wait-on-many, but isn't IPC-able), and SyncSocket (which cannot wait-on-many, but is IPC-able). It looks like we could either add wait-on-many to SyncSocket OR fold SyncSocket into WaitableEvent and make it ICP-able (per fischman's suggestion). Then we can implement the SynchronizersTwoThreads1000Signals using a shmem bitfield and a pair of WE/SS per process. > https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... > File media/audio/shared_mem_synchronizer.cc (right): > > https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... > media/audio/shared_mem_synchronizer.cc:27: last_ = (ret >= 0) ? > static_cast<size_t>(ret) : count_ - 1; > When will the conditional be false, given the > DCHECK_NE(ret, -1); > at the bottom of WaitMultiple() ? > > https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... > media/audio/shared_mem_synchronizer.cc:48: size_t bytes = socket_.Send(&signal, > sizeof(signal)); > Is this depending on the socket having a buffer so that this doesn't block even > if there's no Wait() happening elsewhere? > (what happens if many Signal()'s are sent and none are waited for?) > > https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... > media/audio/shared_mem_synchronizer.cc:130: break; > could just return ret; here and replace the remainder of the function with an > unconditional NOTREACHED/return -1. > > https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... > File media/audio/shared_mem_synchronizer.h (right): > > https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... > media/audio/shared_mem_synchronizer.h:35: // synchronizer_->Wait(); // Wait > for the other process to yield access. > underscores here and two lines lower are superfluous. > > https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... > media/audio/shared_mem_synchronizer.h:110: // handle that case. > the sample code above will be copy/pasted, so it should handle such failure. > (here and elsewhere) > > https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... > media/audio/shared_mem_synchronizer.h:128: > WaitForMultiple(SharedMemSynchronizer* synchronizers, size_t count); > vector to replace the ptr+size?
On 2012/03/08 17:04:36, Ami Fischman wrote: > The new interface & impl don't have anything to do with SharedMemory, do they? Not really. I'm open to suggestions for a better name. I started out by calling this ChessClock as it describes the usage scenario for the class, but then opted for something that describes what I'll be using it for. > This seems more like a version of base::WaitableEvent that can cross process > boundaries on POSIX (on win it already can). Have you considered attempting to > extend base::WaitableEvent to do that instead of introducing this new class? I did consider it but this is different from WaitableEvent. > In particular the API around construction/initialization/sendtoprocess seems > baroque to me. IWBN if instead one could construct a WaitableEvent and simply > IPC Send() it to another process and have it "just work" (by extending > WaitableEvent's impl to account for this, and teaching IPC how to > serialize/deserialize WE). WDYT? That would be nice and that was what I was initially hoping to be able to do. However, WaitableEvent is a different beast. The synchronizer class must be used like a chess clock. Each player must wait for the other to signal before it can signal. While WaitableEvent could be used for that, SharedMemSynchronizer can not be used for most other scenarios that WaitableEvent supports. E.g. WaitableEvent supports automatically resettable events (SharedMemSynchronizer does not). When configured as manual-reset, WaitableEvent unblocks all listening threads whereas SharedMemSynchronizer can only unblock a single listener - even though the operating mode is fixed as 'manual'. These are subtle but important differences. Because of the specific use case that we need for transferring media buffers, we can implement this synchronizer and get a behaviour that is similar to a manually resetable WaitEvent, but it is not identical since it only supports this exact usage pattern. Imho starting to modify WaitableEvent only so that we can use it instead if SharedMemSynchronizer would be a huge and risky task that even when done, would meet with much reviewer resistance :)
On 2012/03/08 19:04:30, scherkus wrote: > I agree the name should be something more WaitableEvent-ish that doesn't hint at > it's intended primary use (shared memory). The only other name I've come up with so far is ChessClock (or GameClock) :) Any suggestions? > > The biggest difference from the regular WaitableEvent and this API is that it's > primary use is to be able to wait on 100s of events across processes. Whether > that warrants a new class or extending an existing API is up to debate :) There are other differences and imo extending WaitableEvent is not feasible or necessary. > What I *am* curious about is that it seems you discovered that > SynchronizersTwoThreads1000Signals is considerably faster after writing this > API. If that's the case then I'm not sure we need an additional synchronization > primitive when we already have WaitableEvent (which can wait-on-many, but isn't > IPC-able), and SyncSocket (which cannot wait-on-many, but is IPC-able). > > It looks like we could either add wait-on-many to SyncSocket OR fold SyncSocket > into WaitableEvent and make it ICP-able (per fischman's suggestion). On POSIX, SharedMemSynchronizer already is SyncSocket (or rather CancelableSyncSocket). On Windows, it uses an event which is equivalent to WaitableEvent and about 25% faster than named pipes which is what SyncSocket on Windows uses. This actually translates into a couple of ms delay. As far as adding Wait() semantics to SyncSocket, I'm not sure that's a good idea. Wait() is basically another way to call Receive(), so in a sense Wait() is already there. Same for Send()/Signal(). SharedMemSynchonizer sits one level higher and abstracts those calls and how many bytes are actually sent or received. Furthermore SharedMemSynchronizer has checks in place to help the programmer do the right thing for a particular usage pattern. SyncSocket has a much wider scope of usability and doesn't define a particular Wait/Signal algorithm. So, to summarize - If we patch this into SyncSocket, SyncSocket becomes a synchronization primitive itself and not just a synchronous socket class. We also force the Windows implementation to use a named pipe which means worse performance. But even if we did that, we would still have to write the logic of Waiting and Signaling correctly in the code. That's basically what SharedMemSynchronizer does. So in the end, what's the gain? > Then we can > implement the SynchronizersTwoThreads1000Signals using a shmem bitfield and a > pair of WE/SS per process. > SynchronizersTwoThreads1000Signals is already implemented by using a bitfield and a pair of WE/SS. (am I missing your point?) SharedMemSynchronzer is an algorithm that uses existing synchronization primitives. It's not a new synchronization primitive in its own right.
https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... File media/audio/shared_mem_synchronizer.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:27: last_ = (ret >= 0) ? static_cast<size_t>(ret) : count_ - 1; On 2012/03/08 17:04:36, Ami Fischman wrote: > When will the conditional be false, given the > DCHECK_NE(ret, -1); > at the bottom of WaitMultiple() ? That DCHECK is to catch programmer error so hopefully never. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:48: size_t bytes = socket_.Send(&signal, sizeof(signal)); On 2012/03/08 17:04:36, Ami Fischman wrote: > Is this depending on the socket having a buffer so that this doesn't block even > if there's no Wait() happening elsewhere? Yes, the socket will buffer the signal to satisfy a single Wait() on the other end. > (what happens if many Signal()'s are sent and none are waited for?) That's not a supported scenario and will result in undefined behavior (not the same on Windows and posix). On Windows you'll hit a DCHECK if you try. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:130: break; On 2012/03/08 17:04:36, Ami Fischman wrote: > could just return ret; here and replace the remainder of the function with an > unconditional NOTREACHED/return -1. This has fewer returns and produces smaller code. Is this less readable to you? https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... File media/audio/shared_mem_synchronizer.h (right): https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:35: // synchronizer_->Wait(); // Wait for the other process to yield access. On 2012/03/08 17:04:36, Ami Fischman wrote: > underscores here and two lines lower are superfluous. Done. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:110: // handle that case. On 2012/03/08 17:04:36, Ami Fischman wrote: > the sample code above will be copy/pasted, so it should handle such failure. > (here and elsewhere) Done. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:128: WaitForMultiple(SharedMemSynchronizer* synchronizers, size_t count); On 2012/03/08 17:04:36, Ami Fischman wrote: > vector to replace the ptr+size? what's the benefit? vectors are more expensive.
I think this is a MultiProcessNotification, FWIW. IIUC the use-case is having a socket be the flow-control channel for inter-process communication being done over shmem. I don't know the history here, but just want to make sure that *you* know why it doesn't make more sense for the actual communication to also be done over a socket, and avoid the need for explicit flow-control. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... File media/audio/shared_mem_synchronizer.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:27: last_ = (ret >= 0) ? static_cast<size_t>(ret) : count_ - 1; On 2012/03/09 12:15:49, tommi wrote: > On 2012/03/08 17:04:36, Ami Fischman wrote: > > When will the conditional be false, given the > > DCHECK_NE(ret, -1); > > at the bottom of WaitMultiple() ? > > That DCHECK is to catch programmer error That's what the DCHECK connotes to me, as well. > so hopefully never. But then the error-handling code here makes me think that it's a case needing handling. Why not just promote the DCHECK to a CHECK and remove this test here? (I don't see why returning count_-1 is helpful; at this point SMS::WFM::W() will already have returned -1 to the user, which is unspecified behavior and the caller would be reasonable to not handle the case explicitly, just like it's unlikely to explicitly handle the case of a ret value of count_ * 100 from here) https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:48: size_t bytes = socket_.Send(&signal, sizeof(signal)); On 2012/03/09 12:15:49, tommi wrote: > On 2012/03/08 17:04:36, Ami Fischman wrote: > > Is this depending on the socket having a buffer so that this doesn't block > even > > if there's no Wait() happening elsewhere? > > Yes, the socket will buffer the signal to satisfy a single Wait() on the other > end. > > > (what happens if many Signal()'s are sent and none are waited for?) > > That's not a supported scenario and will result in undefined behavior (not the > same on Windows and posix). On Windows you'll hit a DCHECK if you try. I suspect on posix you'll get a hang as the pipe's buffer is filled and the write syscall blocks. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:130: break; On 2012/03/09 12:15:49, tommi wrote: > On 2012/03/08 17:04:36, Ami Fischman wrote: > > could just return ret; here and replace the remainder of the function with an > > unconditional NOTREACHED/return -1. > > This has fewer returns and produces smaller code. Is this less readable to you? Yes. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... File media/audio/shared_mem_synchronizer.h (right): https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:48: // SharedMemSynchronizer::InitializePair(&synchronizer_, &b); No error handling https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:50: // b.ShareToProcess(process_b, &handle_1, &handle_2); No error handling https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:128: WaitForMultiple(SharedMemSynchronizer* synchronizers, size_t count); On 2012/03/09 12:15:49, tommi wrote: > On 2012/03/08 17:04:36, Ami Fischman wrote: > > vector to replace the ptr+size? > > what's the benefit? vectors are more expensive. The benefit is that it's more difficult to make programming errors where the size & pointer are mismatched. What expense are you worried about? A vector boils down to a ptr+size, anyway.
https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... File media/audio/shared_mem_synchronizer.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:27: last_ = (ret >= 0) ? static_cast<size_t>(ret) : count_ - 1; On 2012/03/09 16:36:47, Ami Fischman wrote: > On 2012/03/09 12:15:49, tommi wrote: > > On 2012/03/08 17:04:36, Ami Fischman wrote: > > > When will the conditional be false, given the > > > DCHECK_NE(ret, -1); > > > at the bottom of WaitMultiple() ? > > > > That DCHECK is to catch programmer error > > That's what the DCHECK connotes to me, as well. > > > so hopefully never. > > But then the error-handling code here makes me think that it's a case needing > handling. > Why not just promote the DCHECK to a CHECK and remove this test here? > > (I don't see why returning count_-1 is helpful; at this point SMS::WFM::W() will > already have returned -1 to the user, which is unspecified behavior and the > caller would be reasonable to not handle the case explicitly, just like it's > unlikely to explicitly handle the case of a ret value of count_ * 100 from here) Done. Added CHECKs to WaitMultiple. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:48: size_t bytes = socket_.Send(&signal, sizeof(signal)); On 2012/03/09 16:36:47, Ami Fischman wrote: > On 2012/03/09 12:15:49, tommi wrote: > > On 2012/03/08 17:04:36, Ami Fischman wrote: > > > Is this depending on the socket having a buffer so that this doesn't block > > even > > > if there's no Wait() happening elsewhere? > > > > Yes, the socket will buffer the signal to satisfy a single Wait() on the other > > end. > > > > > (what happens if many Signal()'s are sent and none are waited for?) > > > > That's not a supported scenario and will result in undefined behavior (not the > > same on Windows and posix). On Windows you'll hit a DCHECK if you try. > > I suspect on posix you'll get a hang as the pipe's buffer is filled and the > write syscall blocks. It doesn't block actually and there should only be at most 1 byte in the buffer at all times. There's a single threaded test for this, SharedMemSynchronizer.Basic. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.cc:130: break; On 2012/03/09 16:36:47, Ami Fischman wrote: > On 2012/03/09 12:15:49, tommi wrote: > > On 2012/03/08 17:04:36, Ami Fischman wrote: > > > could just return ret; here and replace the remainder of the function with > an > > > unconditional NOTREACHED/return -1. > > > > This has fewer returns and produces smaller code. Is this less readable to > you? > > Yes. Done. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... File media/audio/shared_mem_synchronizer.h (right): https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:48: // SharedMemSynchronizer::InitializePair(&synchronizer_, &b); On 2012/03/09 16:36:47, Ami Fischman wrote: > No error handling It's in a more recent patch set. Basically just a CHECK(). Let me know if you'd like something a bit more advanced. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:50: // b.ShareToProcess(process_b, &handle_1, &handle_2); On 2012/03/09 16:36:47, Ami Fischman wrote: > No error handling ditto. https://chromiumcodereview.appspot.com/9605015/diff/6003/media/audio/shared_m... media/audio/shared_mem_synchronizer.h:128: WaitForMultiple(SharedMemSynchronizer* synchronizers, size_t count); On 2012/03/09 16:36:47, Ami Fischman wrote: > On 2012/03/09 12:15:49, tommi wrote: > > On 2012/03/08 17:04:36, Ami Fischman wrote: > > > vector to replace the ptr+size? > > > > what's the benefit? vectors are more expensive. > > The benefit is that it's more difficult to make programming errors where the > size & pointer are mismatched. > What expense are you worried about? A vector boils down to a ptr+size, anyway. Done. Changed to std::vector<SharedMemSynchronizer*> and not vector<SharedMemSynchronizer> to avoid copy constructors etc.
On 2012/03/09 16:36:47, Ami Fischman wrote: > I think this is a MultiProcessNotification, FWIW. > > IIUC the use-case is having a socket be the flow-control channel for > inter-process communication being done over shmem. I don't know the history > here, but just want to make sure that *you* know why it doesn't make more sense > for the actual communication to also be done over a socket, and avoid the need > for explicit flow-control. > I'm OK with MultiProcessNotification as long as it can't me misunderstood to be usable for more than two processes (or threads). What say you?
re: "MultiProcessNotification" w/ limit to 2 processes/threads: How about CrossProcessNotification ? https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.cc:14: : synchronizers_(synchronizers), last_(synchronizers_->size() - 1) { Why init last_ (an unsigned type) to the result of a subtraction (always a sketchy thing to do with unsigned types)? IOW doing last_(0) would be no less correct, but less confusing. However, using last_(0) has the down-side of the unobvious fact that the first run through WaitMultiple will populate [1,size) followed by 0. If instead of tracking last_ you tracked next_start_offset_ then you could drop the -1 here (by initializing to 0 here), and have a more expected order in the first run of WaitMultiple (and subsequent runs stay as obvious). Plus, IMO, the calc of (i + next_wait_offset_) % synchronizers.size() is slightly clearer than (i + last_signaled + 1) % synchronizers.size() https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.cc:15: DCHECK_GT(synchronizers_->size(), 0U); clearer as DCHECK(!synchronizers_->empty()); https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.cc:27: DCHECK_GT(synchronizers_->size(), 0U); ditto https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.cc:27: DCHECK_GT(synchronizers_->size(), 0U); reset next_start_offset_ (nee last_)? https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer.h (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.h:105: void Signal(); My point about blocking was that if a single synchronizer sent 100k Signal()'s, with nobody calling Wait(), I bet the OS buffer for the socket would fill up and Signal() would block on the write. It's fine to say that's not a supported use-case (specifically that only an initial Signal() is legit without previously Wait()ing), but I was trying to get you to be explicit about that contract. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.h:140: // Call when the array changes. Doco the effect on outstanding Wait()'s? (namely, that this should never be called during an outstanding Wait()?) https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer_posix.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer_posix.cc:103: int ret = (i + last_signaled + 1) % synchronizers.size(); It is asymmetric that ret is an "int" but last_signaled is a "size_t". https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer_posix.cc:110: LOG(ERROR) << "poll() failed: " << errno; The case of poll returning 0 but none of the revents is true is even more of a poll failure. I'd omit this else and just stick a LOG(FATAL) << "poll() failed impossibly"; instead of the CHECK(false) below. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer_unittest.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer_unittest.cc:15: Any reason to use a stack array instead of just using a vector to start with? https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer_win.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer_win.cc:173: int SharedMemSynchronizer::WaitMultiple(const SynchronizerVector& synchronizers, FTR I haven't reviewed the win impl at all (IDK nuthin' 'bout windows).
Comments addressed. CrossProcessNotification sounds good so I'm doing that change now and will upload a new patch set with the name change in a bit. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.cc:14: : synchronizers_(synchronizers), last_(synchronizers_->size() - 1) { On 2012/03/13 20:08:02, Ami Fischman wrote: > Why init last_ (an unsigned type) to the result of a subtraction (always a > sketchy thing to do with unsigned types)? > IOW doing last_(0) would be no less correct, but less confusing. > > However, using last_(0) has the down-side of the unobvious fact that the first > run through WaitMultiple will populate [1,size) followed by 0. > If instead of tracking last_ you tracked next_start_offset_ then you could drop > the -1 here (by initializing to 0 here), and have a more expected order in the > first run of WaitMultiple (and subsequent runs stay as obvious). Plus, IMO, the > calc of > (i + next_wait_offset_) % synchronizers.size() > is slightly clearer than > (i + last_signaled + 1) % synchronizers.size() Done. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.cc:15: DCHECK_GT(synchronizers_->size(), 0U); On 2012/03/13 20:08:02, Ami Fischman wrote: > clearer as > DCHECK(!synchronizers_->empty()); Done. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.cc:27: DCHECK_GT(synchronizers_->size(), 0U); On 2012/03/13 20:08:02, Ami Fischman wrote: > ditto Done. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.cc:27: DCHECK_GT(synchronizers_->size(), 0U); On 2012/03/13 20:08:02, Ami Fischman wrote: > reset next_start_offset_ (nee last_)? Done. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer.h (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.h:105: void Signal(); On 2012/03/13 20:08:02, Ami Fischman wrote: > My point about blocking was that if a single synchronizer sent 100k Signal()'s, > with nobody calling Wait(), I bet the OS buffer for the socket would fill up and > Signal() would block on the write. > It's fine to say that's not a supported use-case (specifically that only an > initial Signal() is legit without previously Wait()ing), but I was trying to get > you to be explicit about that contract. Thanks. I added documentation to make that clear. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer.h:140: // Call when the array changes. On 2012/03/13 20:08:02, Ami Fischman wrote: > Doco the effect on outstanding Wait()'s? > (namely, that this should never be called during an outstanding Wait()?) Done. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer_posix.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer_posix.cc:103: int ret = (i + last_signaled + 1) % synchronizers.size(); On 2012/03/13 20:08:02, Ami Fischman wrote: > It is asymmetric that ret is an "int" but last_signaled is a "size_t". > Done. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer_posix.cc:110: LOG(ERROR) << "poll() failed: " << errno; On 2012/03/13 20:08:02, Ami Fischman wrote: > The case of poll returning 0 but none of the revents is true is even more of a > poll failure. I'd omit this else and just stick a > LOG(FATAL) << "poll() failed impossibly"; > instead of the CHECK(false) below. Done. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer_unittest.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer_unittest.cc:15: On 2012/03/13 20:08:02, Ami Fischman wrote: > Any reason to use a stack array instead of just using a vector to start with? Done. The alternative is to have two loops, one for construction and another for deletion for both arrays. The stack array did the latter for me. I don't feel strongly about this so I removed the stack arrays and wrote a little class that does this for me using STLDeleteElements. Stack arrays are gone. https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... File media/audio/shared_mem_synchronizer_win.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/28002/media/audio/shared_... media/audio/shared_mem_synchronizer_win.cc:173: int SharedMemSynchronizer::WaitMultiple(const SynchronizerVector& synchronizers, On 2012/03/13 20:08:02, Ami Fischman wrote: > FTR I haven't reviewed the win impl at all (IDK nuthin' 'bout windows). noted. Andrew - any specific comments on the win side?
Updated the description but the subject is still the same.
LGTM++ https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... File media/audio/cross_process_notification.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.cc:13: const Notifications* notifications) FWIW the initializer-list & ctor body can both be replaced with a call to Reset(notifications) now. https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.cc:21: wait_offset_ = (static_cast<size_t>(ret) + 1) % notifications_->size(); Is the static_cast<size_t>() really necessary? (I can't make a toy program that generates a warning/error with either clang or gcc on my z600, building in either -m32 or -m64). https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... File media/audio/cross_process_notification.h (right): https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.h:25: // it has finished reading from/writing to the shared memory section. s/shared memory section/shared resource/ https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.h:111: // Waits for the other party to finish using the shared memory. s/memory/resource/ https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.h:143: // index of a signaled synchronizer. s/synchronizer/notification/
All done. Andrew - care to take a look at the windows specific code? https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... File media/audio/cross_process_notification.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.cc:13: const Notifications* notifications) On 2012/03/14 16:40:51, Ami Fischman wrote: > FWIW the initializer-list & ctor body can both be replaced with a call to > Reset(notifications) now. Done. https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.cc:21: wait_offset_ = (static_cast<size_t>(ret) + 1) % notifications_->size(); On 2012/03/14 16:40:51, Ami Fischman wrote: > Is the static_cast<size_t>() really necessary? > (I can't make a toy program that generates a warning/error with either clang or > gcc on my z600, building in either -m32 or -m64). it's not necessary. removed. https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... File media/audio/cross_process_notification.h (right): https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.h:25: // it has finished reading from/writing to the shared memory section. On 2012/03/14 16:40:51, Ami Fischman wrote: > s/shared memory section/shared resource/ Done. https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.h:111: // Waits for the other party to finish using the shared memory. On 2012/03/14 16:40:51, Ami Fischman wrote: > s/memory/resource/ Done. https://chromiumcodereview.appspot.com/9605015/diff/33003/media/audio/cross_p... media/audio/cross_process_notification.h:143: // index of a signaled synchronizer. On 2012/03/14 16:40:51, Ami Fischman wrote: > s/synchronizer/notification/ Done.
LGTM w/ nits I'm terribly sorry for not getting to this earlier! https://chromiumcodereview.appspot.com/9605015/diff/37003/media/audio/cross_p... File media/audio/cross_process_notification.h (right): https://chromiumcodereview.appspot.com/9605015/diff/37003/media/audio/cross_p... media/audio/cross_process_notification.h:169: private: nit: remove duplicate private: section https://chromiumcodereview.appspot.com/9605015/diff/37003/media/audio/cross_p... File media/audio/cross_process_notification_win.cc (right): https://chromiumcodereview.appspot.com/9605015/diff/37003/media/audio/cross_p... media/audio/cross_process_notification_win.cc:12: CrossProcessNotification::~CrossProcessNotification() { nit {}
Thanks Andrew - all done. :) On Tue, Mar 27, 2012 at 4:38 PM, <scherkus@chromium.org> wrote: > LGTM w/ nits > > I'm terribly sorry for not getting to this earlier! > > > https://chromiumcodereview.**appspot.com/9605015/diff/** > 37003/media/audio/cross_**process_notification.h<https://chromiumcodereview.appspot.com/9605015/diff/37003/media/audio/cross_process_notification.h> > File media/audio/cross_process_**notification.h (right): > > https://chromiumcodereview.**appspot.com/9605015/diff/** > 37003/media/audio/cross_**process_notification.h#**newcode169<https://chromiumcodereview.appspot.com/9605015/diff/37003/media/audio/cross_process_notification.h#newcode169> > media/audio/cross_process_**notification.h:169: private: > nit: remove duplicate private: section > > https://chromiumcodereview.**appspot.com/9605015/diff/** > 37003/media/audio/cross_**process_notification_win.cc<https://chromiumcodereview.appspot.com/9605015/diff/37003/media/audio/cross_process_notification_win.cc> > File media/audio/cross_process_**notification_win.cc (right): > > https://chromiumcodereview.**appspot.com/9605015/diff/** > 37003/media/audio/cross_**process_notification_win.cc#**newcode12<https://chromiumcodereview.appspot.com/9605015/diff/37003/media/audio/cross_process_notification_win.cc#newcode12> > media/audio/cross_process_**notification_win.cc:12: > CrossProcessNotification::~**CrossProcessNotification() { > nit {} > > https://chromiumcodereview.**appspot.com/9605015/<https://chromiumcodereview.... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommi@chromium.org/9605015/43002
Try job failure for 9605015-43002 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
