New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`PerformanceObserverInit.buffered` and `add to performance entry buffer` flags #76

Merged
merged 1 commit into from May 17, 2017

Conversation

Projects
None yet
6 participants
@cvazac
Copy link
Contributor

cvazac commented Mar 30, 2017

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Mar 30, 2017

Hmm, looks like the IPR bot is looking for WICG membership, even though this repo got moved back to the WebPerfWG. @dontcallmedom?

index.html Outdated
<!-- TODO: add long task spec reference -->
and <code>"longtask"</code>.</p>
<p>
The <dfn for="PerformanceEntry">startTime</dfn> attribute MUST return
the time value of the first recorded timestamp of this
performance metric.
performance metric. If the startTime concept doesn't apply, a
performance metric may choose to return a `startTime` of <code>0</code>.

This comment has been minimized.

@yoavweiss

yoavweiss Mar 30, 2017

Contributor

Should we change "may choose to" to "SHOULD" or "MUST"?

@cvazac cvazac changed the title buffer only until `onload`, add references to server-timing, add 'bufferedTypes' param buffer only until `onload`, add references to server-timing, add `bufferedTypes` param Mar 30, 2017

@yoavweiss
Copy link
Contributor

yoavweiss left a comment

LGTM!

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Mar 30, 2017

@igrigorik - want to take a look before merging?

@igrigorik
Copy link
Member

igrigorik left a comment

Aside: please use tidy to avoid formatting wars + enforce line length.. :-)

tidy -m -config tidyconfig.txt index.html

index.html Outdated
@@ -131,7 +131,7 @@
&lt;/html&gt;
</pre>
<p>Alternatively, the developer can observe the <a>Performance Timeline</a>
and be notified of new performance metrics via the
and be notified of both previously buffered, and new performance metrics via the

This comment has been minimized.

@igrigorik

igrigorik Mar 30, 2017

Member

We should clarify that buffering only applies until onload.

"... and be notified of new performance metrics, and optionally buffered performance metrics recorded prior to the load event of the document, via the..."

index.html Outdated

// retrieve the buffered Resource-Timing, User-Timing, and Server-Timing events
bufferedTypes:
['resource', 'mark', 'measure', 'server']

This comment has been minimized.

@igrigorik

igrigorik Mar 30, 2017

Member

This seems a little odd. What's the use case for consuming one set of buffered metrics, and observing another? Can we simplify this down a boolean? E.g. subscribe to "mark" and buffered: true also returns all same marks.

This comment has been minimized.

@cvazac

cvazac Mar 30, 2017

Contributor

You are suggesting that the buffered:true will return all of the buffered entries of all of the specified entryTypes?

I know that no one cares about onload anymore, but if an analytics script only wants to report on timing entries for blocking resources, with this approach, they wouldn't have to immediately disconnect to no longer be notified about resources post-onload.

With bufferedTypes, they would write:

new PerformanceObserver(function(list, observer) {
  // process entries here
}).observe({
  bufferedTypes: ['resource']
})

Without:

new PerformanceObserver(function(list, observer) {
  // process entries here
  observer.disconnect()
}).observe({
  entryTypes: ['resource']
})

Also, if blocking entries are all you care about, you wouldn't be able to know if an entry was before or after onload without comparing the timestamps to nav-timing.

This comment has been minimized.

@igrigorik

igrigorik Mar 31, 2017

Member

If we go down this route then we're encouraging developers to create observers that are never disconnected, which is counter to all other observer interfaces. Also, note that the use case you're describing is already covered by querying performance.getEntriesByType within onload event; I don't think we need to invent a new mechanism for this.

The other benefit of a boolean is that you don't have to repeat yourself twice in the constructor. My guess is that in majority of cases the set of entries you care about going forward, and from the past, is the same.

This comment has been minimized.

@cvazac

cvazac Mar 31, 2017

Contributor

I'm fine with making this change, but as a reminder, we talked about PerformanceServerTiming entries not being returned by performance.getEntries() and friends.

This comment has been minimized.

@igrigorik

igrigorik Apr 1, 2017

Member

True. However, if we're queing into the global performance buffer, it only seems natural that these entries are queryiable? In fact, I think that directly falls out of the processing algorithm.

/cc @toddreifsteck

This comment has been minimized.

@cvazac

cvazac Apr 4, 2017

Contributor

With regard to the returned entries from window.performance.getEntries() and those emitted to the PO callback when buffered: true:

@igrigorik are we OK with breaking reverse-compatibility for RT/UT? Or is having a discrepancy between RT/UT and ST more preferred?

This comment has been minimized.

@igrigorik

igrigorik Apr 4, 2017

Member

@cvazac not sure I follow, why would the change we're proposing here break RT/UT?

This comment has been minimized.

@cvazac

cvazac Apr 5, 2017

Contributor

Sorry for not being more clear! :)

-The plan was to stop adding ST entries to the global performance buffer at onload
-Today, RT/UT are collected forever (until RT limit)

-If we keep collecting RT/UT forever, then that will be inconsistent with ST (which may be fine, I don't know)
-If we stop collecting RT/UT at onload, then that will break the polling pattern that devs use to be notified about new entries

cc @igrigorik

This comment has been minimized.

@yoavweiss

yoavweiss Apr 5, 2017

Contributor

FWIW, I understood from previous discussions about exposing ST only through PerfObserver that we intend not to expose it through getEntries*().

If we are exposing it through getEntries() as well, I think we have no choice but to diverge the behavior as @cvazac described, where RT/UT are buffered without time limitations, but ST is not.

FWIW, I think that not exposing it at all through getEntries*() would be less error-prone, as it will be clear that the only way to get ST data is through PerfObserver. Otherwise, developers might erroneously continue to use getEntries() with the same familiar patterns and miss ST entries post onload.

This comment has been minimized.

@igrigorik

igrigorik Apr 5, 2017

Member

Ah, ok, I think I see the confusion. I'll try to unpack..

  1. We should not affect RT/UT processing in any way: all existing entries should be added to the timeline as usual, with no new restrictions on before/after onload.
  2. I'm proposing that we buffer all entry types up until the end of onload event.
    • This doesn't affect current entries like RT/UT, as that's already the case.
    • This does mean that any+all PO-only events are buffered until onload: ST, Long Tasks, and so on.

As a result, entries that have their own buffers are limited by capacity of those buffers, and entries that are queue via PerfObserver are only buffered until end of onload.

Retrieving entries

  • Entries that are queued into global performance timeline can be retrieved via performance.getEntriesBy{Name/Type}. This falls out naturally and works for all types, with only difference that PO-queued entries are only buffered up until end of onload.
  • When a new PO is registered and provides buffer:true, under the hood we pull out all entries of the specified types from the global perf timeline and queue them into the PO observer. In that sense, it's roughly the same as "register and then immediately call getEntries" but avoids race conditions and provides better developer ergonomics.

Is that coherent?

index.html Outdated
<!-- TODO: add long task spec reference -->
and <code>"longtask"</code>.</p>
<p>
The <dfn for="PerformanceEntry">startTime</dfn> attribute MUST return
the time value of the first recorded timestamp of this
performance metric.
performance metric. If the startTime concept doesn't apply, a
performance metric MUST return a `startTime` of <code>0</code>.

This comment has been minimized.

@igrigorik

igrigorik Mar 30, 2017

Member

Why are we adding this here? =/

This comment has been minimized.

@cvazac

cvazac Mar 30, 2017

Contributor

With server-timing, startTime has no meaning. The entry just below about duration specifies that the value can be 0. I was just following the pattern. :)

index.html Outdated
@@ -213,7 +220,7 @@
</li>
</ol>
</li>
<li>For each <i>observer</i> in <i>interested observers</i>:
<li>If the [load event] of the [current document] is not yet completed, for each <i>observer</i> in <i>interested observers</i>:

This comment has been minimized.

@igrigorik

igrigorik Mar 30, 2017

Member

Hmm, wouldn't this mean that we're not queueing any observer events until document is completely loaded?

  • I should be able to register an observer before onload event and observe all events after it
    • Optionally, I should be able to pass in a flag indicating that I also want this observer to consume all buffered events of the specified types.

Conceptually, I think what we want to do here is:

  • Keep existing queue logic as is
  • Add an extra step that...
    • If document is not completely loaded, append the event to the global performance timeline.
  • As part of the register observer algorithm, check for the boolean for buffered events
    • If the boolean is there, retrieve all the events of specified type and append them to the observer buffer

This comment has been minimized.

@cvazac

cvazac Mar 31, 2017

Contributor

My mistake. I will make those edits when we finalize the bufferedTypes part.

@cvazac

This comment has been minimized.

Copy link
Contributor

cvazac commented Mar 30, 2017

oddly, tidy doesn't recognize section as a valid tag and won't format the file

@dontcallmedom

This comment has been minimized.

Copy link
Member

dontcallmedom commented Mar 31, 2017

@yoavweiss IPR status fixed - the tool needed to know @cvazac 's affiliation.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Mar 31, 2017

oddly, tidy doesn't recognize section as a valid tag and won't format the file

Which version of tidy are you using? Mine is 15.18.1.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Apr 3, 2017

@cvazac

This comment has been minimized.

Copy link
Contributor

cvazac commented Apr 3, 2017

re: tidy, I'm running 15.17 (comes with el capitan).

5.4.0 found here creates a super chatty diff.

I will try to get my hands on sierra (15.18.x).

@spanicker

This comment has been minimized.

Copy link

spanicker commented Apr 7, 2017

Could we add a max buffer size recommendation for implementors?
On some sites onload fires very late so the buffer could be arbitrarily large.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Apr 8, 2017

@spanicker how would we define it? As maximum size with respect to all entries, including resource timing, server timing, long tasks, ...? Would we also need "buffer full" notifications, like we have in RT2 and other specs? There's a bunch of corner cases here we'd have to think through. Also, what would you propose as a cap?

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Apr 10, 2017

I don't think a "buffer is full" notification is needed. If you have running script earlier on (that can handle those notifications), you just register an observer and take it from there.

@spanicker

This comment has been minimized.

Copy link

spanicker commented Apr 10, 2017

Agree with Yoav, "buffer is full" doesn't seem critical.
However I do see an issue with:

  • not having a max limit on buffer (this can be arbitrarily large)
  • no defined way to empty the buffer (maybe I missed this -- what would clear the buffer?)
@cvazac

This comment has been minimized.

Copy link
Contributor

cvazac commented Apr 10, 2017

@igrigorik @yoavweiss
high-level for the proposed spec changes:

  • when adding an entry to the global performance timeline, callers will pass in a flag (defaults to true) as to whether the entry should be included in the timeline after the onload event has fired
  • POs registering with buffered: true will be notified about all of the entries in the global performance timeline for each of the specified entryTypes
@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Apr 10, 2017

@cvazac yep, I think that sounds right on both counts. In case of Server Timing the flag will be set to false, which means we'll buffer only up to the end of load event.

@cvazac

This comment has been minimized.

Copy link
Contributor

cvazac commented Apr 13, 2017

@igrigorik this is ready for review.

The only question outstanding is where to put the bits about:

when adding an entry to the global performance timeline, callers will pass in a flag (defaults to true) as to whether the entry should be included in the timeline after the onload event has fired

index.html Outdated
and be notified of new performance metrics via the
<a>PerformanceObserver</a> interface:</p>
and be notified of new performance metrics, and optionally buffered performance
metrics recorded prior to registration, via the

This comment has been minimized.

@igrigorik

igrigorik Apr 18, 2017

Member

"... prior to the end of the document's load event, via the ..."

index.html Outdated
@@ -155,8 +156,11 @@
// maybe disconnect after processing the events.
observer.disconnect();
});
// subscribe to Resource-Timing and User-Timing events
observer.observe({ entryTypes: ["resource", "mark", "measure"] });
observer.observe({

This comment has been minimized.

@igrigorik

igrigorik Apr 18, 2017

Member

Let's keep this simple and stick to Resource Timing and User Timing.. those are uniformly supported by all browsers already.

// retrieve buffered events and subscribe to new events
// for Resource Timing and User Timing
observer.observe({
  entryTypes: ["resource", "mark", "measure"],
  buffered: true
});
index.html Outdated
@@ -329,6 +338,12 @@
that is the <a>context object</a>, replace the <a>context object</a>'s
`options` with <var>options</var>.
</li>
<li>If _options'_ <a for="PerformanceObserverInit">buffered</a>

This comment has been minimized.

@igrigorik

igrigorik Apr 18, 2017

Member

... "If options' buffered flag is set," ...

index.html Outdated
<li>If _options'_ <a for="PerformanceObserverInit">buffered</a>
attribute is present with a value of `true`, for each entryType of
the `entryTypes` sequence, copy the corresponding entries from the
global performance timeline to the newly <a>registered performance

This comment has been minimized.

@igrigorik

igrigorik Apr 18, 2017

Member

copy the corresponding entries from the global performance timeline

We need to spell out the steps here. We're running the same operation as getEntriesByType [1], so we can reuse the language from that. Something like:

  • For each type X in ...
    • Let entries be PerformanceEntryList object returned by 4.2. Filter buffer by name and type algorithm with buffer set to performance entry buffer, name set to null, and type set to X.
    • Append entries to context object's observe buffer.

[1] https://w3c.github.io/performance-timeline/#getentries-method

index.html Outdated
};
</pre>
<dl>
<dt><dfn>entryTypes</dfn></dt>
<dd>A list of entry names to be observed. The list MUST NOT be empty
and types not recognized by the user agent MUST be ignored.</dd>
</dl>
<dl>
<dt><dfn>buffered</dfn></dt>
<dd>A boolean to indicate whether buffered entries should be emitted.</dd>

This comment has been minimized.

@igrigorik

igrigorik Apr 18, 2017

Member

"A flag to indicate whether buffered entries should be queued into observer's buffer."

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Apr 18, 2017

The only question outstanding is where to put the bits about:

when adding an entry to the global performance timeline, callers will pass in a flag (defaults to true) as to whether the entry should be included in the timeline after the onload event has fired

Wait, I'm not sure that works. Let's take Resource Timing as an example:

Step 20: Queue the PerformanceResourceTiming object.
Step 21: Add the PerformanceResourceTiming object to the performance entry buffer.

If queue appends to performance entry buffer by default, then 21 will produce duplicate appends up until end of onload regardless of flag state above. On the other hand—just to make things more fun—in Nav Timing the "add step" comes before the "queue step".

I think we can focus on the "Queue" step and give it an optional flag for "append to the performance entry buffer", which defaults to false. This way RT/NT/UT keep existing behavior without any modifications. In Long Tasks, Server Timing we'll set this flag to true, and processing algorithm will call the "add to performance entry buffer" step there if document's onload has not finished.

Does that work? Somehow I feel we discussed above proposal before, but ruled it out.. I just can't remember why. :)

@cvazac

This comment has been minimized.

Copy link
Contributor

cvazac commented Apr 19, 2017

If I understand you correctly, I think we are fine as-is. If I'm reading Step 20 correctly, that only affects performance observers that are already registered. The bit about copying entries from global into observers, that only happens during observer registration.

index.html Outdated
first recorded timestamp of this performance metric.</dd>
first recorded timestamp of this performance metric. If the startTime
concept doesn't apply, a performance metric may choose to return a
`startTime` of <code>0</code>.</dd></dd>

This comment has been minimized.

@igrigorik

igrigorik Apr 27, 2017

Member

Extra </dd>?

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Apr 27, 2017

@cvazac apologies about the delay.

Observer registration step looks good! The remaining bit is to figure out the "queue" step...

If I understand you correctly, I think we are fine as-is. If I'm reading Step 20 correctly, that only affects performance observers that are already registered.

In RT we call queue for every resource, regardless of whether there are registered observers or not. So the sequence will be: call queue --> appends to observer buffers + performance buffer, call add to performance buffer. That would result in duplicate entries, no?

@cvazac

This comment has been minimized.

Copy link
Contributor

cvazac commented Apr 28, 2017

@igrigorik I think we're finally on the same page, I apologize for the back and forth.

I removed the bits about "before onload" in this spec, because the caller of the "queue" algorithm decides the value of the add flag. Those words will be used in the server timing spec.

@@ -428,8 +426,10 @@
<h2>Processing</h2>
<section data-link-for="PerformanceObserver">
<h2>Queue a <code>PerformanceEntry</code></h2>
<p>To <dfn>queue a PerformanceEntry</dfn> (<i>new entry</i>), run these

This comment has been minimized.

@igrigorik

igrigorik May 2, 2017

Member

We can't remove this, as this formally defines the step that we refer to from other specs.

To queue a PerformanceEntry (new entry) with an optional add to performance entry buffer flag, which is set to false(?) by default, ...

(let's use more descriptive names to make algorithm easier to read)

index.html Outdated
@@ -482,6 +482,9 @@
</li>
</ol>
</li>
<li>If the <var>add</var> flag is set to true, add <i>new entry</i> to

This comment has been minimized.

@igrigorik

igrigorik May 2, 2017

Member

s/is set to true/is set/

index.html Outdated
@@ -482,6 +482,9 @@
</li>
</ol>
</li>
<li>If the <var>add</var> flag is set to true, add <i>new entry</i> to
the <a>performance entry buffer</a>

This comment has been minimized.

@igrigorik

igrigorik May 2, 2017

Member

missing trailing period

index.html Outdated
@@ -285,8 +291,8 @@
"PerformanceObserver">
<h2>The <dfn>PerformanceObserver</dfn> interface</h2>
<p>The <a>PerformanceObserver</a> interface can be used to observe the
<a>Performance Timeline</a> and be notified of new performance entries as
they are recorded.</p>
<a>Performance Timeline</a> to be notified of new performance entries as

This comment has been minimized.

@igrigorik

igrigorik May 2, 2017

Member

s/performance entries/performance metrics/ for consistency?

index.html Outdated
<a>Performance Timeline</a> and be notified of new performance entries as
they are recorded.</p>
<a>Performance Timeline</a> to be notified of new performance entries as
they are recorded, and optionally buffered performance metrics.

This comment has been minimized.

@igrigorik

igrigorik May 2, 2017

Member

I'm struggling with this sentence.. hard to parse.

"and, optionally, previously buffered performance metrics of specified type." =/

@cvazac cvazac changed the title buffer only until `onload`, add references to server-timing, add `bufferedTypes` param `PerformanceObserverInit.buffered` and `add to performance entry buffer` flags May 2, 2017

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented May 3, 2017

Reviewed this with Charles on a quick VC..

  • PerfObserver init adds a new buffered flag that defaults to false; when true it queues buffered entries of specified type(s) into the observer.
  • The "queue" step that external specs call also adds a new "add to performance timeline" flag that defaults to false.
    • In Server Timing, we will set the flag to true if load event has not yet fired.
    • In Resource Timing, no changes needed, the flag is set to false.

I believe above should work; LGTM on my end.


As an aside, we identified a few related improvements we should consider:

  • We should formally define "add" step in Performance Timeline, and update external specs to reference it.
  • We could refactor Resource Timing: remove add step and use the queue + new flag.
@igrigorik
Copy link
Member

igrigorik left a comment

LGTM, let's rebase before merge.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented May 3, 2017

Per discussion on the WG call today, let's get the WPT tests in place before we land? Specifically, we need tests for the new PerfObserver init functionality + queue of buffered metrics.

@cvazac

This comment has been minimized.

Copy link
Contributor

cvazac commented May 5, 2017

test pull request:
web-platform-tests/wpt#5841

igrigorik added a commit to web-platform-tests/wpt that referenced this pull request May 16, 2017

add test for 'PerformanceObserverInit.buffered' (#5841)
* add test for 'PerformanceObserverInit.buffered' per spec update (w3c/performance-timeline#76)

@igrigorik igrigorik requested review from plehegar and toddreifsteck May 16, 2017

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented May 16, 2017

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented May 17, 2017

Merging -- thanks everyone!

@igrigorik igrigorik merged commit b763e5c into w3c:gh-pages May 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ipr PR deemed acceptable.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment