Maybe add an indicator somewhere on timeline, like add more details to the popup generated by mouse hovering? Recommendations appreciated, thank you.
add an event on timeline indicating when a worker started and when it's destroyed instead
Created attachment 151496 [details] Patch
anyone r?
Comment on attachment 151496 [details] Patch Attachment 151496 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13204055
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.
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
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)
Created attachment 151710 [details] Patch
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
r?
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?
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.
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
> > 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?
(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
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.
(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
Created attachment 154976 [details] Patch
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)
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
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
Created attachment 154987 [details] Patch
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.
Created attachment 155008 [details] Patch
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
Comment on attachment 155008 [details] Patch r- as per my earlier review.
Created attachment 155012 [details] Patch
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
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.
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.
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.
> 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.
Created attachment 155049 [details] Patch
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.
Created attachment 155299 [details] Patch
Comment on attachment 155299 [details] Patch Attachment 155299 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13389378
Comment on attachment 155299 [details] Patch Attachment 155299 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13381825
Comment on attachment 155299 [details] Patch Attachment 155299 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13390429
Comment on attachment 155299 [details] Patch Attachment 155299 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13387438
Created attachment 155311 [details] Patch
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.
(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
> 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.
<rdar://problem/19192124>