Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 9 months ago by tommi Modified:
3 years, 9 months ago Reviewers:
the sun CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactor Windows TaskQueue code to only need a single high res timer.
BUG=webrtc:7151
Review-Url: https://codereview.webrtc.org/2733723002
Cr-Commit-Position: refs/heads/master@{#17170}
Committed: https://chromium.googlesource.com/external/webrtc/+/0b942150d35c58bab4efe6dc389489e01f3126a8
Patch Set 1 #Patch Set 2 : Updates #Patch Set 3 : Update comments #Patch Set 4 : Make sure timers are cancelled before rescheduling #Patch Set 5 : Change the PostMultipleDelayed test to fire tasks targeted at different intervals #
Total comments: 17
Patch Set 6 : Switch over to priority_queue #Patch Set 7 : Address other comments #
Total comments: 6
Patch Set 8 : Address comments and fix order in priority queue #Patch Set 9 : Rebase #Patch Set 10 : use greater<> and not greater_or_equal #
Created: 3 years, 9 months ago
Messages
Total messages: 41 (24 generated)
Updates
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
Update comments
Make sure timers are cancelled before rescheduling
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
Description was changed from ========== Refactor Windows TaskQueue code to only need a single timer. BUG=webrtc:7151 ========== to ========== Refactor Windows TaskQueue code to only need a single high res timer. BUG=webrtc:7151 ==========
Change the PostMultipleDelayed test to fire tasks targeted at different intervals
tommi@webrtc.org changed reviewers: + solenberg@webrtc.org
Hej Fredrik - ptal. The windows multimedia timer code strikes again.
lg - spotted one thing that seems wrong, the rest are just nits. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue.h File webrtc/base/task_queue.h (right): https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue.h#... webrtc/base/task_queue.h:16: #include <vector> Hmm. I don't find a reference to vector in this file? https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... File webrtc/base/task_queue_win.cc (right): https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:84: DelayedTaskInfo(DelayedTaskInfo&&) = default; Maybe delete operator= too? https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:86: // Implement for <set>. to maintain an order of increasing |due_time|. nit: , after <set> https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:112: MultimediaTimer(MultimediaTimer&&) = delete; Strictly speaking I don't think this is needed: http://en.cppreference.com/w/cpp/language/move_constructor ...but I don't mind being overly clear. However, then it seems operator=(&&) should be explicitly deleted as well. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:160: std::multiset<DelayedTaskInfo> timer_tasks_; It's not clear to me that multiset is the best structure here. For a small set of pending tasks, wouldn't it be outperformed by a priority_queue/heap (which would over time avoid heap allocs)? The devil is in the profile data... https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:216: auto* task_info = new DelayedTaskInfo(milliseconds, std::move(task)); Add a TODO that we should remove this heap alloc. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:329: timer_tasks_.erase(timer_tasks_.begin(), it); I assume nothing will be erased from the set if both arguments are at .begin(), but docs I found are ambiguous. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:338: static_cast<uint32_t>(timer_tasks_.begin()->due_time - GetTick()); It seems to me you need to manage the case when we get a negative difference here. It's not the same GetTick() call, and result, that you got when purging due tasks.
Switch over to priority_queue
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
Address other comments
https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue.h File webrtc/base/task_queue.h (right): https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue.h#... webrtc/base/task_queue.h:16: #include <vector> On 2017/03/07 11:24:24, the sun wrote: > Hmm. I don't find a reference to vector in this file? Removed. (I added it while it was supposed to have been there for ProcessQueuedMessages... alas that was then removed!) https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... File webrtc/base/task_queue_win.cc (right): https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:84: DelayedTaskInfo(DelayedTaskInfo&&) = default; On 2017/03/07 11:24:24, the sun wrote: > Maybe delete operator= too? Done. (added RTC_DISALLOW_COPY_AND_ASSIGN) https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:86: // Implement for <set>. to maintain an order of increasing |due_time|. On 2017/03/07 11:24:24, the sun wrote: > nit: , after <set> Done. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:112: MultimediaTimer(MultimediaTimer&&) = delete; On 2017/03/07 11:24:24, the sun wrote: > Strictly speaking I don't think this is needed: > http://en.cppreference.com/w/cpp/language/move_constructor > > ...but I don't mind being overly clear. However, then it seems operator=(&&) > should be explicitly deleted as well. Removed. I only needed it as I was changing the previous code and wanted to make sure we didn't require this somewhere. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:160: std::multiset<DelayedTaskInfo> timer_tasks_; On 2017/03/07 11:24:24, the sun wrote: > It's not clear to me that multiset is the best structure here. For a small set > of pending tasks, wouldn't it be outperformed by a priority_queue/heap (which > would over time avoid heap allocs)? The devil is in the profile data... Done. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:216: auto* task_info = new DelayedTaskInfo(milliseconds, std::move(task)); On 2017/03/07 11:24:24, the sun wrote: > Add a TODO that we should remove this heap alloc. Done. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:329: timer_tasks_.erase(timer_tasks_.begin(), it); On 2017/03/07 11:24:24, the sun wrote: > I assume nothing will be erased from the set if both arguments are at .begin(), > but docs I found are ambiguous. Yes - however, the implementation has changed slightly after switching over to priority_queue. I did add an RTC_DCHECK() at the top to show that the queue should never be empty when the function is called. https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:338: static_cast<uint32_t>(timer_tasks_.begin()->due_time - GetTick()); On 2017/03/07 11:24:24, the sun wrote: > It seems to me you need to manage the case when we get a negative difference > here. It's not the same GetTick() call, and result, that you got when purging > due tasks. Good catch. Done.
https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... File webrtc/base/task_queue_win.cc (right): https://codereview.webrtc.org/2733723002/diff/80001/webrtc/base/task_queue_wi... webrtc/base/task_queue_win.cc:84: DelayedTaskInfo(DelayedTaskInfo&&) = default; On 2017/03/09 20:27:42, tommi (webrtc) wrote: > On 2017/03/07 11:24:24, the sun wrote: > > Maybe delete operator= too? > > Done. (added RTC_DISALLOW_COPY_AND_ASSIGN) oops, discard that. I had to support the operator and default ctor after switching to priority_queue (due to pop()).
LGTM! https://codereview.webrtc.org/2733723002/diff/120001/webrtc/base/task_queue_w... File webrtc/base/task_queue_win.cc (right): https://codereview.webrtc.org/2733723002/diff/120001/webrtc/base/task_queue_w... webrtc/base/task_queue_win.cc:88: // Implement for <set> to maintain an order of increasing |due_time_|. nit: <set> not correct anymore https://codereview.webrtc.org/2733723002/diff/120001/webrtc/base/task_queue_w... webrtc/base/task_queue_win.cc:336: if (top.due_time() > now) Is it better to check against GetTick() here, in case a task takes considerable time to run? I believe that was what the previous PS did. What is the reason for changing? https://codereview.webrtc.org/2733723002/diff/120001/webrtc/base/task_queue_w... webrtc/base/task_queue_win.cc:348: auto now = GetTick(); nit: int64_t delay_ms = std::max(0, timer_tasks.top().due_time() - GetTick()); uint32_t milliseconds = rtc::dchecked_cast<uint32_t>(delay_ms); (however unlikely, we may theoretically have overflow at the other end)
Address comments and fix order in priority queue
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
https://codereview.webrtc.org/2733723002/diff/120001/webrtc/base/task_queue_w... File webrtc/base/task_queue_win.cc (right): https://codereview.webrtc.org/2733723002/diff/120001/webrtc/base/task_queue_w... webrtc/base/task_queue_win.cc:88: // Implement for <set> to maintain an order of increasing |due_time_|. On 2017/03/10 11:48:02, the sun wrote: > nit: <set> not correct anymore Done. https://codereview.webrtc.org/2733723002/diff/120001/webrtc/base/task_queue_w... webrtc/base/task_queue_win.cc:336: if (top.due_time() > now) On 2017/03/10 11:48:02, the sun wrote: > Is it better to check against GetTick() here, in case a task takes considerable > time to run? > > I believe that was what the previous PS did. What is the reason for changing? The thinking in the previous patch set was exactly in anticipation of tasks running for some time and then running tasks that became due while a previous task was running. The reason I changed it was because I looked at task execution time in Chrome and in most cases they complete in fraction of a millisecond. Tasks such as encode/decode take longer (we actually don't use a task queue for decoding right now), but those are exception cases. For delayed tasks, it is such a rare thing that they be happening back to back that it felt like optimizing for the wrong thing and instead I could just call GetTick once and still swat multiple tasks with one stroke, assuming they could complete within the same millisecond. https://codereview.webrtc.org/2733723002/diff/120001/webrtc/base/task_queue_w... webrtc/base/task_queue_win.cc:348: auto now = GetTick(); On 2017/03/10 11:48:02, the sun wrote: > nit: > int64_t delay_ms = std::max(0, timer_tasks.top().due_time() - GetTick()); > uint32_t milliseconds = rtc::dchecked_cast<uint32_t>(delay_ms); > > (however unlikely, we may theoretically have overflow at the other end) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/17138)
Rebase
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/7193)
use greater<> and not greater_or_equal
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/20694)
The CQ bit was checked by tommi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2733723002/#ps180001 (title: "use greater<> and not greater_or_equal")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489166583740640,
"parent_rev": "15939e777524eaa3fdb107c8b5f996d0ffaf6c50", "commit_rev":
"0b942150d35c58bab4efe6dc389489e01f3126a8"}
Message was sent while issue was closed.
Description was changed from ========== Refactor Windows TaskQueue code to only need a single high res timer. BUG=webrtc:7151 ========== to ========== Refactor Windows TaskQueue code to only need a single high res timer. BUG=webrtc:7151 Review-Url: https://codereview.webrtc.org/2733723002 Cr-Commit-Position: refs/heads/master@{#17170} Committed: https://chromium.googlesource.com/external/webrtc/+/0b942150d35c58bab4efe6dc3... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/0b942150d35c58bab4efe6dc3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
