Bug 167546 – Implement PerformanceObserver

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Summary:
Implement PerformanceObserver

https://w3c.github.io/performance-timeline/

This is part of Performance Timeline Level 2 and is intended to be the improved way to get Performance Timeline entries without using a global buffer.

Now that we have User Timing as an experimental feature we can build off of that to implement and test PerformanceObserver.

[Radar WebKit Bug Importer]

<rdar://problem/30247959>

[Joseph Pecoraro]

Created attachment 300017 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 300017 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PerformanceObserver.cpp:49:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 300017 [details]
[PATCH] Proposed Fix

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

Oops, let me upload the diff that doesn't think there are file copies.

> Source/WebCore/page/PerformanceObserver.cpp:72
> +        return Exception { TypeError, ASCIILiteral("entryTypes contained only unsupported types") };

I'm not sure if this wording matches other TypeErrors emitted by the engine. Suggestions welcome!

[Joseph Pecoraro]

Created attachment 300019 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 300019 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PerformanceObserver.cpp:49:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 300019 [details]
[PATCH] Proposed Fix

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

> LayoutTests/ChangeLog:9
> +        * performance-observer/performance-observer-api-expected.txt: Added.

I'm thinking of renaming this to:
LayoutTests/performance-api

Since I'll end up putting more tests in here that aren't explicitly about performance-observers. (performance.now in workers, performance.mark/measure when implementing User Timing 2).

[Ryosuke Niwa]

Comment on attachment 300019 [details]
[PATCH] Proposed Fix

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

r=me assuming you fix PerformanceObserver::deliver or have a good explanation as to it doesn't match the spec text as I read it.

> LayoutTests/performance-observer/performance-observer-api.html:12
> +shouldBeDefined(`PerformanceObserver`);
> +shouldBeDefined(`PerformanceObserver.prototype.observe`);

Can we write these tests using testharness.js? (it's in the same directory).
http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html

That would allow us to upstream these tests to web-platform-tests.

> LayoutTests/performance-observer/performance-observer-exception.html:19
> +let observer = new PerformanceObserver((list) => {
> +    debug("PerformanceObserver callback fired");
> +    throw "EXCEPTION MESSAGE";
> +});

Perhaps we should create another test case calling class constructor, which fails to be called?

> LayoutTests/performance-observer/performance-observer-order.html:53
> +// Register all in order.
> +for (let o of [observer1, observer2, observerSpecial, observer3, observer4, observer5])
> +    o.observe({entryTypes: ["mark"]});

You should test calling observe twice on the same observer, calling observe inside another observer's callback,
and inserting a new entry inside some observer (and how other observers receive them).

> Source/WebCore/ChangeLog:19
> +        This implements PerformanceObserver from Performance Timeline Level 2:
> +        https://w3c.github.io/performance-timeline/
> +

This description should appear before the list of tests.

> Source/WebCore/page/Performance.cpp:255
> +        for (auto& observer : observers)
> +            observer->deliver();

This, and the spec language seems to indicate if new entires were added during a call to some of the observers,
then the subsequent observers would immediately see those entries
while the observer which added the new entry as well as the proceeding observers would observe in the next task.

We should also add a test ensuring that PerformanceObservers are called in the next task, not a microtask.
You can do this by creating a mutation observer, and then make WebCore fire some event
and make sure only MutationObservers are called and not PerformanceObserver.

> Source/WebCore/page/PerformanceObserver.cpp:72
> +    if (init.entryTypes.isEmpty())
> +        return Exception { TypeError, ASCIILiteral("entryTypes contained only unsupported types") };

This is an inefficient way of finding an unsupported type.
Why don't we just have a boolean for finding an unknown type?

> Source/WebCore/page/PerformanceObserver.cpp:93
> +    // Deliver any still remaining entries.
> +    deliver();
> +    ASSERT(m_entriesToDeliver.isEmpty());

https://w3c.github.io/performance-timeline/#dom-performanceobserver-disconnect
The spec simply says to "empty context object's observer buffer.

> Source/WebCore/page/PerformanceObserver.cpp:109
> +    Ref<PerformanceObserverEntryList> list = PerformanceObserverEntryList::create(WTFMove(entries));

Why not auto?

> Source/WebCore/page/PerformanceObserverEntryList.cpp:38
> +    std::sort(m_entries.begin(), m_entries.end(), PerformanceEntry::startTimeCompareLessThan);

Ugh... it's sad that we have to call std::sort given these entires are probably already sorted in the right order.
In fact, we might want to consider doing insertion sort in getEntriesByName or some new helper function
(since it also needs to be called by getEntries).

> Source/WebCore/page/PerformanceObserverEntryList.cpp:55
> +    Vector<RefPtr<PerformanceEntry>> entries;

Why can declare this right above the for-loop instead.

> Source/WebCore/page/PerformanceObserverEntryList.h:46
> +    Vector<RefPtr<PerformanceEntry>> getEntries() const;

We can't use Vector<Ref<PerformanceEntry>>?

[Darin Adler]

Comment on attachment 300019 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/page/Performance.cpp:252
> +        Vector<RefPtr<PerformanceObserver>> observers;

Can this use Vector<Ref> instead?

> Source/WebCore/page/Performance.h:98
> +    void queueEntry(Ref<PerformanceEntry>);

This isn’t really the right type. If we are transferring ownership, then please use the type Ref<PerformanceEntry>&&, if not, please just use PerformanceEntry&. Given that we are just doing copyRef all the time, I think the plain reference is the way to go. Callers shouldn’t have to change anything since Ref converts to a reference (unlike RefPtr that requires a get() to turn it into a pointer).

> Source/WebCore/page/PerformanceEntry.h:62
> +    static Type typeForEntryTypeString(const String& entryType);

I think should call this parseTypeString or parseType, and the function return type should be std::optional<Type>. No need to use the word "entry" in a function that is a member of the class "entry". Also I think we should eliminate Type::Unknown since we don’t need to support that in the OptionSet or store that value in a PerformanceEntry.

> Source/WebCore/page/PerformanceObserver.cpp:62
> +    init.entryTypes.removeAllMatching([&] (String& entryType) {

It’s unnecessarily inefficient to remove strings from the vector just as a way to keep track if they were known types.

We could instead just look and see if the option set is empty after processing the vector, then we would not use removeAllMatching or a lambda here. Just a for loop.

> Source/WebCore/page/PerformanceObserver.cpp:63
> +        PerformanceEntry::Type type = PerformanceEntry::typeForEntryTypeString(entryType);

I think these calls to typeForEntryTypeString would be better with auto.

>> Source/WebCore/page/PerformanceObserver.cpp:93
>> +    // Deliver any still remaining entries.
>> +    deliver();
>> +    ASSERT(m_entriesToDeliver.isEmpty());
> 
> https://w3c.github.io/performance-timeline/#dom-performanceobserver-disconnect
> The spec simply says to "empty context object's observer buffer.

How is this assertion guaranteed to be true? Can’t the event handlers do something that results in adding more entries?

> Source/WebCore/page/PerformanceObserver.cpp:107
> +    m_entriesToDeliver.swap(entries);

Should use move instead of swap for this. Swap was the preferred technique back when we didn’t have move.

>> Source/WebCore/page/PerformanceObserver.cpp:109
>> +    Ref<PerformanceObserverEntryList> list = PerformanceObserverEntryList::create(WTFMove(entries));
> 
> Why not auto?

Would be better with auto.

> Source/WebCore/page/PerformanceObserver.h:59
> +    void queueEntry(Ref<PerformanceEntry>);

Type should be Ref<PerformanceEntry>&&.

> Source/WebCore/page/PerformanceObserver.h:66
> +    Vector<RefPtr<PerformanceEntry>> m_entriesToDeliver;

Should use Vector<Ref> instead of Vector<RefPtr>.

> Source/WebCore/page/PerformanceObserver.idl:44
> +    required sequence<DOMString> entryTypes;

Surprised that this is specified with strings rather than an IDL enumeration.

> Source/WebCore/page/PerformanceObserverCallback.h:40
> +    virtual bool handleEvent(PerformanceObserverEntryList*, PerformanceObserver*) = 0;

Can these arguments be references instead of pointers?

Can we use a WTF::Function instead of a reference counted callback object?

>> Source/WebCore/page/PerformanceObserverEntryList.cpp:38
>> +    std::sort(m_entries.begin(), m_entries.end(), PerformanceEntry::startTimeCompareLessThan);
> 
> Ugh... it's sad that we have to call std::sort given these entires are probably already sorted in the right order.
> In fact, we might want to consider doing insertion sort in getEntriesByName or some new helper function
> (since it also needs to be called by getEntries).

I believe two entries can have the same start time. If so, then this needs to be std::stable_sort instead of std::sort, so we don’t end up with the entries sorted in arbitrary order rather than preserving the order they were passed in. We should have test coverage for this behavior, although it might be challenging to come up with a test case that causes std::sort to reverse the order. std::sort is allowed to shuffle ordering of equal entries, which is why std::stable_sort exists, but it is not required to do that.

> Source/WebCore/page/PerformanceObserverEntryList.cpp:45
> +    Vector<RefPtr<PerformanceEntry>> entries;
> +    entries.appendVector(m_entries);
> +    return entries;

How is this different from just:

    return m_entries;

I don’t think it is. We could also let the caller do the copying by changing the return type of this function to const Vector&. The bindings code would do the copying for us. Or maybe the bindings code could even optimize it out! I think this function should return a const Vector& and be inlined in the header.

> Source/WebCore/page/PerformanceObserverEntryList.cpp:58
> +    // PerformanceObservers can only be registered for valid types.
> +    // So if the incoming entryType is an unknown type, we know there will be no matches.

This comment isn’t quite right. Because we put the switches for runtime enabled features in the parse function, we might build a list with a feature turned on and then use it later with the feature turned off, or something like that. It’s a little messy.

> Source/WebCore/page/PerformanceObserverEntryList.h:41
> +    static Ref<PerformanceObserverEntryList> create(Vector<RefPtr<PerformanceEntry>> entries)

The argument type here should be Vector&& rather than Vector. Also could be Vector<Ref> instead of Vector<RefPtr>.

Also please put this function in the .cpp file and instead of marking this inline, mark the constructor inline. Not great to have this in the header file, and it doesn’t really save function call overhead to inline this at the call site when the constructor is not inlined.

> Source/WebCore/page/PerformanceObserverEntryList.h:48
> +    Vector<RefPtr<PerformanceEntry>> getEntries() const;
> +    Vector<RefPtr<PerformanceEntry>> getEntriesByType(const String& entryType) const;
> +    Vector<RefPtr<PerformanceEntry>> getEntriesByName(const String& name, const String& entryType) const;

Can these return types be Vector<Ref>?

> Source/WebCore/page/PerformanceObserverEntryList.h:51
> +    PerformanceObserverEntryList(Vector<RefPtr<PerformanceEntry>> entries);

Ditto.

> Source/WebCore/page/PerformanceObserverEntryList.h:53
> +    Vector<RefPtr<PerformanceEntry>> m_entries;

Can this type be Vector<Ref>?

> Source/WebCore/page/PerformanceUserTiming.cpp:113
> +    Ref<PerformanceMark> entry = PerformanceMark::create(markName, m_performance.now());

Would prefer to have this use auto.

[Joseph Pecoraro]

Comment on attachment 300019 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/performance-observer/performance-observer-api.html:12
>> +shouldBeDefined(`PerformanceObserver.prototype.observe`);
> 
> Can we write these tests using testharness.js? (it's in the same directory).
> http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html
> 
> That would allow us to upstream these tests to web-platform-tests.

web-platform-tests/performance-observer tests exists. I plan on importing them immediately following this landing. However LayoutTests are earlier to write and are clearer to understand the PASS/FAIL reasons. I've been quite disappointed by the web-platform-tests I've seen so far.

I do plan on writing some web-platform-tests for cases I've found where browsers differ. However, I'm going to stick to LayoutTests for now. They are easy to test in all browsers, do not require running a web-platform-tests server instance, and I find them much clearer.

>> LayoutTests/performance-observer/performance-observer-exception.html:19
>> +});
> 
> Perhaps we should create another test case calling class constructor, which fails to be called?

Like this?

    class MyClass {};
    let observer = new PerformanceObserver(MyClass);

>> LayoutTests/performance-observer/performance-observer-order.html:53
>> +    o.observe({entryTypes: ["mark"]});
> 
> You should test calling observe twice on the same observer, calling observe inside another observer's callback,
> and inserting a new entry inside some observer (and how other observers receive them).

The `performance-observer-api.html` and `performance-observer-nested.html` test covers some of this.

I'll add a test for calling observe in a callback.

>> Source/WebCore/page/Performance.cpp:252
>> +        Vector<RefPtr<PerformanceObserver>> observers;
> 
> Can this use Vector<Ref> instead?

I had very unusual issues with auto/Ref/RefPtr. I'll work on cleaning this up tomorrow. Thanks for all of the pointers.

>> Source/WebCore/page/Performance.cpp:255
>> +            observer->deliver();
> 
> This, and the spec language seems to indicate if new entires were added during a call to some of the observers,
> then the subsequent observers would immediately see those entries
> while the observer which added the new entry as well as the proceeding observers would observe in the next task.
> 
> We should also add a test ensuring that PerformanceObservers are called in the next task, not a microtask.
> You can do this by creating a mutation observer, and then make WebCore fire some event
> and make sure only MutationObservers are called and not PerformanceObserver.

So I think the `performance-observer-nested.html` test covers this.

Also I think I would able to use Promises for microtasks `Promise.resolve().then(() => { /*...*/ }); Unless you specifically want to test comparing the relation to MutationObservers.

>>> Source/WebCore/page/PerformanceObserver.cpp:93
>>> +    ASSERT(m_entriesToDeliver.isEmpty());
>> 
>> https://w3c.github.io/performance-timeline/#dom-performanceobserver-disconnect
>> The spec simply says to "empty context object's observer buffer.
> 
> How is this assertion guaranteed to be true? Can’t the event handlers do something that results in adding more entries?

@rniwa: You are correct. I will change the behavior to match the spec.

I did this to match Blink's behavior. I'll write a platform test and file a bug on them.

I think the case Blink was considering is this:

    observer = new PerformanceObserver(console.log);
    observer.observe({entryTypes:["mark"]});
    performance.mark("mark1");
    observer.disconnect();

The observer observed `mark` before disconnect. So they wanted the Observer's callback to reflect that. However the deliver here is weird (its out of order of registered observers, and its essentially happening during the disconnect call instead of as a separate task). So I'll change behavior 

@darin: We have unregistered the observer by this point. So this observer would not have seen any new entries. That said I'll now just clear the list.

>> Source/WebCore/page/PerformanceObserver.idl:44
>> +    required sequence<DOMString> entryTypes;
> 
> Surprised that this is specified with strings rather than an IDL enumeration.

Each of the different built-in types has its own spec. mark/measure, resource, navigation. Chrome has a few others as well, like "longtask" <https://github.com/WICG/longtasks>. I think they want this list to be easily extensible. I don't think you can extend an enum in separate WebIDL extensions (e.g. a "partial enum").

>> Source/WebCore/page/PerformanceObserverCallback.h:40
>> +    virtual bool handleEvent(PerformanceObserverEntryList*, PerformanceObserver*) = 0;
> 
> Can these arguments be references instead of pointers?
> 
> Can we use a WTF::Function instead of a reference counted callback object?

This is the interface our generator needs for `callback` functions. It can be improved, but it will affect many places, not just here.

>> Source/WebCore/page/PerformanceObserverEntryList.cpp:58
>> +    // So if the incoming entryType is an unknown type, we know there will be no matches.
> 
> This comment isn’t quite right. Because we put the switches for runtime enabled features in the parse function, we might build a list with a feature turned on and then use it later with the feature turned off, or something like that. It’s a little messy.

You are correct. I think this is more of a problem with RuntimeEnabledFeatures. I'm not sure how much sense it would make to work around this here. It is harmless to have this partially broken if the page has a partially enabled/disabled feature.

I think ideally the Page (and all its contexts) would not see the new RuntimeEnabledFeatures values until the next page load. Most RuntimeEnabledFeatures guard exposing APIs which can only happen when the page is constructed.

[Joseph Pecoraro]

Comment on attachment 300019 [details]
[PATCH] Proposed Fix

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

>>> Source/WebCore/page/Performance.cpp:252
>>> +        Vector<RefPtr<PerformanceObserver>> observers;
>> 
>> Can this use Vector<Ref> instead?
> 
> I had very unusual issues with auto/Ref/RefPtr. I'll work on cleaning this up tomorrow. Thanks for all of the pointers.

Using either I use ListHashSet<Ref> or ListHashSet<RefPtr> complains if I try copyToVector with Vector<Ref>. It does work with Vector<RefPtr>. The same issues shows up trying to copy a Vector<Ref> to a Vector<Ref> (Ref<> destructor is deleted).

Is there an existing pattern that works for such copies? I did a quick search and could find no instances of a copyToVector call copying to a Vector<Ref>.

[Darin Adler]

(In reply to comment #11)
> Using either ListHashSet<Ref> or ListHashSet<RefPtr> complains if I
> try copyToVector with Vector<Ref>. It does work with Vector<RefPtr>. The
> same issues shows up trying to copy a Vector<Ref> to a Vector<Ref> (Ref<>
> destructor is deleted).
> 
> Is there an existing pattern that works for such copies? I did a quick
> search and could find no instances of a copyToVector call copying to a
> Vector<Ref>.

It’s fine to stick with Vector<RefPtr> if Vector<Ref> doesn’t work yet.

There are many functions that need some refinements to work properly with Ref.

[Joseph Pecoraro]

Comment on attachment 300019 [details]
[PATCH] Proposed Fix

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

>>> Source/WebCore/page/PerformanceObserverEntryList.cpp:38
>>> +    std::sort(m_entries.begin(), m_entries.end(), PerformanceEntry::startTimeCompareLessThan);
>> 
>> Ugh... it's sad that we have to call std::sort given these entires are probably already sorted in the right order.
>> In fact, we might want to consider doing insertion sort in getEntriesByName or some new helper function
>> (since it also needs to be called by getEntries).
> 
> I believe two entries can have the same start time. If so, then this needs to be std::stable_sort instead of std::sort, so we don’t end up with the entries sorted in arbitrary order rather than preserving the order they were passed in. We should have test coverage for this behavior, although it might be challenging to come up with a test case that causes std::sort to reverse the order. std::sort is allowed to shuffle ordering of equal entries, which is why std::stable_sort exists, but it is not required to do that.

Wow,std::sort/stable_sort also don't work with Vector<Ref> but more insidiously it is not a compile error but runtime crashes. Moves happen in the algorithm that clear out a Ref and cause nullptr dereferences. I'm going to bring this back to RefPtr. Ref in collections is causing me too many issues =(.

[Joseph Pecoraro]

Created attachment 300173 [details]
[PATCH] For Bots

This includes all of the additional tests, and some cleaned up Ref/RefPtr/&& logic.

I've filed a number of issues on the spec and updated web-platform-tests. All links can be found from:
https://github.com/w3c/web-platform-tests/pull/4660

[WebKit Commit Bot]

Attachment 300173 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PerformanceEntry.h:74:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/page/PerformanceObserver.cpp:49:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 300179 [details]
[PATCH] For Bots

[WebKit Commit Bot]

Attachment 300179 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PerformanceObserver.cpp:49:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 300184 [details]
[PATCH] For Bots

[WebKit Commit Bot]

Attachment 300184 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PerformanceObserver.cpp:49:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/211406>