Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1846913004:
MediaLog: Replace task posting in AddEvent with lock (Closed)
|
Created:
4 years, 8 months ago by wolenetz Modified:
4 years, 8 months ago Reviewers:
watk CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, xhwang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaLog: Replace task posting in AddEvent with lock
Uses a lock to maintain coherency of sequence of AddEvent(). This
change will help upcoming change(s) that need a coherent view of the
most recently added pipeline and medialog error messages, regardless of
which thread calls AddEvent() and which thread queries for the error
message.
BUG=472253
Committed: https://crrev.com/5038b564d4d05a4e52b0091e005dc86de9bbb4d6
Cr-Commit-Position: refs/heads/master@{#385024}
Patch Set 1 #Patch Set 2 : Initializes |ipc_send_pending_|. Comment clean-ups. #
Total comments: 8
Patch Set 3 : Rebase #Patch Set 4 : Addresses all comments so far. #
Total comments: 6
Patch Set 5 : Addresses watk@'s comments from PS4. #
Dependent Patchsets: Messages
Total messages: 31 (14 generated)
Description was changed from ========== MediaLog: Replace task posting in AddEvent with lock Uses a lock to maintain coherency of sequence of AddEvent(). This change will help upcoming change(s) that need a coherent view of the most recently added pipeline and medialog error messages, regardless of which thread calls AddEvent() and which thread queries for the error message. BUG=472253 ========== to ========== MediaLog: Replace task posting in AddEvent with lock Uses a lock to maintain coherency of sequence of AddEvent(). This change will help upcoming change(s) that need a coherent view of the most recently added pipeline and medialog error messages, regardless of which thread calls AddEvent() and which thread queries for the error message. BUG=472253 ==========
wolenetz@chromium.org changed reviewers: + xhwang@chromium.org
Please take a look at patch set 2. Thanks!
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846913004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846913004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking good. I just have a few nits/discussions. https://chromiumcodereview.appspot.com/1846913004/diff/20001/content/renderer... File content/renderer/media/render_media_log.cc (right): https://chromiumcodereview.appspot.com/1846913004/diff/20001/content/renderer... content/renderer/media/render_media_log.cc:48: Log(event.get()); nit: This can be outside of the lock. https://chromiumcodereview.appspot.com/1846913004/diff/20001/content/renderer... content/renderer/media/render_media_log.cc:97: last_ipc_send_time_ = tick_clock_->NowTicks(); nit: Send involves serialization and could be slow (depending how many messages we have). Can we use vector::swap to swap the queued_media_events_ to a local one, update the last_ipc_send_time_, and then release the lock. Then we can Send() the local one outside the lock. https://chromiumcodereview.appspot.com/1846913004/diff/20001/content/renderer... content/renderer/media/render_media_log.cc:112: base::AutoLock auto_lock(lock_); It seems odd the protect the task runner especially since we are not always protecting the use of task_runner under a lock. https://chromiumcodereview.appspot.com/1846913004/diff/20001/content/renderer... File content/renderer/media/render_media_log.h (right): https://chromiumcodereview.appspot.com/1846913004/diff/20001/content/renderer... content/renderer/media/render_media_log.h:48: mutable base::Lock lock_; Could you please be more specific about what this lock is protecting?
wolenetz@chromium.org changed reviewers: + watk@chromium.org - xhwang@chromium.org
Thanks for comments xhwang@. I believe I have addressed all of them in PS4. watk@, since xhwang@ is OoO this week, can you please continue the CR of this? https://codereview.chromium.org/1846913004/diff/20001/content/renderer/media/... File content/renderer/media/render_media_log.cc (right): https://codereview.chromium.org/1846913004/diff/20001/content/renderer/media/... content/renderer/media/render_media_log.cc:48: Log(event.get()); On 2016/04/01 05:42:55, xhwang (OOO Apr4 - Apr8) wrote: > nit: This can be outside of the lock. Done. https://codereview.chromium.org/1846913004/diff/20001/content/renderer/media/... content/renderer/media/render_media_log.cc:97: last_ipc_send_time_ = tick_clock_->NowTicks(); On 2016/04/01 05:42:55, xhwang (OOO Apr4 - Apr8) wrote: > nit: Send involves serialization and could be slow (depending how many messages > we have). Can we use vector::swap to swap the queued_media_events_ to a local > one, update the last_ipc_send_time_, and then release the lock. Then we can > Send() the local one outside the lock. Good idea. Note that if Send() becomes severely slow, multiple Sends() of the events can be in flight at the same time. That shouldn't violate expectations though, since we'd want to catch up on sending events ASAP if we're behind so much. Done. https://codereview.chromium.org/1846913004/diff/20001/content/renderer/media/... content/renderer/media/render_media_log.cc:112: base::AutoLock auto_lock(lock_); On 2016/04/01 05:42:55, xhwang (OOO Apr4 - Apr8) wrote: > It seems odd the protect the task runner especially since we are not always > protecting the use of task_runner under a lock. Yeah. This does seem odd. I'll not lock in this method. Done. https://codereview.chromium.org/1846913004/diff/20001/content/renderer/media/... File content/renderer/media/render_media_log.h (right): https://codereview.chromium.org/1846913004/diff/20001/content/renderer/media/... content/renderer/media/render_media_log.h:48: mutable base::Lock lock_; On 2016/04/01 05:42:55, xhwang (OOO Apr4 - Apr8) wrote: > Could you please be more specific about what this lock is protecting? Done.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846913004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846913004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... File content/renderer/media/render_media_log.cc (right): https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... content/renderer/media/render_media_log.cc:67: task_runner_->PostDelayedTask( Looks like this doesn't necessarily have to be done while holding the lock. I don't think it's a big deal to leave it though. https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... content/renderer/media/render_media_log.cc:79: FROM_HERE, base::Bind(&RenderMediaLog::SendQueuedMediaEvents, this)); braces for multi-line statement https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... File content/renderer/media/render_media_log.h (right): https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... content/renderer/media/render_media_log.h:54: // |task_runner_|. nit: "throttled send by |task_runner_|" -> "throttled send on |task_runner_|".
Thanks for review. Please take a look at patch set 5. https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... File content/renderer/media/render_media_log.cc (right): https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... content/renderer/media/render_media_log.cc:67: task_runner_->PostDelayedTask( On 2016/04/04 20:32:21, watk wrote: > Looks like this doesn't necessarily have to be done while holding the lock. I > don't think it's a big deal to leave it though. Moved outside the lock. https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... content/renderer/media/render_media_log.cc:79: FROM_HERE, base::Bind(&RenderMediaLog::SendQueuedMediaEvents, this)); On 2016/04/04 20:32:21, watk wrote: > braces for multi-line statement Good catch. git cl format split the line but didn't add {}. https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... File content/renderer/media/render_media_log.h (right): https://codereview.chromium.org/1846913004/diff/60001/content/renderer/media/... content/renderer/media/render_media_log.h:54: // |task_runner_|. On 2016/04/04 20:32:21, watk wrote: > nit: "throttled send by |task_runner_|" -> "throttled send on |task_runner_|". Done.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846913004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846913004/80001
lgtm
The CQ bit was unchecked by wolenetz@chromium.org
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846913004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846913004/80001
The CQ bit was unchecked by wolenetz@chromium.org
I'm going to hold off on landing this - I'm seeing some unexpected crashes in blink layout tests (see https://codereview.chromium.org/1850733003/#ps80001 and https://luci-logdog.appspot.com/v/?s=bb%2Ftryserver.chromium.linux%2Flinux_bl...)
On 2016/04/04 21:20:07, wolenetz wrote: > I'm going to hold off on landing this - I'm seeing some unexpected crashes in > blink layout tests (see https://codereview.chromium.org/1850733003/#ps80001 and > https://luci-logdog.appspot.com/v/?s=bb%2Ftryserver.chromium.linux%2Flinux_bl...) It looks like the crash occurs only on that subsequent CL. I'll go ahead and land this one now.
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846913004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846913004/80001
Message was sent while issue was closed.
Description was changed from ========== MediaLog: Replace task posting in AddEvent with lock Uses a lock to maintain coherency of sequence of AddEvent(). This change will help upcoming change(s) that need a coherent view of the most recently added pipeline and medialog error messages, regardless of which thread calls AddEvent() and which thread queries for the error message. BUG=472253 ========== to ========== MediaLog: Replace task posting in AddEvent with lock Uses a lock to maintain coherency of sequence of AddEvent(). This change will help upcoming change(s) that need a coherent view of the most recently added pipeline and medialog error messages, regardless of which thread calls AddEvent() and which thread queries for the error message. BUG=472253 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== MediaLog: Replace task posting in AddEvent with lock Uses a lock to maintain coherency of sequence of AddEvent(). This change will help upcoming change(s) that need a coherent view of the most recently added pipeline and medialog error messages, regardless of which thread calls AddEvent() and which thread queries for the error message. BUG=472253 ========== to ========== MediaLog: Replace task posting in AddEvent with lock Uses a lock to maintain coherency of sequence of AddEvent(). This change will help upcoming change(s) that need a coherent view of the most recently added pipeline and medialog error messages, regardless of which thread calls AddEvent() and which thread queries for the error message. BUG=472253 Committed: https://crrev.com/5038b564d4d05a4e52b0091e005dc86de9bbb4d6 Cr-Commit-Position: refs/heads/master@{#385024} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5038b564d4d05a4e52b0091e005dc86de9bbb4d6 Cr-Commit-Position: refs/heads/master@{#385024} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
