Bug 84883 - [Resource Timing] Expose timing information
Summary: [Resource Timing] Expose timing information
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Simonsen
URL: http://dvcs.w3.org/hg/webperf/raw-fil...
Keywords:
Depends on: 61152 101300 102151 102862 102995 103777 103927
Blocks: 61138 84885 84886 85026
  Show dependency treegraph
 
Reported: 2012-04-25 12:34 PDT by James Simonsen
Modified: 2017-02-10 11:45 PST (History)
21 users (show)

See Also:


Attachments
Patch (66.09 KB, patch)
2012-07-13 16:02 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (329.28 KB, application/zip)
2012-07-13 18:47 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-01 (369.52 KB, application/zip)
2012-07-13 19:50 PDT, WebKit Review Bot
no flags Details
Patch (66.67 KB, patch)
2012-07-16 12:21 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (176.25 KB, patch)
2012-10-01 20:54 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (176.36 KB, patch)
2012-10-02 11:07 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (180.57 KB, patch)
2012-10-02 12:03 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (178.47 KB, patch)
2012-10-02 15:53 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (203.53 KB, patch)
2012-10-09 17:36 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (203.61 KB, patch)
2012-10-15 17:21 PDT, James Simonsen
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 Simonsen 2012-04-25 12:34:21 PDT
This is the main work to plumb the resource timing information to the Performance Timeline.
Comment 1 James Simonsen 2012-07-13 16:02:20 PDT
Created attachment 152361 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-13 18:47:53 PDT
Comment on attachment 152361 [details]
Patch

Attachment 152361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13238299

New failing tests:
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_dynamic_insertion.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_initiator_types.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_redirects.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_script_types.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_attribute_order.html
Comment 3 WebKit Review Bot 2012-07-13 18:47:58 PDT
Created attachment 152397 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 4 WebKit Review Bot 2012-07-13 19:50:11 PDT
Comment on attachment 152361 [details]
Patch

Attachment 152361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13242317

New failing tests:
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_dynamic_insertion.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_initiator_types.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_redirects.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_script_types.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_attribute_order.html
Comment 5 WebKit Review Bot 2012-07-13 19:50:15 PDT
Created attachment 152401 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 6 James Simonsen 2012-07-16 12:21:13 PDT
Created attachment 152584 [details]
Patch
Comment 7 James Simonsen 2012-10-01 20:54:55 PDT
Created attachment 166597 [details]
Patch
Comment 8 Build Bot 2012-10-01 21:17:45 PDT
Comment on attachment 166597 [details]
Patch

Attachment 166597 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14132058
Comment 9 Build Bot 2012-10-01 21:25:39 PDT
Comment on attachment 166597 [details]
Patch

Attachment 166597 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14137019
Comment 10 James Simonsen 2012-10-02 11:07:34 PDT
Created attachment 166714 [details]
Patch
Comment 11 Build Bot 2012-10-02 11:40:46 PDT
Comment on attachment 166714 [details]
Patch

Attachment 166714 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14130381
Comment 12 Build Bot 2012-10-02 11:49:33 PDT
Comment on attachment 166714 [details]
Patch

Attachment 166714 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14126421
Comment 13 James Simonsen 2012-10-02 12:03:46 PDT
Created attachment 166726 [details]
Patch
Comment 14 Build Bot 2012-10-02 12:30:46 PDT
Comment on attachment 166726 [details]
Patch

Attachment 166726 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14131346
Comment 15 James Simonsen 2012-10-02 15:53:55 PDT
Created attachment 166768 [details]
Patch
Comment 16 Tony Gentilcore 2012-10-02 17:23:19 PDT
Comment on attachment 166768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166768&action=review

Overall this patch looks good to me. Just a couple of nits and test suggestions.

I feel comfortable approving the tests and performance code, but the loader changes are fundamental enough that I'd prefer it if Nate, Adam or someone would take a look.

> Source/WebCore/css/CSSLoadInitiator.h:40
> +namespace WebCore {

Line break after namespace

> Source/WebCore/loader/ThreadableLoader.h:-44
> -    class ResourceError;

In such a large patch, I recommend resisting the urge to do unrelated style fixes.

> LayoutTests/ChangeLog:34
> +        * http/tests/w3c/webperf/submission/resource-timing/html/test_resource_script_types.html: Added.

Please add a test for reparenting nodes across documents.

> LayoutTests/http/tests/w3c/webperf/resources/all_resource_types.htm:39
> +        </ol>

Some more resource types to consider testing:
<applet archive>
<audio src>
<body background>
<embed src>
<input type=image src>
<object data>
<track src>
<video poster>
<video src>

Some things that might need a new test:
<frame src>
<html manifest>

> LayoutTests/http/tests/w3c/webperf/submission/resource-timing/html/test_resource_attribute_order-expected.txt:10
> +FAIL fetchStart should be non-zero assert_true: fetchStart should be non-zero expected true got false

Is this really expected right now?
Comment 17 Adam Barth 2012-10-03 02:25:04 PDT
Comment on attachment 166768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166768&action=review

> Source/WebCore/css/CSSLoadInitiator.h:51
> +    Document* m_document;

What ensures that this doesn't point to garbage memory?
Comment 18 Adam Barth 2012-10-03 02:26:38 PDT
Comment on attachment 166768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166768&action=review

> Source/WebCore/loader/LoadInitiator.h:43
> +    virtual ~LoadInitiator() { }

You've added this class as a base class to Node, which means you've added a new vtable to Node, growing its size by a pointer.
Comment 19 James Simonsen 2012-10-09 17:35:45 PDT
Comment on attachment 166768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166768&action=review

>> Source/WebCore/css/CSSLoadInitiator.h:40
>> +namespace WebCore {
> 
> Line break after namespace

Done.

>> Source/WebCore/css/CSSLoadInitiator.h:51
>> +    Document* m_document;
> 
> What ensures that this doesn't point to garbage memory?

Switched to a RefPtr.

Since LoadInitiators are temporaries, I figured it'd never point to garbage. But, there's probably some code path where requesting a resource destroys the document, so better safe than sorry.

>> Source/WebCore/loader/LoadInitiator.h:43
>> +    virtual ~LoadInitiator() { }
> 
> You've added this class as a base class to Node, which means you've added a new vtable to Node, growing its size by a pointer.

Good point. Changed the design so it no longer uses inheritance.

>> LayoutTests/ChangeLog:34
>> +        * http/tests/w3c/webperf/submission/resource-timing/html/test_resource_script_types.html: Added.
> 
> Please add a test for reparenting nodes across documents.

Good idea. Done.

>> LayoutTests/http/tests/w3c/webperf/resources/all_resource_types.htm:39
>> +        </ol>
> 
> Some more resource types to consider testing:
> <applet archive>
> <audio src>
> <body background>
> <embed src>
> <input type=image src>
> <object data>
> <track src>
> <video poster>
> <video src>
> 
> Some things that might need a new test:
> <frame src>
> <html manifest>

<body background>
<embed src>
<input type=image src>
<object data>
<video poster>
<frame src>

Done.

<applet archive>

This parameter is passed to the plugin to be loaded. WebKit has no visibility.

<audio src>
<track src>
<video src>

The spec intentionally omits media types until we resolve how to handle streaming, etc.

<html manifest>

This is passed to the embedder to be loaded. WebKit has no visibility. We might want to add hooks back into WebKit later to fix this. I think it should be a separate bug though.

>> LayoutTests/http/tests/w3c/webperf/submission/resource-timing/html/test_resource_attribute_order-expected.txt:10
>> +FAIL fetchStart should be non-zero assert_true: fetchStart should be non-zero expected true got false
> 
> Is this really expected right now?

Nope, fixed. But do note that DRT doesn't populate these so this test isn't especially useful anyway. It passes in Chrome too though.
Comment 20 James Simonsen 2012-10-09 17:36:07 PDT
Created attachment 167880 [details]
Patch
Comment 21 Tony Gentilcore 2012-10-15 16:28:45 PDT
Comment on attachment 167880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167880&action=review

lgtm!

However, I'd feel better if abarth@ or japhet@ reviewed since this still touches stuff in loader.

> Source/WebCore/loader/DocumentLoadTiming.h:74
> +    double rawNavigationStart() const { return m_navigationStart; }

Too bad about this method. Is there a more descriptive term than "raw"? Better yet, is there a way to avoid exposing it here altogether?

> Source/WebCore/page/PerformanceResourceTiming.cpp:50
> +    if (!seconds)

Is this test necessary?

> LayoutTests/platform/chromium/TestExpectations:3794
> +# webkit.org/b/61138 http/tests/w3c/webperf/submission/resource-timing [ Skip ]

Don't forget to uncomment this line. Also, is it necessary for other platforms?
Comment 22 James Simonsen 2012-10-15 17:21:41 PDT
Created attachment 168811 [details]
Patch
Comment 23 James Simonsen 2012-10-15 17:24:21 PDT
(In reply to comment #21)
> (From update of attachment 167880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167880&action=review
> 
> lgtm!
> 
> However, I'd feel better if abarth@ or japhet@ reviewed since this still touches stuff in loader.

Nate or Adam, any chance you could take a look at this (again)?

> 
> > Source/WebCore/loader/DocumentLoadTiming.h:74
> > +    double rawNavigationStart() const { return m_navigationStart; }
> 
> Too bad about this method. Is there a more descriptive term than "raw"? Better yet, is there a way to avoid exposing it here altogether?

Renamed to monotonicNavigationStart. Do you like that better? I don't think I can get rid of it entirely, but I'm open to other names.

> 
> > Source/WebCore/page/PerformanceResourceTiming.cpp:50
> > +    if (!seconds)
> 
> Is this test necessary?

Good point. Not anymore. The new monotonicTimeToZeroBasedDocumentTime will behave correctly.
> 
> > LayoutTests/platform/chromium/TestExpectations:3794
> > +# webkit.org/b/61138 http/tests/w3c/webperf/submission/resource-timing [ Skip ]
> 
> Don't forget to uncomment this line. Also, is it necessary for other platforms?

Done and done. The Apple ports skip the entire w3c directory, so no need to update those.
Comment 24 Eric Seidel (no email) 2012-10-15 17:25:25 PDT
Attachment 168811 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/http/tests/w3c/webperf/resources/blank_image.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 94 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Adam Barth 2012-10-16 11:33:49 PDT
Comment on attachment 168811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168811&action=review

> Source/WebCore/css/CSSLoadInitiator.h:47
> +    CSSLoadInitiator(Document*);

Please mark these sort of one-argument constructors with the "explicit" keyword.

> Source/WebCore/css/CSSLoadInitiator.h:50
> +    virtual const WTF::AtomicString& initiatorName() const;

The "WTF::" prefix isn't needed here.

> Source/WebCore/dom/NodeLoadInitiator.h:51
> +    NodeLoadInitiator(Node*);
> +    virtual ~NodeLoadInitiator();
> +
> +    virtual const WTF::AtomicString& initiatorName() const;
> +    virtual Document* initiatorDocument() const;

Similar nits here.

> Source/WebCore/html/HTMLBodyElement.cpp:79
> +            style->setProperty(CSSProperty(CSSPropertyBackgroundImage, imageValue));

imageValue -> imageValue.release()

> Source/WebCore/html/parser/CSSPreloadScanner.h:38
> +class CSSPreloadScanner : private LoadInitiator {

We don't usually use private inheritance, but I guess it makes sense here...

> Source/WebCore/html/parser/CSSPreloadScanner.h:41
>      CSSPreloadScanner(Document*);

Unrelated: This constructor should be marked explicit.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:149
> +    Document* m_document;

How do we know this document will alive as long as this task is alive?

> Source/WebCore/loader/LoadInitiatorHandle.cpp:46
> +    , m_time(monotonicallyIncreasingTime())

So, the copy constructor gets a new time value?  That's confusing.  Can we make this operation explicit rather than implicit?

> Source/WebCore/loader/MainResourceLoader.cpp:534
> +        m_frame->ownerElement()->document()->domWindow()->performance()->addResourceTiming(NodeLoadInitiator(m_frame->ownerElement()), originalRequest(), m_response, documentLoader()->timing()->monotonicNavigationStart(), finishTime);

Why does this get added to the parent document's performance object?  It seems entirely possible that the navigation was initiated from within this frame.

> Source/WebCore/loader/TextTrackLoader.cpp:45
> -    
> +

I ran out of steam reviewing this CL around here.
Comment 26 Adam Barth 2012-10-16 11:39:01 PDT
> 203.61 KB

This patch is a bit big.  :(

There are a bunch of reasons we'd like to know how a load was initiated.  We already have this concept in FrameLoadRequest, for example.

It's not clear to me that it makes sense to store this information in CachedResource, however, since CachedResources are shared by multiple requestors.  Consider an image that is being loaded by two different documents at the same time.  Which one of them is the anointed requestor?

The underlying issue seems to be that the CachedResourceLoader doesn't have a notion of a "request" object to wrap the lower level ResourceRequest object in the same way that FrameLoadRequests wraps ResourceRequest.
Comment 27 James Simonsen 2012-10-16 11:52:19 PDT
(In reply to comment #26)
> > 203.61 KB
> 
> This patch is a bit big.  :(

Yeah, sorry. I broke up this feature as best I could. This one, unfortunately, is the biggest single chunk.

> There are a bunch of reasons we'd like to know how a load was initiated.  We already have this concept in FrameLoadRequest, for example.
> 
> It's not clear to me that it makes sense to store this information in CachedResource, however, since CachedResources are shared by multiple requestors.  Consider an image that is being loaded by two different documents at the same time.  Which one of them is the anointed requestor?

In the Resource Timing spec, it's the first one to request to it, even if the first document cancels the request or the document itself goes away.

> The underlying issue seems to be that the CachedResourceLoader doesn't have a notion of a "request" object to wrap the lower level ResourceRequest object in the same way that FrameLoadRequests wraps ResourceRequest.

That's fine with me too. I'd be happy to create a CachedResourceRequest to wrap this state. Is that what you want?

Should we try to merge it with FrameLoadRequest too?
Comment 28 Adam Barth 2012-10-16 13:08:38 PDT
> In the Resource Timing spec, it's the first one to request to it, even if the first document cancels the request or the document itself goes away.

Does that make sense for other (future) uses of this information?  Having the initiator around is going to tempt future code into use it.  We should be sure we're doing something sensible.

> That's fine with me too. I'd be happy to create a CachedResourceRequest to wrap this state. Is that what you want?

I haven't thought through the design enough to know what the best approach is.  I'm just sharing what I was thinking when reviewing your patch.

> Should we try to merge it with FrameLoadRequest too?

I don't think that makes sense.  FrameLoadRequest's primary job is to hold m_frameName, which doesn't make sense for subresource loads.

I think the underlying issue here is that CachedResourceLoader::requestResource takes a lot of arguments instead of a CachedResourceRequest object.  I would probably start by creating the request object to hold that information and see where that approach leads.

I would also try to avoid adding a LoadInitiatorHandle member variable to CachedResource because the cached resource is shared between many initiators.  It's going to confuse future code if people think that there's a unique initiator for each CachedResource.

(Also, all the spurious whitespace changes in your patch make it harder to see what's going on.)
Comment 29 James Simonsen 2012-10-16 13:58:23 PDT
(In reply to comment #28)
> I think the underlying issue here is that CachedResourceLoader::requestResource takes a lot of arguments instead of a CachedResourceRequest object.  I would probably start by creating the request object to hold that information and see where that approach leads.

Okay, I can do that and post a patch. It seems like a pretty straightforward refactoring. It shouldn't affect the behavior at all.

> I would also try to avoid adding a LoadInitiatorHandle member variable to CachedResource because the cached resource is shared between many initiators.  It's going to confuse future code if people think that there's a unique initiator for each CachedResource.

Well there is a unique initiator for the purposes of Resource Timing: it's always the first one. Are you implying that Resource Timing's definition doesn't belong in loader/? Or is "initiator" not a clear enough term to describe the first element making the request? Maybe I should call the argument passed in to requestResource() "requester" and the first requester becomes the "initiator."

Another ideas is to make PerformanceResourceTiming into an observer and have it store the LoadInitiatorHandles itself. Then we don't need CachedResource to hang on to its initiator.
Comment 30 Adam Barth 2012-10-17 10:45:06 PDT
See bug 92761 for another use case for figuring out who initiated a load.
Comment 31 Joseph Pecoraro 2017-02-10 11:45:45 PST
This bug seems stale. Initiator information is working in the current implementation of Resource Timing, so this bug seems stale. Lets close.