Bug 214883 – Implement PerfomanceObserverInit.buffered

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rob Buis]

This is needed for tests such as:
resource-timing/buffered-flag.any.html
user-timing/buffered-flag.any.html

[Rob Buis]

Created attachment 405908 [details]
Patch

[youenn fablet]

Comment on attachment 405908 [details]
Patch

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

> Source/WebCore/page/Performance.cpp:187
> +    Vector<RefPtr<PerformanceEntry>> entries;

Might be best done as a follow-up but we should most probably use Vector<Ref<>> all over the place, right?

> Source/WebCore/page/Performance.cpp:190
> +        entries.appendVector(m_resourceTimingBuffer);

We could try to optimise this case. Maybe something like:
entries = m_resourceTimingBuffer;

> Source/WebCore/page/Performance.cpp:193
> +        if (entryType.isNull() || equalLettersIgnoringASCIICase(entryType, "mark"))

We are doing case sensitive comparison in other parts of the code, does it make sense to not do so here?
Either way, can we add some related tests if this is not covered?

> Source/WebCore/page/Performance.cpp:199
> +    std::sort(entries.begin(), entries.end(), PerformanceEntry::startTimeCompareLessThan);

Not needed since this is done at call site.

> Source/WebCore/page/PerformanceObserver.cpp:83
> +            m_entriesToDeliver.appendVector(m_performance->getBufferedEntriesByType(*init.type));

Since we are appending a vector, maybe we should write it as:
m_performance->appendBufferedEntriesByType(*init.type, m_entriesToDeliver).

> Source/WebCore/page/PerformanceObserver.cpp:84
> +            std::sort(m_entriesToDeliver.begin(), m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);

We could sort only if new entries are added. Or maybe we can do the sorting only when delivering the entries, in PerformanceObserver::deliver?

[Rob Buis]

Created attachment 405917 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/66529055>

[Rob Buis]

Created attachment 405992 [details]
Patch

[Rob Buis]

Created attachment 405995 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Comment on attachment 405908 [details]
Patch

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

>> Source/WebCore/page/Performance.cpp:187
>> +    Vector<RefPtr<PerformanceEntry>> entries;
> 
> Might be best done as a follow-up but we should most probably use Vector<Ref<>> all over the place, right?

Right.

>> Source/WebCore/page/Performance.cpp:190
>> +        entries.appendVector(m_resourceTimingBuffer);
> 
> We could try to optimise this case. Maybe something like:
> entries = m_resourceTimingBuffer;

This as not needed because of the appendBufferedEntriesByType suggestion.

>> Source/WebCore/page/Performance.cpp:193
>> +        if (entryType.isNull() || equalLettersIgnoringASCIICase(entryType, "mark"))
> 
> We are doing case sensitive comparison in other parts of the code, does it make sense to not do so here?
> Either way, can we add some related tests if this is not covered?

Yes, I erroneously used code that was in place before my other patch. I created https://github.com/web-platform-tests/wpt/pull/24891 for the tests.

>> Source/WebCore/page/Performance.cpp:199
>> +    std::sort(entries.begin(), entries.end(), PerformanceEntry::startTimeCompareLessThan);
> 
> Not needed since this is done at call site.

Done.

>> Source/WebCore/page/PerformanceObserver.cpp:83
>> +            m_entriesToDeliver.appendVector(m_performance->getBufferedEntriesByType(*init.type));
> 
> Since we are appending a vector, maybe we should write it as:
> m_performance->appendBufferedEntriesByType(*init.type, m_entriesToDeliver).

Done.

>> Source/WebCore/page/PerformanceObserver.cpp:84
>> +            std::sort(m_entriesToDeliver.begin(), m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);
> 
> We could sort only if new entries are added. Or maybe we can do the sorting only when delivering the entries, in PerformanceObserver::deliver?

I think it is clearer to always be in sorted state before calling PerformanceObserver::deliver. Fixed.

[Darin Adler]

Comment on attachment 405995 [details]
Patch

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

> Source/WebCore/page/PerformanceObserver.cpp:90
> +                std::sort(m_entriesToDeliver.begin(), m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);

Should this be std::stable_sort? Can two entries have the exact same start time?

[Darin Adler]

Comment on attachment 405995 [details]
Patch

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

>> Source/WebCore/page/PerformanceObserver.cpp:90
>> +                std::sort(m_entriesToDeliver.begin(), m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);
> 
> Should this be std::stable_sort? Can two entries have the exact same start time?

Generally appending and re-sorting is not the optimal way to keep a vector sorted; instead we’d like a sort that understands an existing set that are sorted and a new set that need to be sorted in. Not sure what the correct algorithm is.

[Rob Buis]

Created attachment 406202 [details]
Patch

[Rob Buis]

(In reply to Darin Adler from comment #9)
> Comment on attachment 405995 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405995&action=review
> 
> > Source/WebCore/page/PerformanceObserver.cpp:90
> > +                std::sort(m_entriesToDeliver.begin(), m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);
> 
> Should this be std::stable_sort? Can two entries have the exact same start
> time?

You are right, using that will keep the original order of events with same start time, fixed.

[EWS]

Committed r265390: <https://trac.webkit.org/changeset/265390>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406202 [details].

[Darin Adler]

Comment on attachment 406202 [details]
Patch

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

> Source/WebCore/page/PerformanceObserver.cpp:90
> +            if (m_performance->appendBufferedEntriesByType(*init.type, m_entriesToDeliver))
> +                std::stable_sort(m_entriesToDeliver.begin(), m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);

I suspected there was a better way to do this that does not re-sort everything, and here it is:

    auto oldSize = m_entriesToDeliver.size();
    m_performance->appendBufferedEntriesByType(*init.type, m_entriesToDeliver)
    if (oldSize < m_entriesToDeliver.size()) {
        auto oldEnd = m_entriesToDeliver.begin() + oldSize;
        std::stable_sort(oldEnd, m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);
        std::inplace_merge(m_entriesToDeliver.begin(), oldEnd,  m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);
    }

Can also get rid of the return value from appendBufferedEntriesByType if we do this.

Let me know if you’d like to make this change or if you’d prefer that I did it.

[Darin Adler]

Alternate:

    auto oldSize = m_entriesToDeliver.size();
    m_performance->appendBufferedEntriesByType(*init.type, m_entriesToDeliver)
    auto begin = m_entriesToDeliver.begin();
    auto oldEnd = m_entriesToDeliver.begin() + oldSize;
    auto end = m_entriesToDeliver.end();
    if (oldEnd != end) {
        std::stable_sort(oldEnd, end, PerformanceEntry::startTimeCompareLessThan);
        std::inplace_merge(begin, oldEnd, end, PerformanceEntry::startTimeCompareLessThan);
    }

[Darin Adler]

Alternate:

    auto oldSize = m_entriesToDeliver.size();
    m_performance->appendBufferedEntriesByType(*init.type, m_entriesToDeliver)
    auto begin = m_entriesToDeliver.begin();
    auto oldEnd = begin + oldSize;
    auto end = m_entriesToDeliver.end();
    if (oldEnd != end) {
        std::stable_sort(oldEnd, end, PerformanceEntry::startTimeCompareLessThan);
        std::inplace_merge(begin, oldEnd, end, PerformanceEntry::startTimeCompareLessThan);
    }

[Rob Buis]

(In reply to Darin Adler from comment #14)
> Comment on attachment 406202 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406202&action=review
> 
> > Source/WebCore/page/PerformanceObserver.cpp:90
> > +            if (m_performance->appendBufferedEntriesByType(*init.type, m_entriesToDeliver))
> > +                std::stable_sort(m_entriesToDeliver.begin(), m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);
> 
> I suspected there was a better way to do this that does not re-sort
> everything, and here it is:
> 
>     auto oldSize = m_entriesToDeliver.size();
>     m_performance->appendBufferedEntriesByType(*init.type,
> m_entriesToDeliver)
>     if (oldSize < m_entriesToDeliver.size()) {
>         auto oldEnd = m_entriesToDeliver.begin() + oldSize;
>         std::stable_sort(oldEnd, m_entriesToDeliver.end(),
> PerformanceEntry::startTimeCompareLessThan);
>         std::inplace_merge(m_entriesToDeliver.begin(), oldEnd, 
> m_entriesToDeliver.end(), PerformanceEntry::startTimeCompareLessThan);
>     }
> 
> Can also get rid of the return value from appendBufferedEntriesByType if we
> do this.
> 
> Let me know if you’d like to make this change or if you’d prefer that I did
> it.

Nice! Sure, go ahead.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 406844 [details]
Patch

[Rob Buis]

Created attachment 406847 [details]
Patch

[EWS]

Committed r265869: <https://trac.webkit.org/changeset/265869>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406847 [details].