This is needed for tests such as: resource-timing/buffered-flag.any.html user-timing/buffered-flag.any.html
Created attachment 405908 [details] Patch
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?
Created attachment 405917 [details] Patch
<rdar://problem/66529055>
Created attachment 405992 [details] Patch
Created attachment 405995 [details] Patch
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 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 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 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.
Created attachment 406202 [details] Patch
(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.
Committed r265390: <https://trac.webkit.org/changeset/265390> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406202 [details].
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.
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); }
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); }
(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.
Reopening to attach new patch.
Created attachment 406844 [details] Patch
Created attachment 406847 [details] Patch
Committed r265869: <https://trac.webkit.org/changeset/265869> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406847 [details].