Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
368923004:
Adding an ExecutorFactory for the bindings. (Closed)
|
Created:
6 years, 5 months ago by qsr Modified:
6 years, 5 months ago Reviewers:
rmcilroy CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdding an ExecutorFactory for the bindings.
This will allow to call back the thread owning a connector from the
finalization thread.
This API is private to the bindings code.
R=rmcilroy@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281821
Patch Set 1 #Patch Set 2 : Use a null buffer so that no data is sent. #
Total comments: 16
Patch Set 3 : #
Total comments: 37
Patch Set 4 : #
Messages
Total messages: 9 (0 generated)
You can see the usage of this API here: https://chromiumcodereview.appspot.com/364063006/patch/1/10006
gentle ping?
Overall this looks a lot better than the previous executor CL, thanks. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java (right): https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:64: if (mWaiter != null) { Is this class useful if getDefaultAsyncWaiter() returns null? Maybe just assert(mWaiter != null)? https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:85: ReadMessageResult readMessageResult = mReadHandle.readMessage(NOTIFY_BUFFER, 0, How about doing the readMessageResult check in asyncWait instead? I think this would be clearer. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:87: if (readMessageResult.getMojoResult() == MojoResult.OK) { What about MojoResult.SHOULD_WAIT, would you get this if there was nothing on the mReadHandle? https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:100: void close() { I think you should set a flag here such that execute can throw an exception if called on a closed executor. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:120: public void execute(Runnable command) { How about just make execute and close synchronized methods rather than synchronizing on mWriteHandle. This would also enable you to set/check the is_closed flag in a safe way, and ensure mPendingActions is empty. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:121: mPendingActions.add(command); Check for: if (is_closed) throw new IllegalStateException("..."); before adding the runnable to the queue. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:138: public static Executor getExecutorForCurrentThread(Core core) { This needs to be synchronised if you want there to only be a single executor per thread (otherwise there is a race on getting and setting the threadlocal).
https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java (right): https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:64: if (mWaiter != null) { On 2014/07/07 18:53:30, rmcilroy wrote: > Is this class useful if getDefaultAsyncWaiter() returns null? Maybe just > assert(mWaiter != null)? Done. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:85: ReadMessageResult readMessageResult = mReadHandle.readMessage(NOTIFY_BUFFER, 0, On 2014/07/07 18:53:30, rmcilroy wrote: > How about doing the readMessageResult check in asyncWait instead? I think this > would be clearer. asyncWait is not called with a message in the pipe, at least not at first, and I'd rather not have a special case there. Instead, changed this to do an early return after calling asyncWait, and then a close at the end of the function. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:87: if (readMessageResult.getMojoResult() == MojoResult.OK) { On 2014/07/07 18:53:30, rmcilroy wrote: > What about MojoResult.SHOULD_WAIT, would you get this if there was nothing on > the mReadHandle? If onResult is called with an infinite deadline (which I'm using), the return value of read should never be SHOULD_WAIT https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:100: void close() { On 2014/07/07 18:53:30, rmcilroy wrote: > I think you should set a flag here such that execute can throw an exception if > called on a closed executor. The flag is already there in mWriteHandle.isValid(). Updated the code to throw a IllegalStateException in that case. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:120: public void execute(Runnable command) { On 2014/07/07 18:53:30, rmcilroy wrote: > How about just make execute and close synchronized methods rather than > synchronizing on mWriteHandle. This would also enable you to set/check the > is_closed flag in a safe way, and ensure mPendingActions is empty. So, findbugs doesn't want us to use synchronized method. Also, closing a handle, makes it return false to isValid, so I already have the flag About pending actions, I also made sure not to add anything is the handle is closed. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:121: mPendingActions.add(command); On 2014/07/07 18:53:30, rmcilroy wrote: > Check for: > if (is_closed) throw new IllegalStateException("..."); > before adding the runnable to the queue. Hum, I can already check if the handle is closed. Used that and throw the exception. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:138: public static Executor getExecutorForCurrentThread(Core core) { On 2014/07/07 18:53:30, rmcilroy wrote: > This needs to be synchronised if you want there to only be a single executor per > thread (otherwise there is a race on getting and setting the threadlocal). I must be missing something there. I'm using ThreadLocal, so I set the executor for the current thread, so I cannot have concurrency issue, as the only thing that can access the same slot is the same thread.
Mostly nits - lgtm assuming the comments can be addressed. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java (right): https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:121: mPendingActions.add(command); On 2014/07/08 08:42:41, qsr wrote: > On 2014/07/07 18:53:30, rmcilroy wrote: > > Check for: > > if (is_closed) throw new IllegalStateException("..."); > > before adding the runnable to the queue. > > Hum, I can already check if the handle is closed. Used that and throw the > exception. Using the handle as the flag is fine, thanks for doing this. https://codereview.chromium.org/368923004/diff/20001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:138: public static Executor getExecutorForCurrentThread(Core core) { On 2014/07/08 08:42:40, qsr wrote: > On 2014/07/07 18:53:30, rmcilroy wrote: > > This needs to be synchronised if you want there to only be a single executor > per > > thread (otherwise there is a race on getting and setting the threadlocal). > > I must be missing something there. I'm using ThreadLocal, so I set the executor > for the current thread, so I cannot have concurrency issue, as the only thing > that can access the same slot is the same thread. You're right, I forgot only that only that thread would access that slot. https://codereview.chromium.org/368923004/diff/40001/mojo/android/javatests/s... File mojo/android/javatests/src/org/chromium/mojo/bindings/ExecutorFactoryTest.java (right): https://codereview.chromium.org/368923004/diff/40001/mojo/android/javatests/s... mojo/android/javatests/src/org/chromium/mojo/bindings/ExecutorFactoryTest.java:56: assertEquals(Thread.currentThread(), mThreadContainer.get(0)); nit - loop through all entries in mThreadContainer to check they all equal Thread.currentThread() https://codereview.chromium.org/368923004/diff/40001/mojo/android/javatests/s... mojo/android/javatests/src/org/chromium/mojo/bindings/ExecutorFactoryTest.java:77: lock.notify(); if you want to check that the executor is threadsafe, maybe you should pull the mExecutor.execute out of the synchronized block, e.g.: public void run() { mExecutor.execute(new Runnable() { @Override public void run() { mThreadContainer.add(Thread.currentThread()); } }); synchronized (lock) { lock.notify(); } https://codereview.chromium.org/368923004/diff/40001/mojo/android/javatests/s... mojo/android/javatests/src/org/chromium/mojo/bindings/ExecutorFactoryTest.java:81: lock.wait(); ditto with the WORKER.execute and lock.wait() (which also avoids having the inner runnable nested within a synchronized block, even although it won't be protected by that lock when it is executed. https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... File mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java (right): https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:23: * obtained from. A factory which provides per-thread executors, which enable execution on the thread from which they were obtained. https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:26: /** nit - newline above https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:27: * An null buffer allows to send a message without sending any data. A null buffer which is used to send messages without any data on the PipedExecutor's signaling handles. https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:36: * it. I think something more like this: Implementation of the executor which uses a pair of {@link MessagePipeHandle} for signaling. The executor will wait asynchronously on one end of a {@link MessagePipeHandle} on the thread on which it was created. Other threads can call execute with a {@link Runnable}, and the executor will queue the {@link Runnable} and write a message on the other end of the handle. This will wake up the executor which is waiting on the handle, which will then dequeue the {@link Runnable} and execute it on the original thread. does this sound right? https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:41: * The handle to write to. The handle which is written to. https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:45: * The handle to read from. The handle which is read from. https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:50: * |mWriteHandle| is hold. /s/hold/held https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:53: private final AsyncWaiter mWaiter; nit - javadoc this too please https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:69: * Wait for the next command to arrive. nit - Asynchronously wait.. https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:69: * Wait for the next command to arrive. mention that this should only be called on the executor thread please. https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:100: close(); I think this could benifit from some helper functions, how about: if (result == MojoResult.OK && readNotifyBufferMessage()) { runNextAction(); } else { // If the result is not |MojoResult.OK|, or the read is not |MojoResult.OK|, there is an // issue with the pipe, and this executor must be closed. close(); } with readNotifyBufferMessage doing L87-96 (and returning true if successful), and runNextAction() doing L82-85 https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:106: void close() { private? https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:107: synchronized (mWriteHandle) { Now that you are protecting two objects with this lock, I would prefer just to have a separate lock object field instead of using mWriteHandle https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:118: public void onError(MojoException exception) { nit - move above close() (close to onResult()) https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:123: * @see Executor#execute(Runnable) mention this can be called from any thread. https://codereview.chromium.org/368923004/diff/40001/mojo/bindings/java/src/o... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:127: // Accessing the write handle must be protected, because its internal value will change Accessing the write handle must be protected by the lock, because it can be closed from the executor's thread.
https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/android/java... File mojo/android/javatests/src/org/chromium/mojo/bindings/ExecutorFactoryTest.java (right): https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/ExecutorFactoryTest.java:56: assertEquals(Thread.currentThread(), mThreadContainer.get(0)); On 2014/07/08 12:35:07, rmcilroy wrote: > nit - loop through all entries in mThreadContainer to check they all equal > Thread.currentThread() Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/ExecutorFactoryTest.java:77: lock.notify(); Used a cyclic barrier instead. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/android/java... mojo/android/javatests/src/org/chromium/mojo/bindings/ExecutorFactoryTest.java:81: lock.wait(); On 2014/07/08 12:35:07, rmcilroy wrote: > ditto with the WORKER.execute and lock.wait() (which also avoids having the > inner runnable nested within a synchronized block, even although it won't be > protected by that lock when it is executed. If I do that, it can happen (although unlikely), that the notify happen before the wait, and the wait will then just block forever. Changed all of this to use cyclic barrier instead, and also increase the concurrency. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... File mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java (right): https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:23: * obtained from. On 2014/07/08 12:35:08, rmcilroy wrote: > A factory which provides per-thread executors, which enable execution on the > thread from which they were obtained. Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:26: /** On 2014/07/08 12:35:08, rmcilroy wrote: > nit - newline above Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:27: * An null buffer allows to send a message without sending any data. On 2014/07/08 12:35:08, rmcilroy wrote: > A null buffer which is used to send messages without any data on the > PipedExecutor's signaling handles. Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:36: * it. On 2014/07/08 12:35:08, rmcilroy wrote: > I think something more like this: > > Implementation of the executor which uses a pair of {@link MessagePipeHandle} > for signaling. The executor will wait asynchronously on one end of a {@link > MessagePipeHandle} on the thread on which it was created. Other threads can call > execute with a {@link Runnable}, and the executor will queue the {@link > Runnable} and write a message on the other end of the handle. This will wake up > the executor which is waiting on the handle, which will then dequeue the {@link > Runnable} and execute it on the original thread. > > does this sound right? Yes, it sounds very good. Thanks. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:41: * The handle to write to. On 2014/07/08 12:35:08, rmcilroy wrote: > The handle which is written to. Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:45: * The handle to read from. On 2014/07/08 12:35:08, rmcilroy wrote: > The handle which is read from. Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:50: * |mWriteHandle| is hold. On 2014/07/08 12:35:08, rmcilroy wrote: > /s/hold/held Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:53: private final AsyncWaiter mWaiter; On 2014/07/08 12:35:08, rmcilroy wrote: > nit - javadoc this too please Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:69: * Wait for the next command to arrive. On 2014/07/08 12:35:08, rmcilroy wrote: > mention that this should only be called on the executor thread please. Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:100: close(); On 2014/07/08 12:35:08, rmcilroy wrote: > I think this could benifit from some helper functions, how about: > > if (result == MojoResult.OK && readNotifyBufferMessage()) { > runNextAction(); > } else { > // If the result is not |MojoResult.OK|, or the read is not |MojoResult.OK|, > there is an > // issue with the pipe, and this executor must be closed. > close(); > } > > with readNotifyBufferMessage doing L87-96 (and returning true if successful), > and runNextAction() doing L82-85 Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:106: void close() { On 2014/07/08 12:35:08, rmcilroy wrote: > private? Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:107: synchronized (mWriteHandle) { On 2014/07/08 12:35:08, rmcilroy wrote: > Now that you are protecting two objects with this lock, I would prefer just to > have a separate lock object field instead of using mWriteHandle Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:118: public void onError(MojoException exception) { On 2014/07/08 12:35:08, rmcilroy wrote: > nit - move above close() (close to onResult()) Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:123: * @see Executor#execute(Runnable) On 2014/07/08 12:35:08, rmcilroy wrote: > mention this can be called from any thread. Done. https://chromiumcodereview.appspot.com/368923004/diff/40001/mojo/bindings/jav... mojo/bindings/java/src/org/chromium/mojo/bindings/ExecutorFactory.java:127: // Accessing the write handle must be protected, because its internal value will change On 2014/07/08 12:35:08, rmcilroy wrote: > Accessing the write handle must be protected by the lock, because it can be > closed from the executor's thread. Done.
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/368923004/60001
Message was sent while issue was closed.
Change committed as 281821 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
