Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259013003:
Move and rename TaskTimeTracker to public interface exposed to WebThread, use in WebPerf Agent. (Closed)
|
Created:
4 years, 4 months ago by panicker Modified:
4 years, 3 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, scheduler-bugs_chromium.org, blink-reviews-api_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGeneralize TaskTimeTracker interface and expose WebThread. This is now used by InspectorWebPerfAgent for Long Task Observer.
BUG=635596
Committed: https://crrev.com/d9caeb1b95bbdfab5aa7688e6ce143f018f58178
Cr-Commit-Position: refs/heads/master@{#415051}
Patch Set 1 #Patch Set 2 : fix indent #Patch Set 3 : remove TaskTimeObserver in destructor of WebPerf Agent #Patch Set 4 : remaining fixes for remove observer #
Total comments: 7
Patch Set 5 : Switch TaskTimeTracker over to WebThread::TaskTimeObserver #
Total comments: 24
Patch Set 6 : Move TaskTimeTracker to public interface, expose to WebThread. #
Total comments: 18
Patch Set 7 : address review comments #Patch Set 8 : Rename TaskTimeTracker to TaskTimeObserver, and address remaining nit. #Patch Set 9 : explicitly check if task_start_time was set #
Total comments: 2
Patch Set 10 : Fix nit: move impl to scheduler_helper.cc #
Total comments: 4
Patch Set 11 : address nit: task_start_time initialization #
Total comments: 6
Patch Set 12 : Update missed SetTaskTimeObserver in blink_platform_perftests #
Total comments: 11
Patch Set 13 : address review comments #
Total comments: 1
Patch Set 14 : Track frame context URL using first script heuristic #
Messages
Total messages: 73 (28 generated)
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
Thanks, added a few comments. https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:36: void reportTaskTime(double, double) override; nit: These should have names since it's not obvious what they mean without context. https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:263: WebThread::TaskTimeObserver* task_time_observer_; // NOT OWNED Could we define this interface here instead? In principle base/ things shouldn't depend on core/ things since it creates a cyclic dependency. Also, it feels like the observer should be on the TaskTimeTracker since I think we'll also want to use it internally. https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebThread.h:62: virtual void willProcessTask() = 0; Could you remind me why we need did/willProcess again? In any case they're redundant with TaskObserver so should probably just reuse that.
https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:263: WebThread::TaskTimeObserver* task_time_observer_; // NOT OWNED On 2016/08/19 14:44:56, Sami wrote: > Could we define this interface here instead? In principle base/ things shouldn't > depend on core/ things since it creates a cyclic dependency. > > Also, it feels like the observer should be on the TaskTimeTracker since I think > we'll also want to use it internally. Could you clarify the base/-core/ issue here? Is public/platform/WebThread.h considered "core" ? BTW I first tried to repurpose the TaskTimeTracker interface (platform/scheduler/base/task_time_tracker.h), but the issue was I couldn't find a way to hook up the WebPerfAgent to it without making WebThread aware of it. i.e. in WebPerfAgent if we did something like: Platform::current()->currentThread()->addTaskTimeTracker(this); then WebThread needs to be aware of TaskTimeTracker interface. Instead we can switch the current TaskTimeTracker to merge with the TaskTimeObserver that I just added (instead of the other way around). Does that make sense? https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebThread.h:62: virtual void willProcessTask() = 0; On 2016/08/19 14:44:56, Sami wrote: > Could you remind me why we need did/willProcess again? In any case they're > redundant with TaskObserver so should probably just reuse that. This is to line up the Begin / End with Time Reporting exactly. For instance, I think I will get additional willProcess / DidProcess without corresponding reportTaskTime -- because of the !delegate check in task_queue_manager (the condition is present for reportTaskTime but not for the willProcess / DidProcess)
https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebThread.h:62: virtual void willProcessTask() = 0; On 2016/08/19 18:06:50, Shubhie wrote: > On 2016/08/19 14:44:56, Sami wrote: > > Could you remind me why we need did/willProcess again? In any case they're > > redundant with TaskObserver so should probably just reuse that. > > This is to line up the Begin / End with Time Reporting exactly. > For instance, I think I will get additional willProcess / DidProcess without > corresponding reportTaskTime -- because of the !delegate check in > task_queue_manager (the condition is present for reportTaskTime but not for the > willProcess / DidProcess) If you can suggest a completely different way to hook up WebPerfAgent to the scheduler (i.e. not involving WebThread) that might work too. Thoughts?
On 2016/08/19 18:10:53, Shubhie wrote: > https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... > File third_party/WebKit/public/platform/WebThread.h (right): > > https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... > third_party/WebKit/public/platform/WebThread.h:62: virtual void > willProcessTask() = 0; > On 2016/08/19 18:06:50, Shubhie wrote: > > On 2016/08/19 14:44:56, Sami wrote: > > > Could you remind me why we need did/willProcess again? In any case they're > > > redundant with TaskObserver so should probably just reuse that. > > > > This is to line up the Begin / End with Time Reporting exactly. > > For instance, I think I will get additional willProcess / DidProcess without > > corresponding reportTaskTime -- because of the !delegate check in > > task_queue_manager (the condition is present for reportTaskTime but not for > the > > willProcess / DidProcess) > > If you can suggest a completely different way to hook up WebPerfAgent to the > scheduler (i.e. not involving WebThread) that might work too. Thoughts? To make this discussion more concrete, I switched over TaskTimeTracker over to be a WebThread::TaskTimeObserver. WDYT?
https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebThread.h:62: virtual void willProcessTask() = 0; On 2016/08/19 18:06:50, Shubhie wrote: > On 2016/08/19 14:44:56, Sami wrote: > > Could you remind me why we need did/willProcess again? In any case they're > > redundant with TaskObserver so should probably just reuse that. > > This is to line up the Begin / End with Time Reporting exactly. > For instance, I think I will get additional willProcess / DidProcess without > corresponding reportTaskTime -- because of the !delegate check in > task_queue_manager (the condition is present for reportTaskTime but not for the > willProcess / DidProcess) The task observer can also check if it is nested. Basically you'll get another willProcess() call before the corresponding didProcess(). https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:223: FOR_EACH_OBSERVER(WebThread::TaskTimeObserver, task_time_observers_, (Indentation looks a little off -- git cl format should fix this) https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:90: void AddTaskTimeObserver(WebThread::TaskTimeObserver* task_time_observer); Earlier I really should've talked about base/ vs. not-base since as you rightly pointed out, WebThread is in platform/ too. Anyway, what I was trying to say was that scheduler/base is used to implement WebThread, so it's a little odd for it to depend on it too. How about instead we do this: 1. Add an abstract TaskTimeObserver class here (a separate header maybe). 2. Add a "using scheduler::TaskTimeObserver" in WebThread to bring it into scope (or we could just use it directly there). WDYT? https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1417: void RendererSchedulerImpl::reportTaskTime(double start_time, FYI, altimin@ is adding another task time consumer here: https://codereview.chromium.org/2265873004/
panicker@google.com changed reviewers: + panicker@google.com
https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:90: void AddTaskTimeObserver(WebThread::TaskTimeObserver* task_time_observer); On 2016/08/22 15:51:30, Sami wrote: > Earlier I really should've talked about base/ vs. not-base since as you rightly > pointed out, WebThread is in platform/ too. Anyway, what I was trying to say was > that scheduler/base is used to implement WebThread, so it's a little odd for it > to depend on it too. How about instead we do this: > > 1. Add an abstract TaskTimeObserver class here (a separate header maybe). > 2. Add a "using scheduler::TaskTimeObserver" in WebThread to bring it into scope > (or we could just use it directly there). > > WDYT? I agree it makes sense to make it a separate interface and decouple from WebThread, given what you said. However the question is whether it should live in: third_party/WebKit/public/platform/ OR third_party/WebKit/public/platform/scheduler/base/ The former seems like it would be more consistent with existing code structure and deps than latter. However I'll start with latter and see what folks think.
https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:90: void AddTaskTimeObserver(WebThread::TaskTimeObserver* task_time_observer); On 2016/08/22 16:55:45, panicker wrote: > On 2016/08/22 15:51:30, Sami wrote: > > Earlier I really should've talked about base/ vs. not-base since as you > rightly > > pointed out, WebThread is in platform/ too. Anyway, what I was trying to say > was > > that scheduler/base is used to implement WebThread, so it's a little odd for > it > > to depend on it too. How about instead we do this: > > > > 1. Add an abstract TaskTimeObserver class here (a separate header maybe). > > 2. Add a "using scheduler::TaskTimeObserver" in WebThread to bring it into > scope > > (or we could just use it directly there). > > > > WDYT? > > I agree it makes sense to make it a separate interface and decouple from > WebThread, given what you said. > However the question is whether it should live in: > third_party/WebKit/public/platform/ OR > third_party/WebKit/public/platform/scheduler/base/ > The former seems like it would be more consistent with existing code structure > and deps than latter. > However I'll start with latter and see what folks think. > > The latter sounds good to me. I think we're still figuring out how the scheduler code will eventually meld into Blink.
caseq@chromium.org changed reviewers: + caseq@chromium.org
https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:37: double MonotonicTimeInSeconds(base::TimeTicks timeTicks) { WTF::monotonicallyIncreasingTime()? https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:229: FOR_EACH_OBSERVER(WebThread::TaskTimeObserver, task_time_observers_, Have you considered moving this into ProcessTaskFromWorkQueue()? I don't think we need any signals in case it bails out without running the task, and it will also make this similar to other observers.
alph@chromium.org changed reviewers: + alph@chromium.org
https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:17: if (frame->isLocalRoot()) { style: use early return https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:20: frame->instrumentingAgents()->addInspectorWebPerfAgent( nit: make it a single line. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:398: task_time_observers_.AddObserver(task_time_observer); nit: You could also add it into task_observers_, so you don't need an extra code that calls 'will...' and 'did...' https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:246: bool has_task_time_observer; Is it used? https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/child/webthread_base.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/child/webthread_base.cc:67: AddTaskTimeObserverInternal(task_time_observer); nit: something wrong with indentation here. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:41: return base::TimeTicks::FromInternalValue( nit: Consider the following one, as it ensures better unit agreement: base::TimeTicks() + base::TimeDelta::FromSecondsD(monotonicTimeInSeconds)
The comments below were already with previous patch set 6. PTAL. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:37: double MonotonicTimeInSeconds(base::TimeTicks timeTicks) { On 2016/08/22 18:48:49, caseq wrote: > WTF::monotonicallyIncreasingTime()? I'm trying to convert the provided timeTicks, not just seeking a time value https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:223: FOR_EACH_OBSERVER(WebThread::TaskTimeObserver, task_time_observers_, On 2016/08/22 15:51:30, Sami wrote: > (Indentation looks a little off -- git cl format should fix this) Done. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:229: FOR_EACH_OBSERVER(WebThread::TaskTimeObserver, task_time_observers_, On 2016/08/22 18:48:49, caseq wrote: > Have you considered moving this into ProcessTaskFromWorkQueue()? I don't think > we need any signals in case it bails out without running the task, and it will > also make this similar to other observers. Addressed in new patch. Sami had the same comment -- and it turns out that the additional set of willProcess & didProcess wasn't strictly necessary. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:398: task_time_observers_.AddObserver(task_time_observer); On 2016/08/22 22:21:27, alph wrote: > nit: You could also add it into task_observers_, so you don't need an extra code > that calls 'will...' and 'did...' yep - done in new patch. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:90: void AddTaskTimeObserver(WebThread::TaskTimeObserver* task_time_observer); On 2016/08/22 17:02:58, Sami wrote: > On 2016/08/22 16:55:45, panicker wrote: > > On 2016/08/22 15:51:30, Sami wrote: > > > Earlier I really should've talked about base/ vs. not-base since as you > > rightly > > > pointed out, WebThread is in platform/ too. Anyway, what I was trying to say > > was > > > that scheduler/base is used to implement WebThread, so it's a little odd for > > it > > > to depend on it too. How about instead we do this: > > > > > > 1. Add an abstract TaskTimeObserver class here (a separate header maybe). > > > 2. Add a "using scheduler::TaskTimeObserver" in WebThread to bring it into > > scope > > > (or we could just use it directly there). > > > > > > WDYT? > > > > I agree it makes sense to make it a separate interface and decouple from > > WebThread, given what you said. > > However the question is whether it should live in: > > third_party/WebKit/public/platform/ OR > > third_party/WebKit/public/platform/scheduler/base/ > > The former seems like it would be more consistent with existing code structure > > and deps than latter. > > However I'll start with latter and see what folks think. > > > > > > The latter sounds good to me. I think we're still figuring out how the scheduler > code will eventually meld into Blink. Done. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:246: bool has_task_time_observer; On 2016/08/22 22:21:27, alph wrote: > Is it used? Removed in new patch https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/child/webthread_base.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/child/webthread_base.cc:67: AddTaskTimeObserverInternal(task_time_observer); On 2016/08/22 22:21:27, alph wrote: > nit: something wrong with indentation here. Acknowledged. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1417: void RendererSchedulerImpl::reportTaskTime(double start_time, On 2016/08/22 15:51:30, Sami wrote: > FYI, altimin@ is adding another task time consumer here: > https://codereview.chromium.org/2265873004/ Acknowledged.
Looks like alph@'s comments crossed with in-flight patchset #6. Please take another look.
https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:41: return base::TimeTicks::FromInternalValue( On 2016/08/22 22:21:27, alph wrote: > nit: Consider the following one, as it ensures better unit agreement: > base::TimeTicks() + base::TimeDelta::FromSecondsD(monotonicTimeInSeconds) What's the an equivalent counterpart for converting the other way around? I want to keep the conversions here and in MonotonicTimeInSeconds in third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc to be consistent with each other.
https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:41: return base::TimeTicks::FromInternalValue( On 2016/08/22 23:29:46, Shubhie wrote: > On 2016/08/22 22:21:27, alph wrote: > > nit: Consider the following one, as it ensures better unit agreement: > > base::TimeTicks() + base::TimeDelta::FromSecondsD(monotonicTimeInSeconds) > > What's the an equivalent counterpart for converting the other way around? > I want to keep the conversions here and in MonotonicTimeInSeconds in > third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc > to be consistent with each other. Added it as a comment. https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:26: InspectorWebPerfAgent(InspectedFrames*); nit: you used to add explicit here. curious why is it gone? https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:39: return (timeTicks.ToInternalValue() / nit: (timeTicks - base::TimeTicks()).InSecondsF() https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:241: task_start_time = task_end_time; Looks like the first time the observer will get a bogus times. The task_start_time is not initialized as there were no observers in the beginning.
I think the plumbing looks good now. https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:33: // WebThread::TaskTimeObserver implementation. s/Time// https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:90: void AddTaskTimeTracker(TaskTimeTracker* task_time_tracker); naming bikeshed: Should we rename this to TaskTimeObserver for consistency? https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:29: #include "public/platform/scheduler/base/task_time_tracker.h" nit: Could forward declare this instead. https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:68: virtual void addTaskObserver(TaskObserver*) { } Could you add a comment here saying that both the task observer and task time tracker APIs are very sensitive to performance and must not be used without a good reason? https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/child/webthread_base.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/child/webthread_base.h:12: #include "public/platform/scheduler/base/task_time_tracker.h" Ditto about forward declaring.
PTAL https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:26: InspectorWebPerfAgent(InspectedFrames*); On 2016/08/22 23:51:07, alph wrote: > nit: you used to add explicit here. curious why is it gone? Oops, fixed. https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:33: // WebThread::TaskTimeObserver implementation. On 2016/08/23 10:34:59, Sami wrote: > s/Time// Done. https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:241: task_start_time = task_end_time; On 2016/08/22 23:51:07, alph wrote: > Looks like the first time the observer will get a bogus times. The > task_start_time is not initialized as there were no observers in the beginning. There can be an issue if there are 0 observers at line 204 and an observer is added before line 235. In practice I don't think it's an issue as the renderer-scheduler is registered as observer early on in its constructor. UNLESS the WorkerScheduler is created beforehand and already processing tasks when the renderer-scheduler is created. To be safe I could initialize task_start_time to base::TimeTicks() and add an additional check here task_start_time != base::TimeTicks() (appears to be a common pattern for checking validity of TimeTicks) OTOH if this is not strictly necessary it could be needless overhead to do this check per task. So maybe a comment instead? Sami - any suggestions? https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:29: #include "public/platform/scheduler/base/task_time_tracker.h" On 2016/08/23 10:34:59, Sami wrote: > nit: Could forward declare this instead. Done. https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:68: virtual void addTaskObserver(TaskObserver*) { } On 2016/08/23 10:34:59, Sami wrote: > Could you add a comment here saying that both the task observer and task time > tracker APIs are very sensitive to performance and must not be used without a > good reason? How is this? https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/child/webthread_base.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/child/webthread_base.h:12: #include "public/platform/scheduler/base/task_time_tracker.h" On 2016/08/23 10:34:59, Sami wrote: > Ditto about forward declaring. Done.
On 2016/08/23 17:43:31, panicker wrote: > PTAL > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:26: > InspectorWebPerfAgent(InspectedFrames*); > On 2016/08/22 23:51:07, alph wrote: > > nit: you used to add explicit here. curious why is it gone? > > Oops, fixed. > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:33: // > WebThread::TaskTimeObserver implementation. > On 2016/08/23 10:34:59, Sami wrote: > > s/Time// > > Done. > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc > (right): > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:241: > task_start_time = task_end_time; > On 2016/08/22 23:51:07, alph wrote: > > Looks like the first time the observer will get a bogus times. The > > task_start_time is not initialized as there were no observers in the > beginning. > > There can be an issue if there are 0 observers at line 204 and an observer is > added before line 235. > In practice I don't think it's an issue as the renderer-scheduler is registered > as observer early on in its constructor. UNLESS the WorkerScheduler is created > beforehand and already processing tasks when the renderer-scheduler is created. > > To be safe I could initialize task_start_time to base::TimeTicks() and add an > additional check here task_start_time != base::TimeTicks() (appears to be a > common pattern for checking validity of TimeTicks) > > OTOH if this is not strictly necessary it could be needless overhead to do this > check per task. So maybe a comment instead? > Sami - any suggestions? > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebThread.h (right): > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebThread.h:29: #include > "public/platform/scheduler/base/task_time_tracker.h" > On 2016/08/23 10:34:59, Sami wrote: > > nit: Could forward declare this instead. > > Done. > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebThread.h:68: virtual void > addTaskObserver(TaskObserver*) { } > On 2016/08/23 10:34:59, Sami wrote: > > Could you add a comment here saying that both the task observer and task time > > tracker APIs are very sensitive to performance and must not be used without a > > good reason? > > How is this? > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/scheduler/child/webthread_base.h > (right): > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/pub... > third_party/WebKit/public/platform/scheduler/child/webthread_base.h:12: #include > "public/platform/scheduler/base/task_time_tracker.h" > On 2016/08/23 10:34:59, Sami wrote: > > Ditto about forward declaring. > > Done. (Whoops, still learning not to mix @google and @chromiuim accounts :))
Description was changed from ========== Add TaskTimeObserver interface in WebThread, to observe task time in addition totask begin & end states. This is used by InspectorWebPerfAgent for Long Task Observer. BUG= ========== to ========== Add TaskTimeObserver interface in WebThread, to observe task time in addition totask begin & end states. This is used by InspectorWebPerfAgent for Long Task Observer. BUG= ==========
panicker@chromium.org changed reviewers: - panicker@google.com
Description was changed from ========== Add TaskTimeObserver interface in WebThread, to observe task time in addition totask begin & end states. This is used by InspectorWebPerfAgent for Long Task Observer. BUG= ========== to ========== Generalize TaskTimeTracker interface and expose WebThread. This is now used by InspectorWebPerfAgent for Long Task Observer. BUG=635596 ==========
https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:90: void AddTaskTimeTracker(TaskTimeTracker* task_time_tracker); On 2016/08/23 10:34:59, Sami wrote: > naming bikeshed: Should we rename this to TaskTimeObserver for consistency? Sure, I'm fine with that. Although I'd prefer to make the naming change after addressing all other review comments.
PTAL. Addressed bike-shed naming change and remaining nit. https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2259013003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:41: return base::TimeTicks::FromInternalValue( On 2016/08/22 23:51:07, alph wrote: > On 2016/08/22 23:29:46, Shubhie wrote: > > On 2016/08/22 22:21:27, alph wrote: > > > nit: Consider the following one, as it ensures better unit agreement: > > > base::TimeTicks() + base::TimeDelta::FromSecondsD(monotonicTimeInSeconds) > > > > What's the an equivalent counterpart for converting the other way around? > > I want to keep the conversions here and in MonotonicTimeInSeconds in > > third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc > > to be consistent with each other. > > Added it as a comment. Done. https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:39: return (timeTicks.ToInternalValue() / On 2016/08/22 23:51:07, alph wrote: > nit: (timeTicks - base::TimeTicks()).InSecondsF() Done. https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:241: task_start_time = task_end_time; On 2016/08/23 17:43:31, panicker wrote: > On 2016/08/22 23:51:07, alph wrote: > > Looks like the first time the observer will get a bogus times. The > > task_start_time is not initialized as there were no observers in the > beginning. > > There can be an issue if there are 0 observers at line 204 and an observer is > added before line 235. > In practice I don't think it's an issue as the renderer-scheduler is registered > as observer early on in its constructor. UNLESS the WorkerScheduler is created > beforehand and already processing tasks when the renderer-scheduler is created. > > To be safe I could initialize task_start_time to base::TimeTicks() and add an > additional check here task_start_time != base::TimeTicks() (appears to be a > common pattern for checking validity of TimeTicks) > > OTOH if this is not strictly necessary it could be needless overhead to do this > check per task. So maybe a comment instead? > Sami - any suggestions? I'm leaving this as is, as this is not worse than today i.e. in theory task_time_tracker_ could change from null to non-null between the 2 checks.
https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:241: task_start_time = task_end_time; On 2016/08/25 23:11:52, Shubhie wrote: > On 2016/08/23 17:43:31, panicker wrote: > > On 2016/08/22 23:51:07, alph wrote: > > > Looks like the first time the observer will get a bogus times. The > > > task_start_time is not initialized as there were no observers in the > > beginning. > > > > There can be an issue if there are 0 observers at line 204 and an observer is > > added before line 235. > > In practice I don't think it's an issue as the renderer-scheduler is > registered > > as observer early on in its constructor. UNLESS the WorkerScheduler is created > > beforehand and already processing tasks when the renderer-scheduler is > created. > > > > To be safe I could initialize task_start_time to base::TimeTicks() and add an > > additional check here task_start_time != base::TimeTicks() (appears to be a > > common pattern for checking validity of TimeTicks) > > > > OTOH if this is not strictly necessary it could be needless overhead to do > this > > check per task. So maybe a comment instead? > > Sami - any suggestions? > > I'm leaving this as is, as this is not worse than today i.e. in theory > task_time_tracker_ could change from null to non-null between the 2 checks. Sorry, I missed this question earlier. I think it would make sense to replace the second might_have_observers() check with task_start_time == base::TimeTicks() check. It's not an error to call FOR_EACH_OBSERVER when there are no observers. This would mean the clients don't need to guard against bogus values.
On 2016/08/26 11:03:57, Sami wrote: > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc > (right): > > https://codereview.chromium.org/2259013003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:241: > task_start_time = task_end_time; > On 2016/08/25 23:11:52, Shubhie wrote: > > On 2016/08/23 17:43:31, panicker wrote: > > > On 2016/08/22 23:51:07, alph wrote: > > > > Looks like the first time the observer will get a bogus times. The > > > > task_start_time is not initialized as there were no observers in the > > > beginning. > > > > > > There can be an issue if there are 0 observers at line 204 and an observer > is > > > added before line 235. > > > In practice I don't think it's an issue as the renderer-scheduler is > > registered > > > as observer early on in its constructor. UNLESS the WorkerScheduler is > created > > > beforehand and already processing tasks when the renderer-scheduler is > > created. > > > > > > To be safe I could initialize task_start_time to base::TimeTicks() and add > an > > > additional check here task_start_time != base::TimeTicks() (appears to be a > > > common pattern for checking validity of TimeTicks) > > > > > > OTOH if this is not strictly necessary it could be needless overhead to do > > this > > > check per task. So maybe a comment instead? > > > Sami - any suggestions? > > > > I'm leaving this as is, as this is not worse than today i.e. in theory > > task_time_tracker_ could change from null to non-null between the 2 checks. > > Sorry, I missed this question earlier. I think it would make sense to replace > the second might_have_observers() check with task_start_time == > base::TimeTicks() check. It's not an error to call FOR_EACH_OBSERVER when there > are no observers. This would mean the clients don't need to guard against bogus > values. Done. PTAL.
panicker@chromium.org changed reviewers: + dglazkov@chromium.org
+dglazkov for OWNERS review for WebThread.
Thank you, lgtm with one nit. https://codereview.chromium.org/2259013003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/2259013003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:61: void AddTaskTimeObserver(TaskTimeObserver* task_time_observer) { nit: Please implement these in the .cc instead.
On 2016/08/26 16:08:56, Sami wrote: > Thank you, lgtm with one nit. > > https://codereview.chromium.org/2259013003/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h > (right): > > https://codereview.chromium.org/2259013003/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:61: void > AddTaskTimeObserver(TaskTimeObserver* task_time_observer) { > nit: Please implement these in the .cc instead. Thanks for the review!
https://codereview.chromium.org/2259013003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/2259013003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:61: void AddTaskTimeObserver(TaskTimeObserver* task_time_observer) { On 2016/08/26 16:08:56, Sami wrote: > nit: Please implement these in the .cc instead. Done.
lgtm https://codereview.chromium.org/2259013003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:201: base::TimeTicks task_start_time = base::TimeTicks(); nit: revert this.
https://codereview.chromium.org/2259013003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:201: base::TimeTicks task_start_time = base::TimeTicks(); On 2016/08/26 18:24:17, alph wrote: > nit: revert this. Can you clarify why this should be removed? I think it makes sense to initialize task_start_time to base::TimeTicks()
https://codereview.chromium.org/2259013003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:201: base::TimeTicks task_start_time = base::TimeTicks(); On 2016/08/26 18:27:57, Shubhie wrote: > On 2016/08/26 18:24:17, alph wrote: > > nit: revert this. > > Can you clarify why this should be removed? > I think it makes sense to initialize task_start_time to base::TimeTicks() The original line does also make it call a default constructor on task_start_time. You don't need to assign it explicitly.
https://codereview.chromium.org/2259013003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2259013003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:201: base::TimeTicks task_start_time = base::TimeTicks(); On 2016/08/26 18:35:11, alph wrote: > On 2016/08/26 18:27:57, Shubhie wrote: > > On 2016/08/26 18:24:17, alph wrote: > > > nit: revert this. > > > > Can you clarify why this should be removed? > > I think it makes sense to initialize task_start_time to base::TimeTicks() > > The original line does also make it call a default constructor on > task_start_time. You don't need to assign it explicitly. Done.
The CQ bit was checked by panicker@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by panicker@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...
lgtm https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:22: , public WebThread::TaskObserver style: nuke extra space before public https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:23: , public scheduler::TaskTimeObserver { ditto https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:19: virtual void ReportTaskTime(double startTime, double endTime) = 0; Now that this is in blink land, should we start following blink's code style? Sami, what's the plan for the scheduler?
https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:22: , public WebThread::TaskObserver On 2016/08/26 22:42:25, caseq wrote: > style: nuke extra space before public Done. https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:23: , public scheduler::TaskTimeObserver { On 2016/08/26 22:42:25, caseq wrote: > ditto Done. https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2259013003/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:19: virtual void ReportTaskTime(double startTime, double endTime) = 0; On 2016/08/26 22:42:25, caseq wrote: > Now that this is in blink land, should we start following blink's code style? > Sami, what's the plan for the scheduler? Yeah that would be nice :) Keeps confusing the formatter.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
panicker@chromium.org changed reviewers: + esprehn@chromium.org
Dimitri or Elliott, could one of you review for WebThread OWNERS? Thank you!
The CQ bit was checked by panicker@chromium.org to run a CQ dry run
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:103: manager_->AddTaskTimeObserver(&test_task_time_observer_); Is it essential that you now could have 2 observers? https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:77: // TaskTimeObserver is an object that receives notifications for Should we instead set the observer to prevent the abuse? https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:19: virtual void ReportTaskTime(double startTime, double endTime) = 0; reportTaskTime (from lower case). Also, do you need start and end or the task length?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Pavel for the review! https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:103: manager_->AddTaskTimeObserver(&test_task_time_observer_); On 2016/08/29 17:23:08, pfeldman wrote: > Is it essential that you now could have 2 observers? Removed. Although this was not strictly necessary before my change either. https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:77: // TaskTimeObserver is an object that receives notifications for On 2016/08/29 17:23:08, pfeldman wrote: > Should we instead set the observer to prevent the abuse? Sorry, could you elaborate on what you mean by "set the observer" ? (To provide context: this API is to enable usage from Inspector Agent etc.) https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:19: virtual void ReportTaskTime(double startTime, double endTime) = 0; On 2016/08/29 17:23:08, pfeldman wrote: > reportTaskTime (from lower case). Also, do you need start and end or the task > length? This is still following pre-Blink Scheduler code style to be consistent with code in third_party/WebKit/public/platform/scheduler/ caseq had a comment about switching Scheduler over to Blink style - but not sure of timeline there.
https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:77: // TaskTimeObserver is an object that receives notifications for You are saying here that I need to have a compelling reason to add another observer. But since we only have single client at this point, should we not allow adding observers at all and introduce SetTaskTimeObserver for now? That would eliminate potential abuse. https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:19: virtual void ReportTaskTime(double startTime, double endTime) = 0; > This is still following pre-Blink Scheduler code style to be consistent with > code in third_party/WebKit/public/platform/scheduler/ > caseq had a comment about switching Scheduler over to Blink style - but not sure > of timeline there. Okay. What about the startTime/endTime vs length? Do you envision reporting the monotonic times to the users on the perf timeline?
https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:77: // TaskTimeObserver is an object that receives notifications for On 2016/08/29 17:52:54, pfeldman wrote: > You are saying here that I need to have a compelling reason to add another > observer. But since we only have single client at this point, should we not > allow adding observers at all and introduce SetTaskTimeObserver for now? That > would eliminate potential abuse. Reflector will also need this imminently as MessageLoop tasks is in their first milestone, so it's not just single client. https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:19: virtual void ReportTaskTime(double startTime, double endTime) = 0; On 2016/08/29 17:52:54, pfeldman wrote: > > This is still following pre-Blink Scheduler code style to be consistent with > > code in third_party/WebKit/public/platform/scheduler/ > > caseq had a comment about switching Scheduler over to Blink style - but not > sure > > of timeline there. > > Okay. What about the startTime/endTime vs length? Do you envision reporting the > monotonic times to the users on the perf timeline? Oops yeah - I do need startTime / EndTime as it's needed by the PerformanceEntry ctor: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe... https://www.w3.org/TR/performance-timeline-2/#the-performanceentry-interface
https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebThread.h (right): https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebThread.h:77: // TaskTimeObserver is an object that receives notifications for Ok, I must say I am not a fan of it (I'd prefer single entity to collect the data and others to sample it), but let us land this to unblock you and see how reflector jumps on board and reuses it. We will then revisit it if reflector and webperf gain a lot of similarity. lgtm.
On 2016/08/29 18:04:27, pfeldman wrote: > https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebThread.h (right): > > https://codereview.chromium.org/2259013003/diff/220001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebThread.h:77: // TaskTimeObserver is an > object that receives notifications for > Ok, I must say I am not a fan of it (I'd prefer single entity to collect the > data and others to sample it), but let us land this to unblock you and see how > reflector jumps on board and reuses it. We will then revisit it if reflector and > webperf gain a lot of similarity. > > lgtm. Discussed offline. The upshot is that we may want to consolidate consumers like Web Perf & Reflector down the road.
panicker@chromium.org changed reviewers: - dglazkov@chromium.org, esprehn@chromium.org
Thanks all for the review. submitting soon.
The CQ bit was checked by panicker@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...
https://codereview.chromium.org/2259013003/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2259013003/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:16: TaskTimeObserver() {} nit: WK style on new files?
On 2016/08/29 18:50:18, alph wrote: > https://codereview.chromium.org/2259013003/diff/240001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h > (right): > > https://codereview.chromium.org/2259013003/diff/240001/third_party/WebKit/pub... > third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:16: > TaskTimeObserver() {} > nit: WK style on new files? Pavel & Andrei mentioned this also. I don't think it makes sense to mix and match styles in the scheduler directory. Sami what's the plan for switching scheduler to Blink style?
The CQ bit was unchecked by panicker@chromium.org
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, alph@chromium.org, caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2259013003/#ps240001 (title: "address review comments")
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 ========== Generalize TaskTimeTracker interface and expose WebThread. This is now used by InspectorWebPerfAgent for Long Task Observer. BUG=635596 ========== to ========== Generalize TaskTimeTracker interface and expose WebThread. This is now used by InspectorWebPerfAgent for Long Task Observer. BUG=635596 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Generalize TaskTimeTracker interface and expose WebThread. This is now used by InspectorWebPerfAgent for Long Task Observer. BUG=635596 ========== to ========== Generalize TaskTimeTracker interface and expose WebThread. This is now used by InspectorWebPerfAgent for Long Task Observer. BUG=635596 Committed: https://crrev.com/d9caeb1b95bbdfab5aa7688e6ce143f018f58178 Cr-Commit-Position: refs/heads/master@{#415051} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/d9caeb1b95bbdfab5aa7688e6ce143f018f58178 Cr-Commit-Position: refs/heads/master@{#415051}
Message was sent while issue was closed.
On 2016/08/29 19:24:45, Shubhie wrote: > I don't think it makes sense to mix and match styles in the scheduler directory. > Sami what's the plan for switching scheduler to Blink style? I understood the plan is to go in the opposite direction and switch Blink to Chromium style, but have no idea what the timeline is. I wouldn't want to re-style the scheduler code just to switch it back later.
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2300513002/ by panicker@google.com. The reason for reverting is: oops, unintentionally added to wrong CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
