There is desire to instrument WebCore codebase for better understanding about its behavior. We already have InspectorInstrumentation. But we also want to have - chromium profiler - timing api - experimental metrics like css property sharing status We should have a way to give these instrumentation in more uniform, less distractive manner. Blocking bugs are tentative. Feel free to add/remove them.
Created attachment 134034 [details] Patch
Comment on attachment 134034 [details] Patch Attachment 134034 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12142485
Comment on attachment 134034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134034&action=review Some comments. Could you share how you envision the next step(s)? For now, it looks like we need to add a lot of boiler-plate code to get some new instrumentation into WebKit, will this effort change that? (it may be an orthogonal goal but still would be neat) > Source/WebCore/dom/Document.cpp:4547 > - > - f->loader()->finishedParsing(); > - > - InspectorInstrumentation::domContentLoadedEventFired(f.get()); > + f->finishedParsing(); I don't really see the need for this function. It's used only here and the Frame didn't care about parsing until you added this function. > Source/WebCore/instrument/DocumentInstrumentation.h:49 > + InstrumentBeginTrace("dom", "Document::recalcStyle"); I don't see the need for that. If the inspector is already catching this, why do we need more notifications? > Source/WebCore/instrument/LayoutSchedulingInstrumentation.h:64 > + printf("LayoutScheduling: %s at time %d\n", functionName, document->elapsedTime()); As we are moving this code, it would be a good time to actually switch to WTF::log. > Source/WebCore/instrument/LayoutSchedulingInstrumentation.h:93 > + printf("LayoutScheduling: %s at time %d. %d stylesheets still remain.\n", __FUNCTION__, document->elapsedTime(), document->pendingStylesheets()); Ditto. > Source/WebCore/instrument/LayoutSchedulingInstrumentation.h:109 > +#endif // !defined(LayoutSchedulingInstrumentation_h) Nit: The preferred style is without the !defined() AFAICT.
We seem to have this general direction of modularizing things because modularization is always good, and this patch centralizes instrumentation because centralization is always good, correct?
Comment on attachment 134034 [details] Patch I don't like (or don't understand) the way you are changing things. I'd like to avoid wrapping inspector instrumentation, I don't see why document instrumentation and LayoutSchedulingInstrumentation are extracted. I also think that hiding Chromium-specific TraceEvents behind the WebCore's instrumentation is a wrong approach. Could you please explain your intent upfront in the bug and wait for the feedback from the inspector team prior to suggesting patches for review?
Hi folks, thanks for your feedback! (In reply to comment #4) > We seem to have this general direction of modularizing things because modularization is always good, and this patch centralizes instrumentation because centralization is always good, correct? Good point. The summary was wrong. This is yet another modularization, of instrumentation for this case. I'll update the summary. My intention is to have things under similar concept in the same place. (In reply to comment #5) > (From update of attachment 134034 [details]) > I don't like (or don't understand) the way you are changing things. I'd like to avoid wrapping inspector instrumentation, I don't see why document instrumentation and LayoutSchedulingInstrumentation are extracted. I also think that hiding Chromium-specific TraceEvents behind the WebCore's instrumentation is a wrong approach. Could you please explain your intent upfront in the bug and wait for the feedback from the inspector team prior to suggesting patches for review? Sure. I should've CC-ed inspector team. In this patch, I'm trying to do two things as you pointed: A: To isolate instrumentation from usual code path. InspectorInstrumentation is doing this well. I want to have it for chromium TraceEvent. But simply instrumenting the code like this http://wkb.ug/76325 makes the main logic hard to read. I want to keep same level of intrusiveness of InspectorInstrumentation even after TraceEvent-based instrumentation. B: To categorize instrumentation. I thinks this makes sense because by splitting instrumentation into separate files, the number of files to be re-compiled will become smaller. But there might be some cons for this approach. I'd like to hear your feedback. My primary motivation is A. B is kinda side effect of that. On LayoutSchedulingInstrumentation I felt it as an instrumentation and can be isolated out. But it might be overkill. Any suggestions are appreciated. Thanks,
(In reply to comment #6) > Hi folks, thanks for your feedback! > > (In reply to comment #4) > > We seem to have this general direction of modularizing things because modularization is always good, and this patch centralizes instrumentation because centralization is always good, correct? > > Good point. The summary was wrong. This is yet another modularization, of instrumentation > for this case. I'll update the summary. > My intention is to have things under similar concept in the same place. > InspectorInstrumentation will itself be split into the core instrumentation and modules instrumentations (such as FileSystemInstrumentation) as a part of the modularization process. > > (In reply to comment #5) > > (From update of attachment 134034 [details] [details]) > > I don't like (or don't understand) the way you are changing things. I'd like to avoid wrapping inspector instrumentation, I don't see why document instrumentation and LayoutSchedulingInstrumentation are extracted. I also think that hiding Chromium-specific TraceEvents behind the WebCore's instrumentation is a wrong approach. Could you please explain your intent upfront in the bug and wait for the feedback from the inspector team prior to suggesting patches for review? > > Sure. I should've CC-ed inspector team. > > In this patch, I'm trying to do two things as you pointed: > > A: To isolate instrumentation from usual code path. > InspectorInstrumentation is doing this well. I want to have it for chromium TraceEvent. InspectorInstrumentation and TraceEvent do not share the data flow. One is reporting data to the inspector core and front-end, the other sends the data into the embedder. Mixing the two is considered harmful. > But simply instrumenting the code like this http://wkb.ug/76325 makes the main logic > hard to read. I want to keep same level of intrusiveness of InspectorInstrumentation > even after TraceEvent-based instrumentation. > I think we should focus on streamlining the way instrumentation is done. I would not invest into standardizing on the TraceEvent or its wrapper, I'd rather figure out why Inspector's instrumentation is not solving embedder's needs. > B: To categorize instrumentation. > I thinks this makes sense because by splitting instrumentation into separate files, > the number of files to be re-compiled will become smaller. > But there might be some cons for this approach. I'd like to hear your feedback. > See the note on modularization above. > My primary motivation is A. B is kinda side effect of that. > > On LayoutSchedulingInstrumentation > I felt it as an instrumentation and can be isolated out. > But it might be overkill. > > Any suggestions are appreciated. > Thanks,
Hi Pavel, Thanks for your feedback and suggestion! I was not aware of modularization plan about inspector. That is great to hear. This change shouldn't bother the process. The difference between inspector and TraceEvent is making sense. I agree that it won't be a good abstraction to blindly unify both. My understanding why Chromium has TraceEvent apart from inspector is that we want to see when layout/styleRecalc is happening in context of GPU load. So it might be helpful if there is some way to flow a part of inspector's instrumentation data into TraceEvent. I'd like to hear compositor folks' opinion here though. My original motivation was to figure out the way to land http://wkb.ug/76325 in less intrusive manner. Another intention behind is that I want to have a place to put metrics-collection related code. But apparently this change is overrun for that purpose and bringing inspector as part of it is a wrong direction anyway. I need to find something better.
Let me throw something out there... I'm not sure what conceretely to do, myself, but maybe talking philosophy for a second might help. I've always thought that there's a distinction between "user-facing instrumentation" and "stuff a developer might want to instrument." In our case, the "user" is a web developer. User facing instrumentation ends up being (a) supported over a long period of time and (b) the user can do actionable things based on seeing that instrumentation. For example "paint' --- if its big, you can go poke at your borders. This is currently served by InspectorInstrumentation. Developer facing instrumentation is stuff you use for automated perf testing, to drive tooling [shark, instruments, dtrace, trace_event, linux kernel markers, graphics debuggers, memory checkers, etc etc etc]. Its highly coupled to the current implementation and changes along with the codebase. We have this with our logging channels, today. In general, instrumentation that is user facing is also of interest to developer instrumentation. Paint for example --- we give it to users, and we also want it for tools. So, maybe two questions to get people's opinions about: 1) should we allow developer-facing instrumentation into WebCore? 2) if we do it, should it use the same infrastructure as user facing instrumentation, or different? And, if the instrumentation to #2 is "different," then 3) how do we make user-facing instrumentation available to the developer instrumentation API? Pavel, any thoughts?
@pfeldman, ping on above?
(In reply to comment #10) > @pfeldman, ping on above? Sorry, thread got archived. > So, maybe two questions to get people's opinions about: > 1) should we allow developer-facing instrumentation into WebCore? I am not sure, this is outside my scope. I would send an email to webkit-dev with the proposal. I would expect some ports to require that it is behind a certain macro. > 2) if we do it, should it use the same infrastructure as user facing instrumentation, or different? We can make InspectorInstrumentation extensible. But it should only be used for the cases where the signals are leveraging inspector's general data flow. I.e. resulting data is accessible via inspector front-end and/or remote debugging. What I don't want to see is non-inspector instrumentation pretending to be such, but behaving differently (say reporting results by means specific, non-inspector WebKit client APIs). > > And, if the instrumentation to #2 is "different," then > 3) how do we make user-facing instrumentation available to the developer instrumentation API? I am not sure I fully understand this one. You mean about:tracing-alike UIs on the WebCore level?
(In reply to comment #11) > I am not sure, this is outside my scope. I would send an email to webkit-dev with the proposal. I would expect some ports to require that it is behind a certain macro. Great idea. Thanks. :) > > And, if the instrumentation to #2 is "different," then > > 3) how do we make user-facing instrumentation available to the developer instrumentation API? > > I am not sure I fully understand this one. You mean about:tracing-alike UIs on the WebCore level? Suppose I want to have about:tracing show recalculateStyle. There are probes in Document::recalculateStyle for inspector. Should we just have two probes in there, one for the profiling api and one for inspector? Its totally cool if your answer is "ask webkit-dev" :)
> Suppose I want to have about:tracing show recalculateStyle. There are probes in Document::recalculateStyle for inspector. Should we just have two probes in there, one for the profiling api and one for inspector? > > Its totally cool if your answer is "ask webkit-dev" :) Nah, asking webkit-dev is waste of time :) I would say that about:tracing's backend should act as an alternate inspector front-end and attach to the inspector backend. In Chromium terms it is: Implement your own DevToolsClientHost (front-end representative). We have three of them now: one for remote debugging, one for embedded devtools window and one for chrome.debugger extensions API. Your instance will receive all the JSON messages inspector's front-end is typically getting, messages will conform to the remote debugging protocol (https://developers.google.com/chrome-developer-tools/docs/protocol/tot/). Then push it to the about:tracing's client code and combine the figures with your own instrumentation. In WebKit terms it is: Embedder can attach to the inspector instrumentation from within its internal facilities. It should act as an alternate front-end for the inspector instrumentation. Non-inspector instrumentation will be getting to the embedder differently, so it should not look like inspector's one. Does it make sense?
(In reply to comment #13) > Nah, asking webkit-dev is waste of time :) > > I would say that about:tracing's backend should act as an alternate inspector front-end and attach to the inspector backend. I follow, and if its the only solution, thats okay. However, two concerns: 1) Consider writing WebKit perf test that e.g. ryoske or jchaffraix are trying to write that wants to measure css relayout time. You're thinking we should do the same thing and have the perf test install a frontend too? 2) Any tab that has inspector open will have its inspector instrumentation un-traceable to about:tracing. That makes it hard for about:tracing to give a reliable "ground truth" answer, which is what we want out of that tool. Or, is it possible to have multiple frontends?
(In reply to comment #14) > (In reply to comment #13) > > Nah, asking webkit-dev is waste of time :) > > > > I would say that about:tracing's backend should act as an alternate inspector front-end and attach to the inspector backend. > > I follow, and if its the only solution, thats okay. > I don't think it is the only solution, but I think that's the way embedder should interact with WebKit. It seems like I'm missing something because you clearly imply that you would not want it to work so. Let me re-iterate: I want it to be clear which instrumentation is for what, so that the call site would be giving a clue on where the information goes. I also think that different instrumentation may end up behind different compile-time flags. > However, two concerns: > 1) Consider writing WebKit perf test that e.g. ryoske or jchaffraix are trying to write that wants to measure css relayout time. You're thinking we should do the same thing and have the perf test install a frontend too? > If they want this information for be available to the web developers, they should use InspectorInstrumentation. They can access collected data via being an alternate front-end or exposing it through window.internals. If they don't, let them come up with alternative Instrumentation that will look different from inspector's (say PerfInstrumentation) and expose information as they would like. > 2) Any tab that has inspector open will have its inspector instrumentation un-traceable to about:tracing. That makes it hard for about:tracing to give a reliable "ground truth" answer, which is what we want out of that tool. Or, is it possible to have multiple frontends? Correct, it is not possible to have multiple frontends. I don't think it is a significant limitation though. You don't typically attach two front-ends to the profiler.