Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716163002:
MessagePort: don't post repeated message dispatch tasks. (Closed)
DescriptionMessagePort: don't post repeated message dispatch tasks.
If a task has already been posted to drain the incoming queue of messages,
don't post another.
R=haraken,dcheng
BUG=693595
Review-Url: https://codereview.chromium.org/2716163002
Cr-Commit-Position: refs/heads/master@{#453205}
Committed: https://chromium.googlesource.com/chromium/src/+/f823ed66c7c348e46f84b95e6c550fc6e827f14e
Patch Set 1 #Patch Set 2 : improve comments #
Total comments: 2
Messages
Total messages: 21 (14 generated)
The CQ bit was checked by sigbjornf@opera.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== MessagePort: don't post repeated message dispatch tasks. If a task has already been posted to drain the incoming queue of messages, don't post another. R= BUG= ========== to ========== MessagePort: don't post repeated message dispatch tasks. If a task has already been posted to drain the incoming queue of messages, don't post another. R= BUG=693595 ==========
The CQ bit was checked by sigbjornf@opera.com 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...
sigbjornf@opera.com changed reviewers: + darin@chormium.org, dcheng@chromium.org, haraken@chromium.org
please take a look.
LGTM
lgtm https://codereview.chromium.org/2716163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/MessagePort.cpp (right): https://codereview.chromium.org/2716163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/MessagePort.cpp:123: if (atomicTestAndSetToOne(&m_pendingDispatchTask)) It really feels like we should have a better way of doing this (since there were no callers of this function before this CL), but maybe it's not a very common situation.
https://codereview.chromium.org/2716163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/MessagePort.cpp (right): https://codereview.chromium.org/2716163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/MessagePort.cpp:123: if (atomicTestAndSetToOne(&m_pendingDispatchTask)) On 2017/02/27 08:06:37, dcheng wrote: > It really feels like we should have a better way of doing this (since there were > no callers of this function before this CL), but maybe it's not a very common > situation. "this" == cross-thread prevention of task re-posts..? I haven't come across anything similar in Blink, fwiw. (Are we allowed to use std::atomic in WebKit/Source/, btw?)
On 2017/02/27 08:21:37, sof wrote: > https://codereview.chromium.org/2716163002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/MessagePort.cpp (right): > > https://codereview.chromium.org/2716163002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/MessagePort.cpp:123: if > (atomicTestAndSetToOne(&m_pendingDispatchTask)) > On 2017/02/27 08:06:37, dcheng wrote: > > It really feels like we should have a better way of doing this (since there > were > > no callers of this function before this CL), but maybe it's not a very common > > situation. > > "this" == cross-thread prevention of task re-posts..? I haven't come across > anything similar in Blink, fwiw. Fair enough. > > (Are we allowed to use std::atomic in WebKit/Source/, btw?) Per https://chromium-cpp.appspot.com/, atomics are still on the banned list until someone figures out why it regressed perf.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== MessagePort: don't post repeated message dispatch tasks. If a task has already been posted to drain the incoming queue of messages, don't post another. R= BUG=693595 ========== to ========== MessagePort: don't post repeated message dispatch tasks. If a task has already been posted to drain the incoming queue of messages, don't post another. R=haraken,dcheng BUG=693595 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488204610602960,
"parent_rev": "aa0576d5b202a536cae3400a9c5f6e76aaf39f0d", "commit_rev":
"f823ed66c7c348e46f84b95e6c550fc6e827f14e"}
Message was sent while issue was closed.
Description was changed from ========== MessagePort: don't post repeated message dispatch tasks. If a task has already been posted to drain the incoming queue of messages, don't post another. R=haraken,dcheng BUG=693595 ========== to ========== MessagePort: don't post repeated message dispatch tasks. If a task has already been posted to drain the incoming queue of messages, don't post another. R=haraken,dcheng BUG=693595 Review-Url: https://codereview.chromium.org/2716163002 Cr-Commit-Position: refs/heads/master@{#453205} Committed: https://chromium.googlesource.com/chromium/src/+/f823ed66c7c348e46f84b95e6c55... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f823ed66c7c348e46f84b95e6c55... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
