WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
Bug 89692
Web Inspector: show worker started and finished on timeline
https://bugs.webkit.org/show_bug.cgi?id=89692
Summary
Web Inspector: show worker started and finished on timeline
Hanna
Reported
2012-06-21 14:54:35 PDT
Maybe add an indicator somewhere on timeline, like add more details to the popup generated by mouse hovering? Recommendations appreciated, thank you.
Attachments
Patch
(18.21 KB, patch)
2012-07-10 11:45 PDT
,
Hanna
no flags
Details
Formatted Diff
Diff
Patch
(17.73 KB, patch)
2012-07-11 08:01 PDT
,
Hanna
no flags
Details
Formatted Diff
Diff
Patch
(21.55 KB, patch)
2012-07-27 09:46 PDT
,
Hanna
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-06
(428.31 KB, application/zip)
2012-07-27 10:23 PDT
,
WebKit Review Bot
no flags
Details
Patch
(21.66 KB, patch)
2012-07-27 10:49 PDT
,
Hanna
no flags
Details
Formatted Diff
Diff
Patch
(21.86 KB, patch)
2012-07-27 12:05 PDT
,
Hanna
no flags
Details
Formatted Diff
Diff
Patch
(21.86 KB, patch)
2012-07-27 12:11 PDT
,
Hanna
no flags
Details
Formatted Diff
Diff
Patch
(21.73 KB, patch)
2012-07-27 14:30 PDT
,
Hanna
no flags
Details
Formatted Diff
Diff
Patch
(21.99 KB, patch)
2012-07-30 09:13 PDT
,
Hanna
gustavo
: commit-queue-
Details
Formatted Diff
Diff
Patch
(21.99 KB, patch)
2012-07-30 10:05 PDT
,
Hanna
pfeldman
: review-
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Hanna
Comment 1
2012-07-04 07:20:38 PDT
add an event on timeline indicating when a worker started and when it's destroyed instead
Hanna
Comment 2
2012-07-10 11:45:23 PDT
Created
attachment 151496
[details]
Patch
Hanna
Comment 3
2012-07-10 11:46:05 PDT
anyone r?
Build Bot
Comment 4
2012-07-10 12:03:17 PDT
Comment on
attachment 151496
[details]
Patch
Attachment 151496
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/13204055
Pavel Feldman
Comment 5
2012-07-10 12:15:55 PDT
Comment on
attachment 151496
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=151496&action=review
> Source/WebCore/inspector/InspectorInstrumentation.cpp:982 > + timelineAgent->didCreateWorker(url, isSharedWorker);
You should pass the worker id as well so that we could navigate between worker records.
> Source/WebCore/inspector/InspectorInstrumentation.cpp:1009 > + if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent())
Why aren't you using didDestroyWorker? It belongs to the legacy workers inspection (fake workers), but should still work.
> Source/WebCore/inspector/InspectorInstrumentation.h:1267 > + didCreateWorkerImpl(instrumentingAgents, id, context->url().string(), isSharedWorker);
Why did this change? We don't make any calls from these inline public methods, all processing should be in impl.
> Source/WebCore/inspector/TimelineRecordFactory.cpp:201 > + data->setBoolean("isSharedWorker", isSharedWorker);
"shared"
> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:523 > + if (this.type !== recordTypes.WorkerCreated && this.type !== recordTypes.WorkerTerminated) {
We already have atomic events with 0 duration that we report. Fixing them altogether should be handled in a separate patch.
> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:584 > + case recordTypes.WorkerCreated:
Missing break; above.
> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:586 > + case recordTypes.WorkerTerminated:
ditto. Did you run this change?
> LayoutTests/inspector/timeline/timeline-web-workers.html:25 > + var sharedWorker = createSharedWorker();
Shared workers instantiation is browser-specific. Please make sure tests pass at least on Mac ports and for Chromium.
> LayoutTests/inspector/timeline/timeline-web-workers.html:29 > + dedicatedWorker.postMessage("close");
Worker message processing is asynchronous to the inspector message interchange. By the time you leave this method and stop timeline, the worker is not necessarily stopped.
Hanna
Comment 6
2012-07-10 13:06:24 PDT
Comment on
attachment 151496
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=151496&action=review
fixing now
>> Source/WebCore/inspector/InspectorInstrumentation.cpp:982 >> + timelineAgent->didCreateWorker(url, isSharedWorker); > > You should pass the worker id as well so that we could navigate between worker records.
sure, except the id is the memory address
>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1009 >> + if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent()) > > Why aren't you using didDestroyWorker? It belongs to the legacy workers inspection (fake workers), but should still work.
didDestroyWorker is not called on our platform
>> Source/WebCore/inspector/InspectorInstrumentation.h:1267 >> + didCreateWorkerImpl(instrumentingAgents, id, context->url().string(), isSharedWorker); > > Why did this change? We don't make any calls from these inline public methods, all processing should be in impl.
turned out that the url passed in is different from context->url().string(), in order to match up with the worker terminated I changed it, but if this messes up other things (ie inspectorAgent->didCreateWorker(id, url, isSharedWorker);) I'll add a new method then pass in context->url().string()
>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:523 >> + if (this.type !== recordTypes.WorkerCreated && this.type !== recordTypes.WorkerTerminated) { > > We already have atomic events with 0 duration that we report. Fixing them altogether should be handled in a separate patch.
got it
>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:586 >> + case recordTypes.WorkerTerminated: > > ditto. Did you run this change?
it is supposed to fall through, since workercreated has both isSharedWorker and url, but I couldn't pass the isSharedWorker info to workerterminated easily
>> LayoutTests/inspector/timeline/timeline-web-workers.html:25 >> + var sharedWorker = createSharedWorker(); > > Shared workers instantiation is browser-specific. Please make sure tests pass at least on Mac ports and for Chromium.
this method that I used is copied directly from LayoutTests/fast/workers for both sharedWorker and dedicatedWorker, should that be fine?
>> LayoutTests/inspector/timeline/timeline-web-workers.html:29 >> + dedicatedWorker.postMessage("close"); > > Worker message processing is asynchronous to the inspector message interchange. By the time you leave this method and stop timeline, the worker is not necessarily stopped.
will change right away
Hanna
Comment 7
2012-07-10 13:48:08 PDT
Comment on
attachment 151496
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=151496&action=review
>>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1009 >>> + if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent()) >> >> Why aren't you using didDestroyWorker? It belongs to the legacy workers inspection (fake workers), but should still work. > > didDestroyWorker is not called on our platform
my bad, it might be called on the worker thread, but I tried didDestroyWorker it doesn't show up on the timeline of the mainthread (which is the purpose of this PR)
Hanna
Comment 8
2012-07-11 08:01:49 PDT
Created
attachment 151710
[details]
Patch
Hanna
Comment 9
2012-07-11 09:10:42 PDT
Note: 1. the worker ID is the address 2. on termination, worker ID and isSharedWorker is hard to know (since didDestroyWorker doesn't work in this case), it might be possible to include those things in ScriptExecutionContext, however I do not know if this is desired 3. for testing, I added setTimeout to StopTimeline, I hope the workers will be done in 5 seconds
Hanna
Comment 10
2012-07-12 08:25:57 PDT
r?
Pavel Feldman
Comment 11
2012-07-12 08:43:09 PDT
Comment on
attachment 151496
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=151496&action=review
>>>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1009 >>>> + if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent()) >>> >>> Why aren't you using didDestroyWorker? It belongs to the legacy workers inspection (fake workers), but should still work. >> >> didDestroyWorker is not called on our platform > > my bad, it might be called on the worker thread, but I tried didDestroyWorker it doesn't show up on the timeline of the mainthread (which is the purpose of this PR)
Hm. That would be a serios bug. We should never get here on the worker thread. Could you look into what is wrong here? This change is about reliably instrumenting creation and destruction of the worker on the main thread and you should not use workarounds if underlying code is behaving unexpectedly.
>>> Source/WebCore/inspector/InspectorInstrumentation.h:1267 >>> + didCreateWorkerImpl(instrumentingAgents, id, context->url().string(), isSharedWorker); >> >> Why did this change? We don't make any calls from these inline public methods, all processing should be in impl. > > turned out that the url passed in is different from context->url().string(), in order to match up with the worker terminated I changed it, but if this messes up other things (ie inspectorAgent->didCreateWorker(id, url, isSharedWorker);) I'll add a new method then pass in context->url().string()
What is the difference between the url and context's url?
Pavel Feldman
Comment 12
2012-07-12 08:48:17 PDT
Comment on
attachment 151710
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=151710&action=review
> Source/WebCore/inspector/InspectorInstrumentation.cpp:982 > + timelineAgent->didCreateWorker(id, urlFromContext, isSharedWorker);
Passing two urls is confusing, we need to figure out why they differ.
> Source/WebCore/inspector/InspectorInstrumentation.cpp:1009 > + if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent())
It would be nice if didDestroyWorker was called when it should be.
> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:586 > + contentHelper._appendTextRow(WebInspector.UIString("URL"), this.data["url"]);
I think Worker ID applies to the terminated info. Loading multiple workers from the same URL could be used for parallel data processing, so it happens more often than you might think.
> LayoutTests/inspector/timeline/timeline-web-workers.html:31 > + InspectorTest.startTimeline(function() {
We always use named functions with { on the next line.
> LayoutTests/inspector/timeline/timeline-web-workers.html:37 > + setTimeout(function(){InspectorTest.stopTimeline(step3);}, 5000);
Tests should not have timeouts. You should signal worker termination from the page to the front-end via logging something to the console and sniffing for the console message.
Hanna
Comment 13
2012-07-12 10:01:20 PDT
Comment on
attachment 151496
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=151496&action=review
>>>>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1009 >>>>> + if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent()) >>>> >>>> Why aren't you using didDestroyWorker? It belongs to the legacy workers inspection (fake workers), but should still work. >>> >>> didDestroyWorker is not called on our platform >> >> my bad, it might be called on the worker thread, but I tried didDestroyWorker it doesn't show up on the timeline of the mainthread (which is the purpose of this PR) > > Hm. That would be a serios bug. We should never get here on the worker thread. Could you look into what is wrong here? This change is about reliably instrumenting creation and destruction of the worker on the main thread and you should not use workarounds if underlying code is behaving unexpectedly.
Sorry if it sounds confusing, I was making speculations because as of now the inspector I have only shows the timeline events on the mainthread (I was trying to say I do not know if diddestroyworker would show up on the inspector window for the worker which I do not have) and unfortunately diddestroyworker is never called either way, so I used didterminatecontext to approximate when the worker is destroyed since didterminatecontext is called after the workerthread (and everything relating to it) is de-allocated which should also be valid
>>>> Source/WebCore/inspector/InspectorInstrumentation.h:1267 >>>> + didCreateWorkerImpl(instrumentingAgents, id, context->url().string(), isSharedWorker); >>> >>> Why did this change? We don't make any calls from these inline public methods, all processing should be in impl. >> >> turned out that the url passed in is different from context->url().string(), in order to match up with the worker terminated I changed it, but if this messes up other things (ie inspectorAgent->didCreateWorker(id, url, isSharedWorker);) I'll add a new method then pass in context->url().string() > > What is the difference between the url and context's url?
the url being passed in is more specific, the context's url is cut half, ie less specific, but the first part of the urls are identical
Pavel Feldman
Comment 14
2012-07-13 00:57:51 PDT
> > Hm. That would be a serios bug. We should never get here on the worker thread. Could you look into what is wrong here? This change is about reliably instrumenting creation and destruction of the worker on the main thread and you should not use workarounds if underlying code is behaving unexpectedly. > > Sorry if it sounds confusing, I was making speculations because as of now the inspector I have only shows the timeline events on the mainthread (I was trying to say I do not know if diddestroyworker would show up on the inspector window for the worker which I do not have) and unfortunately diddestroyworker is never called either way, so I used didterminatecontext to approximate when the worker is destroyed since didterminatecontext is called after the workerthread (and everything relating to it) is de-allocated which should also be valid
I do realize that it will work this way, I just want to make sure that we fix things along the way and if the one that should work is not working, we fix it instead of looking for the workaround.
> the url being passed in is more specific, the context's url is cut half, ie less specific, but the first part of the urls are identical
Why would one want the truncated url? Can we use the full url for everything?
Hanna
Comment 15
2012-07-18 12:53:46 PDT
(In reply to
comment #14
)
> > I do realize that it will work this way, I just want to make sure that we fix things along the way and if the one that should work is not working, we fix it instead of looking for the workaround. >
So I looked into it a little. The current inspectorinstrumentation::diddestroyworker is called in ~abstrackworker, which sharedworker inherits, though it wasn't even called on sharedworker's destruction What I am thinking is to move the instrumentation of diddestroyworker to injectedscripthost, which is called via injectedfakeworker.js,and the instrumentation should be called on fakeworker destruction. Please let me know if this is be valid as I am not familiar with fakeworker, thank you
Pavel Feldman
Comment 16
2012-07-20 08:58:00 PDT
I'd like to remove fake workers inspection to avoid the confusion (
https://bugs.webkit.org/show_bug.cgi?id=91867
). Once that instrumentation is removed, you should be able to add missing instrumentation to show workers in the timeline.
Hanna
Comment 17
2012-07-20 09:17:26 PDT
(In reply to
comment #16
)
> I'd like to remove fake workers inspection to avoid the confusion (
https://bugs.webkit.org/show_bug.cgi?id=91867
). Once that instrumentation is removed, you should be able to add missing instrumentation to show workers in the timeline.
I think it might also be a javascript parsing issue that diddestroyworker is not called (in workersidebarpane, on
http://pmav.eu/stuff/javascript-webworkers/
, if you run the workers multiple times, they get piled up and is never terminated in the workersidebarpane, nevertheless this does not happen in the latest build of chromium, but happens in the official release of chromium on ubuntu and in everything else, ie safari, bb, qt), so I wonder if this has anything to do with how javascript core and v8 handles worker termination, nevertheless I am still investigating and will use diddestroyworker to make the instrumentation to timeline once I get it working
Hanna
Comment 18
2012-07-27 09:46:13 PDT
Created
attachment 154976
[details]
Patch
Hanna
Comment 19
2012-07-27 09:47:46 PDT
note, the layout test for workerFinished only has id but not url since url for workerFinished is added in the frontend when the id of workerFinished matches with its parent record (workerStarted)
WebKit Review Bot
Comment 20
2012-07-27 10:23:16 PDT
Comment on
attachment 154976
[details]
Patch
Attachment 154976
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/13380326
New failing tests: inspector/timeline/timeline-web-workers.html
WebKit Review Bot
Comment 21
2012-07-27 10:23:21 PDT
Created
attachment 154982
[details]
Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Hanna
Comment 22
2012-07-27 10:49:18 PDT
Created
attachment 154987
[details]
Patch
Konrad Piascik
Comment 23
2012-07-27 11:12:53 PDT
Comment on
attachment 154987
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=154987&action=review
You have workerId, url and workerId, url used repeatedly in different places. Choose an order for the two parameters for consistency. I prefer workerId, url but I leave it to you
> Source/WebCore/inspector/InspectorInstrumentation.cpp:972 > + int id = 0;
can a worker ever have id 0? it's better to set it to -1
> Source/WebCore/inspector/InspectorInstrumentation.cpp:977 > + if (id && timelineAgent)
check for id > 0 if you set id = -1 as initial value.
> Source/WebCore/inspector/InspectorInstrumentation.cpp:996 > + int id = 0;
ditto
> Source/WebCore/inspector/InspectorInstrumentation.cpp:1001 > + if (id && timelineAgent)
ditto
> Source/WebCore/inspector/InspectorWorkerAgent.cpp:86 > + static int s_nextId;
I don't like the variable being public. I'd rather see a static getter and static setter.
> Source/WebCore/inspector/InspectorWorkerAgent.cpp:-206 > - return;
why did you remove the early return? You should "return id;" here as well.
Hanna
Comment 24
2012-07-27 12:05:30 PDT
Created
attachment 155008
[details]
Patch
Pavel Feldman
Comment 25
2012-07-27 12:07:36 PDT
Comment on
attachment 154987
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=154987&action=review
> Source/WebCore/ChangeLog:8 > + Test: inspector/timeline/timeline-web-workers.html
Please explain what happens and why. I'd also be interested in learning more about the use cases. Is this a user request? There is a lot happening in the browser, not everything deserves to be on the timeline. Sorry for bringing it up so late, but I am sure you have some good answers to this. We do need to justify the code we are adding.
> Source/WebCore/inspector/InspectorTimelineAgent.cpp:105 > +static const char WorkerFinished[] = "WorkerFinished";
"WorkerTerminated" ?
> Source/WebCore/inspector/InspectorTimelineAgent.cpp:449 > +void InspectorTimelineAgent::didFinishWorker(int workerId)
::didTerminateWorker
> Source/WebCore/inspector/InspectorTimelineAgent.cpp:451 > + appendRecord(TimelineRecordFactory::createAnimationFrameData(workerId), TimelineRecordType::WorkerFinished, false, 0);
Why createAnimationFrameData ?
> Source/WebCore/inspector/InspectorWorkerAgent.cpp:59 > + , m_id(id)
Please roll this back.
>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:86 >> + static int s_nextId; > > I don't like the variable being public. I'd rather see a static getter and static setter.
Please don't make random fields public.
> Source/WebCore/inspector/InspectorWorkerAgent.cpp:196 > + createWorkerFrontendChannel(workerContextProxy, url.string(), WorkerFrontendChannel::s_nextId);
Wouldn't it be more elegant for createWorkerFrontendChannel to return channel's id?
> Source/WebCore/inspector/InspectorWorkerAgent.cpp:202 > + int id = 0;
Remove this code.
>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:-206 >> - return; > > why did you remove the early return? You should "return id;" here as well.
should be return it->second->id()
> Source/WebCore/inspector/InspectorWorkerAgent.cpp:214 > + return id;
return 0; (or -1 depending on the value you use to identify non-existing worker)
> Source/WebCore/inspector/InspectorWorkerAgent.h:82 > + typedef HashMap<WorkerContextProxy*, std::pair<int, String> > DedicatedWorkers;
You don't need to change this map, do you?
> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:102 > + recordStyles[recordTypes.WorkerFinished] = { title: WebInspector.UIString("Worker Finished"), category: categories["scripting"] };
Worker Terminated
Pavel Feldman
Comment 26
2012-07-27 12:08:13 PDT
Comment on
attachment 155008
[details]
Patch r- as per my earlier review.
Hanna
Comment 27
2012-07-27 12:11:44 PDT
Created
attachment 155012
[details]
Patch
Hanna
Comment 28
2012-07-27 12:21:29 PDT
Comment on
attachment 154987
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=154987&action=review
>> Source/WebCore/ChangeLog:8 >> + Test: inspector/timeline/timeline-web-workers.html > > Please explain what happens and why. > > I'd also be interested in learning more about the use cases. Is this a user request? There is a lot happening in the browser, not everything deserves to be on the timeline. Sorry for bringing it up so late, but I am sure you have some good answers to this. We do need to justify the code we are adding.
It is a user request
>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:59 >> + , m_id(id) > > Please roll this back.
there will be instances where the browser don't have workerfrontend
>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:196 >> + createWorkerFrontendChannel(workerContextProxy, url.string(), WorkerFrontendChannel::s_nextId); > > Wouldn't it be more elegant for createWorkerFrontendChannel to return channel's id?
the problem is not every browser has a worker frontend (as a matter of fact only chromium has it), idealy this thing has to work with all the browser
>> Source/WebCore/inspector/InspectorWorkerAgent.h:82 >> + typedef HashMap<WorkerContextProxy*, std::pair<int, String> > DedicatedWorkers; > > You don't need to change this map, do you?
I have to store id somewhere
Konrad Piascik
Comment 29
2012-07-27 12:40:22 PDT
Comment on
attachment 154987
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=154987&action=review
>>> Source/WebCore/ChangeLog:8 >>> + Test: inspector/timeline/timeline-web-workers.html >> >> Please explain what happens and why. >> >> I'd also be interested in learning more about the use cases. Is this a user request? There is a lot happening in the browser, not everything deserves to be on the timeline. Sorry for bringing it up so late, but I am sure you have some good answers to this. We do need to justify the code we are adding. > > It is a user request
Even though the worker's javascript is processed on a separate thread it is still useful to know when a worker is created/terminated. This is especially important on devices with limited system resources. This is meant to be a surface level overview and for further details about how things run Web Workers can be inspected in their own timeline.
Pavel Feldman
Comment 30
2012-07-27 12:42:30 PDT
Comment on
attachment 154987
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=154987&action=review
>>> Source/WebCore/ChangeLog:8 >>> + Test: inspector/timeline/timeline-web-workers.html >> >> Please explain what happens and why. >> >> I'd also be interested in learning more about the use cases. Is this a user request? There is a lot happening in the browser, not everything deserves to be on the timeline. Sorry for bringing it up so late, but I am sure you have some good answers to this. We do need to justify the code we are adding. > > It is a user request
I'd like to challenge it. Is is mentioned in any public forum or a bug tracker?
>>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:59 >>> + , m_id(id) >> >> Please roll this back. > > there will be instances where the browser don't have workerfrontend
I see what you mean now.
>>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:86 >>> + static int s_nextId; >> >> I don't like the variable being public. I'd rather see a static getter and static setter. > > Please don't make random fields public.
I would move it into the InspectorWorkerAgent then.
> Source/WebCore/inspector/InspectorWorkerAgent.cpp:194 > + m_dedicatedWorkers.set(workerContextProxy, std::make_pair(++WorkerFrontendChannel::s_nextId, url.string()));
It is cleaner to generate an id first and then to use it in two methods below.
Pavel Feldman
Comment 31
2012-07-27 12:43:41 PDT
Comment on
attachment 155012
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=155012&action=review
r- per review comments from previous patches.
> Source/WebCore/inspector/InspectorWorkerAgent.cpp:86 > + static int nextId()
No, i don't think we need getters, as I mention, I'd just move it into the InspectorWorkerAgent.
Pavel Feldman
Comment 32
2012-07-27 13:19:00 PDT
> Even though the worker's javascript is processed on a separate thread it is still useful to know when a worker is created/terminated. This is especially important on devices with limited system resources. This is meant to be a surface level overview and for further details about how things run Web Workers can be inspected in their own timeline.
There are multiple timeline operation modes we are providing: 1. Performance angle. Workers should not be a big deal here. They operate elsewhere, while worker script loading is reflected in the page's timeline 2. Async causation analysis - we glue setTimeout with timer fire events in that mode. Also does not make much sense to the workers 3. Memory view (no correlation) 4. Frame mode (no correlation) 5. Getting to know what the program is doing when one has no clue Workers fall to under (5), but many, many other things do too. I don't think we should add workers before we add most of the events there. That's why I am a bit reluctant to this change.
Hanna
Comment 33
2012-07-27 14:30:34 PDT
Created
attachment 155049
[details]
Patch
Rob Buis
Comment 34
2012-07-30 07:10:54 PDT
Comment on
attachment 155049
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=155049&action=review
Looks good in general.
> Source/WebCore/inspector/InspectorInstrumentation.cpp:978 > + timelineAgent->didStartWorker(id, url);
How about more early return style here? InspectorWorkerAgent* workerAgent = instrumentingAgents->inspectorWorkerAgent(); if (!workerAgent) return; int id = workerAgent->didStartWorkerContext(workerContextProxy, url); if (!id) return; if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent()) timelineAgent->didStartWorker(id, url);
> Source/WebCore/inspector/InspectorInstrumentation.cpp:1002 > + timelineAgent->didTerminateWorker(id);
Ditto.
Hanna
Comment 35
2012-07-30 09:13:29 PDT
Created
attachment 155299
[details]
Patch
Gustavo Noronha (kov)
Comment 36
2012-07-30 09:26:54 PDT
Comment on
attachment 155299
[details]
Patch
Attachment 155299
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/13389378
Build Bot
Comment 37
2012-07-30 09:27:39 PDT
Comment on
attachment 155299
[details]
Patch
Attachment 155299
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/13381825
Early Warning System Bot
Comment 38
2012-07-30 09:29:00 PDT
Comment on
attachment 155299
[details]
Patch
Attachment 155299
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/13390429
Build Bot
Comment 39
2012-07-30 09:41:43 PDT
Comment on
attachment 155299
[details]
Patch
Attachment 155299
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/13387438
Hanna
Comment 40
2012-07-30 10:05:10 PDT
Created
attachment 155311
[details]
Patch
Pavel Feldman
Comment 41
2012-08-02 11:47:03 PDT
Comment on
attachment 155311
[details]
Patch Is there a way I get a pointer to a request for this feature? It sounds like 1) there are not so many scenarios that involve it and 2) worker context destruction time does not really map to what developer might expect well.
Hanna
Comment 42
2012-08-02 12:13:27 PDT
(In reply to
comment #41
)
> (From update of
attachment 155311
[details]
) > Is there a way I get a pointer to a request for this feature? It sounds like 1) there are not so many scenarios that involve it and 2) worker context destruction time does not really map to what developer might expect well.
The current worker panel uses instrumentation to worker context start/finished to add and remove workers in the panel, so it makes sense to use them here (only for dedicated workers) The original request was for the timeline to show all the events in the main and the worker process instead of having them in 2 seperate frontends (they want to see the events happening all together in one place and having a tag indicating it is from which process), and this is just a start. I would appreciate your opinions/thoughts on this project/request. It also makes sense if the user wants to see when the worker starts and how long it's been active relative to everything else
Pavel Feldman
Comment 43
2012-08-02 12:17:18 PDT
> The current worker panel uses instrumentation to worker context start/finished to add and remove workers in the panel, so it makes sense to use them here (only for dedicated workers)
It does not. Sources panel does not imply the timing is accurate, while timeline does.
> The original request was for the timeline to show all the events in the main and the worker process instead of having them in 2 seperate frontends (they want to see the events happening all together in one place and having a tag indicating it is from which process), and this is just a start. I would appreciate your opinions/thoughts on this project/request.
Worker front-end has its own Timeline, we can't show workers and main page on the same timeline and I wonder why one would need that.
> > It also makes sense if the user wants to see when the worker starts and how long it's been active relative to everything else
Given that this is a fairly rare debugging scenario, it seems like user could put console.timeStamp() marks in his code to track the worker lifetime.
Radar WebKit Bug Importer
Comment 44
2014-12-09 10:48:11 PST
<
rdar://problem/19192124
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug