Closed
Bug 1158731
Opened 9 years ago
Closed 9 years ago
Buffer for Performance APIs (Resource Timing, User Timing) should be separeted
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(1 file, 2 obsolete files)
23.66 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
Current implementation of Performance APIs shares the buffer for PerformanceEntryList. The buffer should be separated respectively.
Assignee | ||
Comment 1•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eec9ee8fee0 I should note about images used in test. All images are different from images in resource_timing_main_test.html. If the images are the same as the others, either test_resource_timing.html or test_resource_timing_same_image.html fails. The failing test depends on execution order. I don't know the failure reason yet, I've filed bug 1160086 for the issue.
Attachment #8599793 -
Flags: review?(amarchesini)
Comment 2•9 years ago
|
||
Comment on attachment 8599793 [details] [diff] [review] Proposed fix Review of attachment 8599793 [details] [diff] [review]: ----------------------------------------------------------------- Sounds good, but can you check the sorting issue in the spec? Question: do you think it makes sense to split the 'mark' list too? Then submit a new patch with these comments. Thanks! ::: dom/base/nsPerformance.cpp @@ +509,5 @@ > return PerformanceBinding::Wrap(cx, this, aGivenProto); > } > > void > nsPerformance::GetEntries(nsTArray<nsRefPtr<PerformanceEntry> >& retval) remove the extra space here too. @@ +514,5 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > + retval = mResourceEntries; > + retval.AppendElements(mUserEntries); do we have to sort them somehow? What does the spec says about it? @@ +519,5 @@ > } > > void > nsPerformance::GetEntriesByType(const nsAString& entryType, > nsTArray<nsRefPtr<PerformanceEntry> >& retval) maybe also this space. @@ +528,5 @@ > + if (entryType.EqualsLiteral("resource")) { > + retval = mResourceEntries; > + } else if (entryType.EqualsLiteral("mark") || > + entryType.EqualsLiteral("measure")) { > + for (PerformanceEntry* entry : mUserEntries) { if this compiles everywhere I like it. @@ +560,1 @@ > } again, what about the sorting? @@ +678,1 @@ > NS_WARNING("Performance Entry buffer size maximum reached!"); if I remember correctly, we removed this warning. Is your tree up-to-date? @@ +687,5 @@ > } > > void > +nsPerformance::InsertUserEntry(PerformanceEntry* aEntry) > +{ I guess you should still use nsContentUtils::IsUserTimingLoggingEnabled(), right? @@ +769,1 @@ > NS_WARNING("Performance Entry buffer size maximum reached!"); Also this warning should be gone. ::: dom/base/nsPerformance.h @@ +364,5 @@ > nsRefPtr<nsDOMNavigationTiming> mDOMTiming; > nsCOMPtr<nsITimedChannel> mChannel; > nsRefPtr<nsPerformanceTiming> mTiming; > nsRefPtr<nsPerformanceNavigation> mNavigation; > + nsTArray<nsRefPtr<PerformanceEntry> > mResourceEntries; remove the extra space between > > @@ +365,5 @@ > nsCOMPtr<nsITimedChannel> mChannel; > nsRefPtr<nsPerformanceTiming> mTiming; > nsRefPtr<nsPerformanceNavigation> mNavigation; > + nsTArray<nsRefPtr<PerformanceEntry> > mResourceEntries; > + nsTArray<nsRefPtr<PerformanceEntry> > mUserEntries; here too.
Attachment #8599793 -
Flags: review?(amarchesini)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #2) :baku, thank you for your kindly review! > Sounds good, but can you check the sorting issue in the spec? Thank you for pointing it out. I totally missed sorting part in the spec. This patch adds some test cases for sorting issue. > Question: do you think it makes sense to split the 'mark' list too? I don't think so. I think it is reasonable to have a buffer on each entry type. > @@ +678,1 @@ > > NS_WARNING("Performance Entry buffer size maximum reached!"); > > if I remember correctly, we removed this warning. Is your tree up-to-date? The removed warning was in nsPerformance::AddEntry. Two other warnings still remain. This patch also removes the last two. > @@ +687,5 @@ > > } > > > > void > > +nsPerformance::InsertUserEntry(PerformanceEntry* aEntry) > > +{ > > I guess you should still use nsContentUtils::IsUserTimingLoggingEnabled(), > right? Thanks. I removed the check accidentally, and revised. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f8a77ca620c
Attachment #8599793 -
Attachment is obsolete: true
Attachment #8600240 -
Flags: review?(amarchesini)
Comment 4•9 years ago
|
||
Comment on attachment 8600240 [details] [diff] [review] Split performance entry list buffer v2 Review of attachment 8600240 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsPerformance.cpp @@ +676,4 @@ > { > MOZ_ASSERT(aEntry); > + MOZ_ASSERT(mResourceEntries.Length() < mResourceTimingBufferSize); > + if (mResourceEntries.Length() == mResourceTimingBufferSize) { >= ::: dom/tests/mochitest/general/performance_timeline_main_test.html @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ > +--> > + > +<!-- > +--> remove this empty comment. @@ +45,5 @@ > + is(window.performance.getEntriesByType("resource").length, 4, "The number of PerformanceResourceTiming should be 4."); > + is(window.performance.getEntriesByType("mark").length, 2, "The number of PerformanceMark entries should be 2."); > + is(window.performance.getEntriesByType("measure").length, 1, "The number of PerformanceMeasure entries should be 1."); > + > + is(0, receivedBufferFullEvents, "onresourcetimingbufferfull should never be called."); is(receivedBufferFullEvents, 0, ... @@ +65,5 @@ > +} > + > +function secondCheck() { > + is(window.performance.getEntriesByType("resource").length, 5, "The number of PerformanceResourceTiming should be 5."); > + is(1, receivedBufferFullEvents, "onresourcetimingbufferfull should never be called since the last call."); reverse the 2 args.
Attachment #8600240 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Carrying over review+. Addressed all review comments. :baku, thank you!
Attachment #8600240 -
Attachment is obsolete: true
Attachment #8603220 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/97f9e0d535e9
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•