Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268183005:
Worker: Refine state management of WorkerThread (Closed)
|
Created:
4 years, 4 months ago by nhiroki Modified:
4 years, 3 months ago CC:
chromium-reviews, blink-reviews, falken, kinuko+worker_chromium.org, blink-worker-reviews_chromium.org, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents a state of the worker thread,
2) stops accessing |m_globalScope| from the main thread for detecting worker
thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to emphasize that these fields represent
not a state of the worker thread but facts that the worker thread has been
requested to start/terminate from the main thread, and
4) removes mention of |m_microtaskRunner| from a comment about
|m_threadStateMutex| because the member is accessed only from the worker
thread.
BUG=638877, 640843
Committed: https://crrev.com/aefae76fee3afe6cc9eabbe5d06cde009d317c16
Cr-Commit-Position: refs/heads/master@{#414336}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : revert a change of the comment about m_threadStateMutex #Patch Set 4 : remove TODO comment to avoid patch confliction #
Dependent Patchsets: Messages
Total messages: 34 (22 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Worker: Refine state management of WorkerThread
BUG=638877, 639153
==========
to
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents the state of the worker thread,
2) stops accessing |m_globalScope| from the main thread in order to detect
worker thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to clarify that these fields represent not
the state of the worker thread but facts that operations have been requested
from the main thread, and
4) updates a comment about |m_threadStateMutex| as follows:
- Mention of |m_microtaskRunner| is removed because it's accessed only from
the worker thread and it's not necessary to protect it in the first place.
- Mention of |m_globalScope| is removed because of 2).
BUG=638877, 639153
==========
Description was changed from
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents the state of the worker thread,
2) stops accessing |m_globalScope| from the main thread in order to detect
worker thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to clarify that these fields represent not
the state of the worker thread but facts that operations have been requested
from the main thread, and
4) updates a comment about |m_threadStateMutex| as follows:
- Mention of |m_microtaskRunner| is removed because it's accessed only from
the worker thread and it's not necessary to protect it in the first place.
- Mention of |m_globalScope| is removed because of 2).
BUG=638877, 639153
==========
to
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents a state of the worker thread,
2) stops accessing |m_globalScope| from the main thread for detecting worker
thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to clarify that these fields represent not
a state of the worker thread but facts that the worker thread has been
requested to start/terminate from the main thread, and
4) updates a comment about |m_threadStateMutex| as follows:
- Mention of |m_microtaskRunner| is removed because it's accessed only from
the worker thread and it's not necessary to protect it in the first place.
- Mention of |m_globalScope| is removed because of 2).
BUG=638877, 639153
==========
Description was changed from
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents a state of the worker thread,
2) stops accessing |m_globalScope| from the main thread for detecting worker
thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to clarify that these fields represent not
a state of the worker thread but facts that the worker thread has been
requested to start/terminate from the main thread, and
4) updates a comment about |m_threadStateMutex| as follows:
- Mention of |m_microtaskRunner| is removed because it's accessed only from
the worker thread and it's not necessary to protect it in the first place.
- Mention of |m_globalScope| is removed because of 2).
BUG=638877, 639153
==========
to
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents a state of the worker thread,
2) stops accessing |m_globalScope| from the main thread for detecting worker
thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to clarify that these fields represent not
a state of the worker thread but facts that the worker thread has been
requested to start/terminate from the main thread, and
4) updates a comment about |m_threadStateMutex| as follows:
- Mention of |m_microtaskRunner| is removed because it's accessed only from
the worker thread and it's not necessary to protect the field.
- Mention of |m_globalScope| is removed because of 2).
BUG=638877, 639153
==========
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents a state of the worker thread,
2) stops accessing |m_globalScope| from the main thread for detecting worker
thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to clarify that these fields represent not
a state of the worker thread but facts that the worker thread has been
requested to start/terminate from the main thread, and
4) updates a comment about |m_threadStateMutex| as follows:
- Mention of |m_microtaskRunner| is removed because it's accessed only from
the worker thread and it's not necessary to protect the field.
- Mention of |m_globalScope| is removed because of 2).
BUG=638877, 639153
==========
to
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents a state of the worker thread,
2) stops accessing |m_globalScope| from the main thread for detecting worker
thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to emphasize that these fields represent
not a state of the worker thread but facts that the worker thread has been
requested to start/terminate from the main thread, and
4) updates a comment about |m_threadStateMutex| as follows:
- Mention of |m_microtaskRunner| is removed because it's accessed only from
the worker thread and it's not necessary to protect the field.
- Mention of |m_globalScope| is removed because of 2).
BUG=638877, 639153
==========
nhiroki@chromium.org changed reviewers: + haraken@chromium.org, yhirano@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2268183005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2268183005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.h:217: // - The main thread can read this with the lock. This looks a bit complicated. Would it be possible to take one of the following options? - Acquire the lock unconditionally. - Disallow the main thread access m_threadState.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2268183005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2268183005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.h:217: // - The main thread can read this with the lock. On 2016/08/24 06:07:44, haraken wrote: > > This looks a bit complicated. Would it be possible to take one of the following > options? > > - Acquire the lock unconditionally. I'd prefer to avoid acquiring a lock from a worker thread in some cases because there was a deadlock problem before: https://chromium.googlesource.com/chromium/src/+/18d1be3c48e503e5d080361f8fe6... > - Disallow the main thread access m_threadState. This could be difficult. For example, ForceTerminationTask::run() needs to check this member on the main thread.
On 2016/08/24 08:14:05, nhiroki wrote: > https://codereview.chromium.org/2268183005/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/workers/WorkerThread.h (right): > > https://codereview.chromium.org/2268183005/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/workers/WorkerThread.h:217: // - The main > thread can read this with the lock. > On 2016/08/24 06:07:44, haraken wrote: > > > > This looks a bit complicated. Would it be possible to take one of the > following > > options? > > > > - Acquire the lock unconditionally. > > I'd prefer to avoid acquiring a lock from a worker thread in some cases because > there was a deadlock problem before: > https://chromium.googlesource.com/chromium/src/+/18d1be3c48e503e5d080361f8fe6... > > > - Disallow the main thread access m_threadState. > > This could be difficult. For example, ForceTerminationTask::run() needs to check > this member on the main thread. Thanks for the clarification! LGTM.
JFYI: > 4) updates a comment about |m_threadStateMutex| as follows: > - Mention of |m_microtaskRunner| is removed because it's accessed only from > the worker thread and it's not necessary to protect the field. > - Mention of |m_globalScope| is removed because of 2). PatchSet3 reverted "Mention of |m_globalScope| is removed because of 2)." because a prerequisite CL was reverted: https://codereview.chromium.org/2277703002/
Description was changed from
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents a state of the worker thread,
2) stops accessing |m_globalScope| from the main thread for detecting worker
thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to emphasize that these fields represent
not a state of the worker thread but facts that the worker thread has been
requested to start/terminate from the main thread, and
4) updates a comment about |m_threadStateMutex| as follows:
- Mention of |m_microtaskRunner| is removed because it's accessed only from
the worker thread and it's not necessary to protect the field.
- Mention of |m_globalScope| is removed because of 2).
BUG=638877, 639153
==========
to
==========
Worker: Refine state management of WorkerThread
This CL...
1) introduces ThreadState enum that represents a state of the worker thread,
2) stops accessing |m_globalScope| from the main thread for detecting worker
thread initialization and instead checks ThreadState,
3) renames |m_started| and |m_terminated| with |m_requestedToStart| and
|m_requestedToTerminate| in order to emphasize that these fields represent
not a state of the worker thread but facts that the worker thread has been
requested to start/terminate from the main thread, and
4) removes mention of |m_microtaskRunner| from a comment about
|m_threadStateMutex| because the member is accessed only from the worker
thread.
BUG=638877, 639153
==========
lgtm
Thank you for reviewing :)
Description was changed from ========== Worker: Refine state management of WorkerThread This CL... 1) introduces ThreadState enum that represents a state of the worker thread, 2) stops accessing |m_globalScope| from the main thread for detecting worker thread initialization and instead checks ThreadState, 3) renames |m_started| and |m_terminated| with |m_requestedToStart| and |m_requestedToTerminate| in order to emphasize that these fields represent not a state of the worker thread but facts that the worker thread has been requested to start/terminate from the main thread, and 4) removes mention of |m_microtaskRunner| from a comment about |m_threadStateMutex| because the member is accessed only from the worker thread. BUG=638877, 639153 ========== to ========== Worker: Refine state management of WorkerThread This CL... 1) introduces ThreadState enum that represents a state of the worker thread, 2) stops accessing |m_globalScope| from the main thread for detecting worker thread initialization and instead checks ThreadState, 3) renames |m_started| and |m_terminated| with |m_requestedToStart| and |m_requestedToTerminate| in order to emphasize that these fields represent not a state of the worker thread but facts that the worker thread has been requested to start/terminate from the main thread, and 4) removes mention of |m_microtaskRunner| from a comment about |m_threadStateMutex| because the member is accessed only from the worker thread. BUG=638877, 640843 ==========
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2268183005/#ps120001 (title: "remove TODO comment to avoid patch confliction")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Worker: Refine state management of WorkerThread This CL... 1) introduces ThreadState enum that represents a state of the worker thread, 2) stops accessing |m_globalScope| from the main thread for detecting worker thread initialization and instead checks ThreadState, 3) renames |m_started| and |m_terminated| with |m_requestedToStart| and |m_requestedToTerminate| in order to emphasize that these fields represent not a state of the worker thread but facts that the worker thread has been requested to start/terminate from the main thread, and 4) removes mention of |m_microtaskRunner| from a comment about |m_threadStateMutex| because the member is accessed only from the worker thread. BUG=638877, 640843 ========== to ========== Worker: Refine state management of WorkerThread This CL... 1) introduces ThreadState enum that represents a state of the worker thread, 2) stops accessing |m_globalScope| from the main thread for detecting worker thread initialization and instead checks ThreadState, 3) renames |m_started| and |m_terminated| with |m_requestedToStart| and |m_requestedToTerminate| in order to emphasize that these fields represent not a state of the worker thread but facts that the worker thread has been requested to start/terminate from the main thread, and 4) removes mention of |m_microtaskRunner| from a comment about |m_threadStateMutex| because the member is accessed only from the worker thread. BUG=638877, 640843 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Worker: Refine state management of WorkerThread This CL... 1) introduces ThreadState enum that represents a state of the worker thread, 2) stops accessing |m_globalScope| from the main thread for detecting worker thread initialization and instead checks ThreadState, 3) renames |m_started| and |m_terminated| with |m_requestedToStart| and |m_requestedToTerminate| in order to emphasize that these fields represent not a state of the worker thread but facts that the worker thread has been requested to start/terminate from the main thread, and 4) removes mention of |m_microtaskRunner| from a comment about |m_threadStateMutex| because the member is accessed only from the worker thread. BUG=638877, 640843 ========== to ========== Worker: Refine state management of WorkerThread This CL... 1) introduces ThreadState enum that represents a state of the worker thread, 2) stops accessing |m_globalScope| from the main thread for detecting worker thread initialization and instead checks ThreadState, 3) renames |m_started| and |m_terminated| with |m_requestedToStart| and |m_requestedToTerminate| in order to emphasize that these fields represent not a state of the worker thread but facts that the worker thread has been requested to start/terminate from the main thread, and 4) removes mention of |m_microtaskRunner| from a comment about |m_threadStateMutex| because the member is accessed only from the worker thread. BUG=638877, 640843 Committed: https://crrev.com/aefae76fee3afe6cc9eabbe5d06cde009d317c16 Cr-Commit-Position: refs/heads/master@{#414336} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aefae76fee3afe6cc9eabbe5d06cde009d317c16 Cr-Commit-Position: refs/heads/master@{#414336} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
