Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2464383002:
Use queue label as id in SequencedTaskChecker when not running on TaskQueue (Closed)
|
Created:
4 years, 1 month ago by kthelgason Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse queue label as id in SequencedTaskChecker when not running on TaskQueue
This is intended to make SequencedTaskChecker work for native dispatch queues
on iOS and macOS. These labels can be compared by their pointers to determine
if a task is running on the same queue.
BUG=webrtc:6643
Committed: https://crrev.com/44e0efe0e67797702c0a71e3798d2d2c047f1368
Cr-Commit-Position: refs/heads/master@{#14900}
Patch Set 1 #
Total comments: 8
Patch Set 2 : code review #
Messages
Total messages: 18 (8 generated)
kthelgason@webrtc.org changed reviewers: + magjed@webrtc.org, tommi@webrtc.org
The CQ bit was checked by kthelgason@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: This issue passed the CQ dry run.
Perfect! This is exactly what I wanted and the implementation is very clean. https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... File webrtc/base/sequenced_task_checker_impl.cc (right): https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... webrtc/base/sequenced_task_checker_impl.cc:28: // If we're not running on a TaskQueue, use the system dispatch queue nit: I would like the comments above the if-statement instead. https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... webrtc/base/sequenced_task_checker_impl.cc:30: current_queue = dispatch_queue_get_label(DISPATCH_CURRENT_QUEUE_LABEL); We don't need any include for this magical dispatch_queue_get_label?
Oh, lgtm.
lgtm with comments addressed. btw, following up on our discussion earlier today I dug through the docs a little and found that for TaskQueue, we can improve the Current() implementation. Instead of using Tls as we're doing now, we can use GCD's get/set specific APIs. Those APIs are GCD equivalents of the pthread APIs with similar names. https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... File webrtc/base/sequenced_task_checker_impl.cc (right): https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... webrtc/base/sequenced_task_checker_impl.cc:28: // If we're not running on a TaskQueue, use the system dispatch queue On 2016/11/02 12:04:41, magjed_webrtc wrote: > nit: I would like the comments above the if-statement instead. agree. style comment: as is, this should use {} since the body of the conditional, including comments, is more than one line. https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... webrtc/base/sequenced_task_checker_impl.cc:30: current_queue = dispatch_queue_get_label(DISPATCH_CURRENT_QUEUE_LABEL); On 2016/11/02 12:04:41, magjed_webrtc wrote: > We don't need any include for this magical dispatch_queue_get_label? good point. We need #include <dispatch/dispatch.h> which is transitively included right now via task_queue.h. https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... File webrtc/base/sequenced_task_checker_impl.h (right): https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... webrtc/base/sequenced_task_checker_impl.h:26: typedef const void* QueueId; can we make this private?
https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... File webrtc/base/sequenced_task_checker_impl.cc (right): https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... webrtc/base/sequenced_task_checker_impl.cc:28: // If we're not running on a TaskQueue, use the system dispatch queue On 2016/11/02 16:32:50, tommi (webrtc) wrote: > On 2016/11/02 12:04:41, magjed_webrtc wrote: > > nit: I would like the comments above the if-statement instead. > > agree. > style comment: as is, this should use {} since the body of the conditional, > including comments, is more than one line. Done. https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... webrtc/base/sequenced_task_checker_impl.cc:30: current_queue = dispatch_queue_get_label(DISPATCH_CURRENT_QUEUE_LABEL); On 2016/11/02 16:32:50, tommi (webrtc) wrote: > On 2016/11/02 12:04:41, magjed_webrtc wrote: > > We don't need any include for this magical dispatch_queue_get_label? > > good point. We need #include <dispatch/dispatch.h> which is transitively > included right now via task_queue.h. Done. https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... File webrtc/base/sequenced_task_checker_impl.h (right): https://codereview.webrtc.org/2464383002/diff/1/webrtc/base/sequenced_task_ch... webrtc/base/sequenced_task_checker_impl.h:26: typedef const void* QueueId; On 2016/11/02 16:32:50, tommi (webrtc) wrote: > can we make this private? Done.
On 2016/11/02 16:32:50, tommi (webrtc) wrote: > btw, following up on our discussion earlier today I dug through the docs a > little and found that for TaskQueue, we can improve the Current() > implementation. Instead of using Tls as we're doing now, we can use GCD's > get/set specific APIs. Those APIs are GCD equivalents of the pthread APIs with > similar names. I knew about those APIs, in fact I had an implementation pretty much finished before I realised that would not help in this case. After our discussion I saw no reason to continue down that path.
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2464383002/#ps20001 (title: "code review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use queue label as id in SequencedTaskChecker when not running on TaskQueue This is intended to make SequencedTaskChecker work for native dispatch queues on iOS and macOS. These labels can be compared by their pointers to determine if a task is running on the same queue. BUG=webrtc:6643 ========== to ========== Use queue label as id in SequencedTaskChecker when not running on TaskQueue This is intended to make SequencedTaskChecker work for native dispatch queues on iOS and macOS. These labels can be compared by their pointers to determine if a task is running on the same queue. BUG=webrtc:6643 Committed: https://crrev.com/44e0efe0e67797702c0a71e3798d2d2c047f1368 Cr-Commit-Position: refs/heads/master@{#14900} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/44e0efe0e67797702c0a71e3798d2d2c047f1368 Cr-Commit-Position: refs/heads/master@{#14900}
Message was sent while issue was closed.
On 2016/11/02 16:58:15, kthelgason wrote: > On 2016/11/02 16:32:50, tommi (webrtc) wrote: > > btw, following up on our discussion earlier today I dug through the docs a > > little and found that for TaskQueue, we can improve the Current() > > implementation. Instead of using Tls as we're doing now, we can use GCD's > > get/set specific APIs. Those APIs are GCD equivalents of the pthread APIs > with > > similar names. > > I knew about those APIs, in fact I had an implementation pretty much finished > before I realised that would not help in this case. After our discussion I saw > no reason to continue down that path. Great. I guess it'll just make things a little neater. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
