1158731 - Buffer for Performance APIs (Resource Timing, User Timing) should be separeted

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Current implementation of Performance APIs shares the buffer for PerformanceEntryList. The buffer should be separated respectively.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed fix

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.

Comment 2

[Andrea Marchesini [:baku]]

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.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Split performance entry list buffer v2

(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

Comment 4

[Andrea Marchesini [:baku]]

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.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Split performance entry list buffer v3

Carrying over review+.

Addressed all review comments.

:baku, thank you!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97f9e0d535e9

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97f9e0d535e9

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/97f9e0d535e9