Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2053273002:
Remove WorkerBackingThread::scriptCount (Closed)
|
Created:
4 years, 6 months ago by yhirano Modified:
4 years, 6 months ago CC:
chromium-reviews, falken, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove WorkerBackingThread::scriptCount
This CL removes scriptCount from WorkerBackingThread. The count was
needed for compositor workers, but we don't need it any more because
of compositor workers' design change.
Before this change, compositor workers are exempted from
TerminateExecution call because multiple workers can share the same
backing thread, but with this change TerminateExecution can be
called for a compositor worker when it doesn't start the shutdown
sequence in a timely manner.
BUG=487050
Committed: https://crrev.com/17686215e73797de165dd749f8f77d1388f1ede6
Cr-Commit-Position: refs/heads/master@{#400102}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 5
Patch Set 3 : fix #Patch Set 4 : rebase #Messages
Total messages: 34 (14 generated)
Description was changed from ========== Remove WorkerBackingThread::scriptCount BUG= ========== to ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 ==========
yhirano@chromium.org changed reviewers: + nhiroki@chromium.org
Description was changed from ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 ========== to ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
lgtm https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.h:155: WaitableEvent* shutdownEventForTesting() { return m_shutdownEvent.get(); } I'd prefer to hide an internal field as much as possible. Instead of exposing WaitableEvent, how about directly waiting for shutdown like this? void waitForShutdownForTesting() { m_shutdownEvent->wait(); } And then, replace WorkerThreadTest::waitForShutdown() with this?
This looks great! LGTM.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
looks great, plus one high-level comment (not directly related to this CL) https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:28: class CORE_EXPORT WorkerBackingThread final { Now that this class doesn't to much (and there's some home assignment we haven't solved reg: separation between WorkerThread and WorkerBackingThread)... I started to wonder we might want to rethink if we still need this class?
yhirano@chromium.org changed reviewers: + flackr@chromium.org
+flackr@
kinuko@chromium.org changed reviewers: - kinuko@chromium.org
On 2016/06/14 at 11:05:32, yhirano wrote: > +flackr@ LGTM, thanks for doing this cleanup.
https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:28: class CORE_EXPORT WorkerBackingThread final { On 2016/06/14 09:06:56, kinuko wrote: > Now that this class doesn't to much (and there's some home assignment we haven't > solved reg: separation between WorkerThread and WorkerBackingThread)... I > started to wonder we might want to rethink if we still need this class? I still think it's good to separate this class from WorkerThread due to CompositorWorkerThread's initialization. I'm wondering if it would be reasonable to merge this class to WebThreadSupportingGC, i.e., making WebThreadSupportingGC capable of attaching a v8 isolate. If reviewers are happy with that idea I'm willing to do that later. What do you think?
https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:28: class CORE_EXPORT WorkerBackingThread final { On 2016/06/14 20:19:25, yhirano (slow) wrote: > On 2016/06/14 09:06:56, kinuko wrote: > > Now that this class doesn't to much (and there's some home assignment we > haven't > > solved reg: separation between WorkerThread and WorkerBackingThread)... I > > started to wonder we might want to rethink if we still need this class? > > I still think it's good to separate this class from WorkerThread due to > CompositorWorkerThread's initialization. I'm wondering if it would be reasonable > to merge this class to WebThreadSupportingGC, i.e., making WebThreadSupportingGC > capable of attaching a v8 isolate. If reviewers are happy with that idea I'm > willing to do that later. What do you think? Regarding merge, I'm not really sure whether it's worth doing it because capability to attach a V8 isolate is not necessary for other threads that own WebThreadSupportingGC (e.g. HTMLParserThread).
https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.h:155: WaitableEvent* shutdownEventForTesting() { return m_shutdownEvent.get(); } On 2016/06/13 19:39:45, nhiroki (slow until Jun 20) wrote: > I'd prefer to hide an internal field as much as possible. Instead of exposing > WaitableEvent, how about directly waiting for shutdown like this? > > void waitForShutdownForTesting() { m_shutdownEvent->wait(); } > > And then, replace WorkerThreadTest::waitForShutdown() with this? Done.
On 2016/06/15 05:51:19, nhiroki (slow until Jun 20) wrote: > https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): > > https://codereview.chromium.org/2053273002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/workers/WorkerBackingThread.h:28: class > CORE_EXPORT WorkerBackingThread final { > On 2016/06/14 20:19:25, yhirano (slow) wrote: > > On 2016/06/14 09:06:56, kinuko wrote: > > > Now that this class doesn't to much (and there's some home assignment we > > haven't > > > solved reg: separation between WorkerThread and WorkerBackingThread)... I > > > started to wonder we might want to rethink if we still need this class? > > > > I still think it's good to separate this class from WorkerThread due to > > CompositorWorkerThread's initialization. I don't disagree that there's a benefit having this class, but as far as we have things that are unclear about which class they should belong to I can't say that's absolutely better than not having the separation. > I'm wondering if it would be > reasonable > > to merge this class to WebThreadSupportingGC, i.e., making > WebThreadSupportingGC > > capable of attaching a v8 isolate. If reviewers are happy with that idea I'm > > willing to do that later. What do you think? I first thought it could be a good idea (assuming we can somehow nicely have both cases that require v8 isolate and not within WebThreadSupportingGC class), but current WorkerBackingThread code's still doing a bit of worker-specific things too, which needs to move back to WorkerThread in the case. Hm. Separately-- let's land this change if no one objects! (Additional lgtm just fyi) > Regarding merge, I'm not really sure whether it's worth doing it because > capability to attach a V8 isolate is not necessary for other threads that own > WebThreadSupportingGC (e.g. HTMLParserThread).
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, haraken@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2053273002/#ps80001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053273002/80001
Message was sent while issue was closed.
Description was changed from ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 ========== to ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 ========== to ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 Committed: https://crrev.com/17686215e73797de165dd749f8f77d1388f1ede6 Cr-Commit-Position: refs/heads/master@{#400102} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/17686215e73797de165dd749f8f77d1388f1ede6 Cr-Commit-Position: refs/heads/master@{#400102}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:80001) has been created in https://codereview.chromium.org/2066403003/ by grunell@chromium.org. The reason for reverting is: Two compositorworker tests have failed flakily after this change: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b... https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b....
Message was sent while issue was closed.
Description was changed from ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 Committed: https://crrev.com/17686215e73797de165dd749f8f77d1388f1ede6 Cr-Commit-Position: refs/heads/master@{#400102} ========== to ========== Remove WorkerBackingThread::scriptCount This CL removes scriptCount from WorkerBackingThread. The count was needed for compositor workers, but we don't need it any more because of compositor workers' design change. Before this change, compositor workers are exempted from TerminateExecution call because multiple workers can share the same backing thread, but with this change TerminateExecution can be called for a compositor worker when it doesn't start the shutdown sequence in a timely manner. BUG=487050 Committed: https://crrev.com/17686215e73797de165dd749f8f77d1388f1ede6 Cr-Commit-Position: refs/heads/master@{#400102} ==========
reopen
yhirano@chromium.org changed reviewers: + grunell@chromium.org
On 2016/06/16 11:52:28, Henrik Grunell wrote: > A revert of this CL (patchset #3 id:80001) has been created in > https://codereview.chromium.org/2066403003/ by mailto:grunell@chromium.org. > > The reason for reverting is: Two compositorworker tests have failed flakily > after this change: > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b... > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b.... Is the failure caused by this CL? It looks flaky with or without this CL. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=plu...
On 2016/06/17 09:19:18, yhirano (unavailable in June) wrote: > On 2016/06/16 11:52:28, Henrik Grunell wrote: > > A revert of this CL (patchset #3 id:80001) has been created in > > https://codereview.chromium.org/2066403003/ by mailto:grunell@chromium.org. > > > > The reason for reverting is: Two compositorworker tests have failed flakily > > after this change: > > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b... > > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b.... > > Is the failure caused by this CL? It looks flaky with or without this CL. > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=plu... It was not this CL. The bot turned green before the revert landed. I'll revert the revert.
Message was sent while issue was closed.
On 2016/06/17 09:58:04, Henrik Grunell wrote: > On 2016/06/17 09:19:18, yhirano (unavailable in June) wrote: > > On 2016/06/16 11:52:28, Henrik Grunell wrote: > > > A revert of this CL (patchset #3 id:80001) has been created in > > > https://codereview.chromium.org/2066403003/ by mailto:grunell@chromium.org. > > > > > > The reason for reverting is: Two compositorworker tests have failed flakily > > > after this change: > > > > > > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b... > > > > > > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b.... > > > > Is the failure caused by this CL? It looks flaky with or without this CL. > > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=plu... > > It was not this CL. The bot turned green before the revert landed. I'll revert > the revert. Thank you! |
