Bug 25503 - Add instrumentation framework to WebCore
Summary: Add instrumentation framework to WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-30 19:01 PDT by James Robinson
Modified: 2009-09-08 11:35 PDT (History)
10 users (show)

See Also:


Attachments
Implementation of instrumentation and simple aggregate in DRT (51.68 KB, patch)
2009-04-30 19:02 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Addresses some comments and rebases against r43514 (51.76 KB, patch)
2009-05-11 15:07 PDT, James Robinson
oliver: review-
Details | Formatted Diff | Diff
Similar instrumentation points, but this time it goes to an agent in InspectorController. (37.70 KB, patch)
2009-08-21 11:21 PDT, Kelly Norton
timothy: review-
Details | Formatted Diff | Diff
Addresses review suggestions and updates to r47896 (53.13 KB, patch)
2009-08-31 15:37 PDT, Kelly Norton
timothy: review-
Details | Formatted Diff | Diff
Fixes the remaining iterator nit. (53.02 KB, patch)
2009-09-03 13:56 PDT, Kelly Norton
no flags Details | Formatted Diff | Diff
Adds Windows build changes. (54.40 KB, patch)
2009-09-04 15:49 PDT, Kelly Norton
timothy: review+
eric: commit-queue-
Details | Formatted Diff | Diff
Same patch updated to r48154. (55.45 KB, patch)
2009-09-08 08:55 PDT, Kelly Norton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2009-04-30 19:01:08 PDT
I think it would be good to have a lightweight instrumentation framework in WebCore to keep tabs on things like the number of layouts and their duration.
Comment 1 James Robinson 2009-04-30 19:02:55 PDT
Created attachment 29934 [details]
Implementation of instrumentation and simple aggregate in DRT

This is an example of instrumentation in layout, style recalculation, html parsing and painting.  There is also a fairly simplistic implementation of an aggregator in DumpRenderTree that will optionally spit out stats when running layout tests.
Comment 2 Sam Weinig 2009-05-04 17:08:10 PDT
Some basic comments.  The names of the client methods don't really mesh well with our current idiom for client methods.  For instance, instead of "layoutStarting", we would use "didStartLayout".

How does this effect the performance of the various perf tests we run? 
Comment 3 Simon Fraser (smfr) 2009-05-04 17:10:50 PDT
Some general feedback (I didn't read Sam's yet)

* I'm not sure I like the pairing of a specific start signal ("recalcStyleStarting") with a generic end signal ("finished"). Seems like it would be good to force pairing, even if just to assert that they are correctly paired.
* Rather than hard-coding all the categories in method names, why not make it extensible:

analysisClient()->start("parsing");
analysisClient()->end("parsing");

Then you just need a hash to store data in on the collection side.

* Why have WebAnalysisClient in WebKit? Seems like the mechanics of data collection and analysis will be the same for every platform, so should be in WebCore. Then what you export via WebKit is:
i) a way to turn it on and off
ii) a way to get at the raw data (e.g. for a UI visualizer) or summary data?
I think WebCore should also have code to dump the output. You could use a log channel for output, maybe.
Comment 4 James Robinson 2009-05-11 15:07:23 PDT
Created attachment 30205 [details]
Addresses some comments and rebases against r43514

This patch addresses the naming issues with AnalysisClient and creates a separate didFoo() callback instead of the generic finished() callback.  This way an implementation only interested in a subset of the instrumentation points could support only the willFoo()/didFoo() pairs they cared about.

This patch also merges up to r43514 and fixes some whitespace/path issues from the original patch.  I'll address the other comments raised in the bug directly.
Comment 5 James Robinson 2009-05-11 15:10:10 PDT
(In reply to comment #2)
> Some basic comments.  The names of the client methods don't really mesh well
> with our current idiom for client methods.  For instance, instead of
> "layoutStarting", we would use "didStartLayout".
> 
> How does this effect the performance of the various perf tests we run? 
> 

The performance of the instrumentation depends pretty much entirely on how much work is done by the AnalysisClient implementation.  In this case, since very little work is done by the DRT implementation there is no measurable performance impact as determined by running SunSpider, the layout tests in LayoutTests/fast, or a mozilla page cycler with some popular websites.  I don't have access to IBench.
Comment 6 James Robinson 2009-05-11 15:42:52 PDT
(In reply to comment #3)
> Some general feedback (I didn't read Sam's yet)
> 
> * I'm not sure I like the pairing of a specific start signal
> ("recalcStyleStarting") with a generic end signal ("finished"). Seems like it
> would be good to force pairing, even if just to assert that they are correctly
> paired.
> * Rather than hard-coding all the categories in method names, why not make it
> extensible:
> 
> analysisClient()->start("parsing");
> analysisClient()->end("parsing");
> 
> Then you just need a hash to store data in on the collection side.

The advantage of having separate callbacks is that the instrumentation points can be easily parameterized to keep data around beyond simple start/end timestamps. For example, the paint instrumentation point could also have the size of the bounding box being painted.

> 
> * Why have WebAnalysisClient in WebKit? Seems like the mechanics of data
> collection and analysis will be the same for every platform, so should be in
> WebCore. Then what you export via WebKit is:
> i) a way to turn it on and off
> ii) a way to get at the raw data (e.g. for a UI visualizer) or summary data?
> I think WebCore should also have code to dump the output. You could use a log
> channel for output, maybe.
> 

The DRT aggregation is fairly simple and could potentially apply to all platforms, but it's more intended as an example of a use of this data and not the only one.   We've experimented with tools that keep entire trace trees around and found that the data is quite interesting but also very large and probably doesn't make sense to keep around in WebCore proper.  In the other extreme, the Android folks have experimented with instrumentation points that do nothing except log to one of their system logs to keep the observer effects down while understand perf regressions.  I think it makes sense to keep the instrumentation itself very generic for now and figure out exactly what sort of processing should be in WebCore vs the platform layer as times goes on.  Does that approach seem good to you?
Comment 7 Oliver Hunt 2009-05-22 20:22:34 PDT
Comment on attachment 30205 [details]
Addresses some comments and rebases against r43514

I am not sure why these need to use client objects -- as this is primarily of value to developers it should be entirely inside webcore with appropriate bindings to the inspector.  Currently it sits in the inspector directory but the inspector does not use it at all.

I feel it's value in DRT is vastly less than in the inspector so there should be at least rudimentary information displayed in the inspector as part of this patch.

Also you're adding branches to some very hot code paths so i'd like to be sure that you've ensured that there isn't a performance regression in layout or rendering.

Given these issues i'm r-'ing this for now
Comment 8 Kelly Norton 2009-08-21 11:21:56 PDT
Created attachment 38382 [details]
Similar instrumentation points, but this time it goes to an agent in InspectorController.

This path is a much delayed follow up to James' patches that addresses the first part of Oliver's objection. Are there runnable perf tests for determining the impact of some of the branches this adds?

Also note that this does not add any UI to inspector. This is something I discussed with Timonthy and Sam a little while back and I think they were ok with that. (I requested Timothy on the review).

I'm also kellegous on IRC if there are questions better handled in real-time.
Comment 9 Timothy Hatcher 2009-08-27 09:26:27 PDT
Comment on attachment 38382 [details]
Similar instrumentation points, but this time it goes to an agent in InspectorController.


> +InspectorTimelineAgent* Document::inspectorTimelineAgent() const
> +{
> +    return page() ? page()->inspectorTimelineAgent() : 0;
> +}

Make these getters inline functions and it will be better. It would make the callers simplier to keep them.

> +    if (inspectorTimelineAgent())
> +        inspectorTimelineAgent()->willRecalculateStyle();

This should store the timeline agent into a local so the work isn't done twice when it is enabled. Same for the other callers.

> +void InspectorTimelineAgent::willDispatchDOMEvent(const Event& event)
> +{
> +    TimelineItem* newItem = newTimelineItem(DOMDispatchTimelineItemType);
> +    newItem->addDOMEventProperties(m_frontend, event);
> +}
> +
> +void InspectorTimelineAgent::didDispatchDOMEvent()
> +{
> +    didCompleteCurrentRecord();
> +}

I worry about didCompleteCurrentRecord() ending the wrong record. As long as all the recorded things are synchronous it is fine. We should ASSERT here that the current TimelineItem is the right type (DOMDispatchTimelineItemType in this case). And in all the other places didCompleteCurrentRecord() is called.

> +void InspectorTimelineAgent::TimelineItem::addItemProperties(double endTime)
> +{
> +    m_scriptObject.set("time", m_startTime);
> +    m_scriptObject.set("type", static_cast<int>(m_itemType));
> +    m_scriptObject.set("duration", endTime - m_startTime);
> +    if (m_childObjects.length())
> +        m_scriptObject.set("children", m_childObjects);
> +}
> +
> +void InspectorTimelineAgent::TimelineItem::addDOMEventProperties(InspectorFrontend* frontend, const Event& event)
> +{
> +    ScriptObject data = frontend->newScriptObject();
> +    data.set("type", event.type().string());
> +    m_scriptObject.set("data", data);
> +}

These should be normal data members, and more like ConsoleMessage when the ScriptObjects are created. Like what Pavel was saying on IRC.

> +        // Must be kept in sync with Timeline.js

This should say TimelineAgent.js.
Comment 10 Kelly Norton 2009-08-31 15:37:25 PDT
Created attachment 38838 [details]
Addresses review suggestions and updates to r47896
Comment 11 Timothy Hatcher 2009-09-03 13:04:44 PDT
Comment on attachment 38838 [details]
Addresses review suggestions and updates to r47896

Looks good! Some small nits. Will r+ when you fix these.

> +    InspectorTimelineAgent* timelineAgent = inspectorTimelineAgent();
> +    if (timelineAgent)
> +        timelineAgent->willRecalculateStyle();

You can combine these into 2 lines. Same comment for the rest of the palces like this.

    if (InspectorTimelineAgent* timelineAgent = inspectorTimelineAgent())
        timelineAgent->willRecalculateStyle();

> +    int i = 0;
> +    for (TimelineItems::const_iterator it(m_children.begin()); it != m_children.end(); ++it) {
> +        ScriptObject childObject = (*it)->convertToScriptObject(frontend);
> +        children.set(i++, childObject);
> +    }

Just use a normal for loop and a count and not iterators.

> +InspectorTimelineAgent* Page::inspectorTimelineAgent() const
> +{
> +    return m_inspectorController->timelineAgent();
> +}

You should inline this in the header.
Comment 12 Kelly Norton 2009-09-03 13:56:52 PDT
Created attachment 39009 [details]
Fixes the remaining iterator nit.
Comment 13 Pavel Feldman 2009-09-04 02:08:24 PDT
I think it will break the Windows WebKit build. Could you update vcproj file as well?
Comment 14 Kelly Norton 2009-09-04 15:49:59 PDT
Created attachment 39093 [details]
Adds Windows build changes.
Comment 15 Eric Seidel (no email) 2009-09-06 21:56:15 PDT
Comment on attachment 39093 [details]
Adds Windows build changes.

Rejecting patch 39093 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=39093 from bug 25503 failed to download and apply.
Comment 16 Eric Seidel (no email) 2009-09-07 00:13:27 PDT
Several patch application failures, including the XCode project file.  the commit-queue can't land this unless someone updates it for tip of tree.
Comment 17 Kelly Norton 2009-09-08 08:55:18 PDT
Created attachment 39191 [details]
Same patch updated to r48154.
Comment 18 Eric Seidel (no email) 2009-09-08 09:13:36 PDT
Comment on attachment 39191 [details]
Same patch updated to r48154.

Based on Tim's previous r+, this still looks fine.  The bots are red atm so it will be a bit before the commit-queue wakes up and commits this.
Comment 19 Eric Seidel (no email) 2009-09-08 10:55:45 PDT
Comment on attachment 39191 [details]
Same patch updated to r48154.

Clearing flags on attachment: 39191

Committed r48167: <http://trac.webkit.org/changeset/48167>
Comment 20 Eric Seidel (no email) 2009-09-08 10:55:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Dimitri Glazkov (Google) 2009-09-08 11:35:55 PDT
Build fix landed as http://trac.webkit.org/changeset/48171.