Bug 214883 - Implement PerfomanceObserverInit.buffered
Summary: Implement PerfomanceObserverInit.buffered
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-28 10:14 PDT by Rob Buis
Modified: 2020-08-19 10:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.12 KB, patch)
2020-08-04 01:51 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.99 KB, patch)
2020-08-04 06:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.17 KB, patch)
2020-08-05 02:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (16.17 KB, patch)
2020-08-05 06:18 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (16.19 KB, patch)
2020-08-07 13:03 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.23 KB, patch)
2020-08-19 08:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.26 KB, patch)
2020-08-19 09:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2020-07-28 10:14:42 PDT
This is needed for tests such as:
resource-timing/buffered-flag.any.html
user-timing/buffered-flag.any.html
Comment 1 Rob Buis 2020-08-04 01:51:10 PDT
Created attachment 405908 [details]
Patch
Comment 2 youenn fablet 2020-08-04 05:01:52 PDT
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?
Comment 3 Rob Buis 2020-08-04 06:45:18 PDT
Created attachment 405917 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2020-08-04 10:15:17 PDT
<rdar://problem/66529055>
Comment 5 Rob Buis 2020-08-05 02:50:50 PDT
Created attachment 405992 [details]
Patch
Comment 6 Rob Buis 2020-08-05 06:18:26 PDT
Created attachment 405995 [details]
Patch
Comment 7 EWS Watchlist 2020-08-05 06:19:08 PDT
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
Comment 8 Rob Buis 2020-08-05 07:42:55 PDT
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.
Comment 9 Darin Adler 2020-08-05 23:04:11 PDT
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?
Comment 10 Darin Adler 2020-08-05 23:06:54 PDT
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.
Comment 11 Rob Buis 2020-08-07 13:03:40 PDT
Created attachment 406202 [details]
Patch
Comment 12 Rob Buis 2020-08-07 13:54:37 PDT
(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.
Comment 13 EWS 2020-08-07 14:23:13 PDT
Committed r265390: <https://trac.webkit.org/changeset/265390>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406202 [details].
Comment 14 Darin Adler 2020-08-07 15:08:59 PDT
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.
Comment 15 Darin Adler 2020-08-07 15:12:34 PDT Comment hidden (obsolete)
Comment 16 Darin Adler 2020-08-07 15:13:00 PDT
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);
    }
Comment 17 Rob Buis 2020-08-08 06:45:28 PDT
(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.
Comment 18 Rob Buis 2020-08-19 08:32:38 PDT
Reopening to attach new patch.
Comment 19 Rob Buis 2020-08-19 08:32:42 PDT
Created attachment 406844 [details]
Patch
Comment 20 Rob Buis 2020-08-19 09:22:16 PDT
Created attachment 406847 [details]
Patch
Comment 21 EWS 2020-08-19 10:07:42 PDT
Committed r265869: <https://trac.webkit.org/changeset/265869>

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