Bug 174235 – [WK2] Use a rolling 30-day uptime for processing statistics

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brent Fulgham]

Switch from using a 30-day calendar date to a rolling set of 30 uptime events before processing load statistics.

[Brent Fulgham]

<rdar://problem/33164381>

[Brent Fulgham]

Created attachment 314810 [details]
Patch

[Brent Fulgham]

Using EWS as my build machine right now.

[Brent Fulgham]

Created attachment 314860 [details]
Patch

[Chris Dumez]

Comment on attachment 314860 [details]
Patch

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

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:532
> +void WebResourceLoadStatisticsStore::handleDailyTasks()

performDailyTasks?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:541
>  void WebResourceLoadStatisticsStore::telemetryTimerFired()

Can we rename to submitTelemetryIfNecessary() for clarity?

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:93
> +    // Running history

Missing period at the end.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:138
> +    // Running history

Missing period at the end.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:315
> +    if (doesExceedActiveWindow(resourceStatistic.mostRecentUserInteractionTime)) {

Would exceedsActiveWindow() work? or hasStatisticsExpired(resourceStatistic) ?

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:426
> +bool ResourceLoadStatisticsStore::isSameDayAsCurrentOperatingDate(const WTF::WallTime& date) const

This is always called with WallTime::now() parameter. Does it really need a parameter?

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:437
> +    m_operatingDates.append(WallTime::now());

How about:
while (m_operatingDates.size() >= operatingDatesWindow)
    m_operatingDates.removeFirst();
m_operatingDates.append(WallTime::now());

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:447
> +bool ResourceLoadStatisticsStore::doesExceedActiveWindow(const WTF::WallTime& date) const

We do not need WTF:: prefix in the cpp. The only reason you need it in the header is because you forward declared.
WallTime is a double, no need to pass by const reference, we should pass it by value everywhere.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:454
> +    // For testing

Missing period at the end. What does this do? Why is this needed for testing?

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:104
> +    bool doesExceedActiveWindow(const WTF::WallTime&) const;

Can be private.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:105
> +    bool isSameDayAsCurrentOperatingDate(const WTF::WallTime&) const;

Could we move this into includeTodayAsOperatingDate(), which we would rename to includeTodayAsOperatingDateIfNecesary() ?

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:114
> +    Vector<WTF::WallTime> m_operatingDates;

This should be a Deque, not a Vector, since you remove from the front.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:115
> +    WTF::WallTime m_earliestRetainedDate;

Why do we need this data member? It seems it is always m_operatingDates.first() ?

[Brent Fulgham]

Comment on attachment 314860 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:541
>>  void WebResourceLoadStatisticsStore::telemetryTimerFired()
> 
> Can we rename to submitTelemetryIfNecessary() for clarity?

Done.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:93
>> +    // Running history
> 
> Missing period at the end.

This comment doesn't really add anything, so I'll remove it.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:138
>> +    // Running history
> 
> Missing period at the end.

I'll remove this comment, too.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:315
>> +    if (doesExceedActiveWindow(resourceStatistic.mostRecentUserInteractionTime)) {
> 
> Would exceedsActiveWindow() work? or hasStatisticsExpired(resourceStatistic) ?

I like "hasStatisticsExpired"

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:426
>> +bool ResourceLoadStatisticsStore::isSameDayAsCurrentOperatingDate(const WTF::WallTime& date) const
> 
> This is always called with WallTime::now() parameter. Does it really need a parameter?

I guess not!

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:437
>> +    m_operatingDates.append(WallTime::now());
> 
> How about:
> while (m_operatingDates.size() >= operatingDatesWindow)
>     m_operatingDates.removeFirst();
> m_operatingDates.append(WallTime::now());

Yes -- that's better.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:447
>> +bool ResourceLoadStatisticsStore::doesExceedActiveWindow(const WTF::WallTime& date) const
> 
> We do not need WTF:: prefix in the cpp. The only reason you need it in the header is because you forward declared.
> WallTime is a double, no need to pass by const reference, we should pass it by value everywhere.

Will do.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:454
>> +    // For testing
> 
> Missing period at the end. What does this do? Why is this needed for testing?

We can set m_timeToLiveUserInteraction to a tighter window than the default, which is helpful for triggering this during testing.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:104
>> +    bool doesExceedActiveWindow(const WTF::WallTime&) const;
> 
> Can be private.

Will do.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:105
>> +    bool isSameDayAsCurrentOperatingDate(const WTF::WallTime&) const;
> 
> Could we move this into includeTodayAsOperatingDate(), which we would rename to includeTodayAsOperatingDateIfNecesary() ?

Yes, I think that works properly. I'll make that change.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:114
>> +    Vector<WTF::WallTime> m_operatingDates;
> 
> This should be a Deque, not a Vector, since you remove from the front.

Yeah, that makes sense. I was a little lazy here and just used Vector, since it encodes/decodes without additional steps.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:115
>> +    WTF::WallTime m_earliestRetainedDate;
> 
> Why do we need this data member? It seems it is always m_operatingDates.first() ?

I thought it would be more efficient to cache this value the one time a day we change it, so that checks against it would be faster. Maybe there is no meaningful per difference with a dequeue first() dereference?

[Chris Dumez]

Comment on attachment 314860 [details]
Patch

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

>>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:454
>>> +    // For testing
>> 
>> Missing period at the end. What does this do? Why is this needed for testing?
> 
> We can set m_timeToLiveUserInteraction to a tighter window than the default, which is helpful for triggering this during testing.

Yes, I would just include this in the comment.

>>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:115
>>> +    WTF::WallTime m_earliestRetainedDate;
>> 
>> Why do we need this data member? It seems it is always m_operatingDates.first() ?
> 
> I thought it would be more efficient to cache this value the one time a day we change it, so that checks against it would be faster. Maybe there is no meaningful per difference with a dequeue first() dereference?

I wouldn't think it is worth it to optimize a Deque::first() access, although you'll need a branch to check if the Deque is empty too.

[Brent Fulgham]

Created attachment 314875 [details]
Patch

[Brent Fulgham]

Comment on attachment 314875 [details]
Patch

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

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:44
> +const Seconds lengthOfDay { 30_s };

Whoops! This shouldn't stay like this.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:152
> +        m_operatingDates.append(WTFMove(date));

Chris: Is there a more efficient way to populate a Deque from a Vector?

[Brent Fulgham]

Comment on attachment 314860 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:115
>>>> +    WTF::WallTime m_earliestRetainedDate;
>>> 
>>> Why do we need this data member? It seems it is always m_operatingDates.first() ?
>> 
>> I thought it would be more efficient to cache this value the one time a day we change it, so that checks against it would be faster. Maybe there is no meaningful per difference with a dequeue first() dereference?
> 
> I wouldn't think it is worth it to optimize a Deque::first() access, although you'll need a branch to check if the Deque is empty too.

Actually, the only place I am using this is right after a size check, so it's pretty silly to bother trying to cache this value. I'll switch to m_earliestRetainedDate.first() instead and get rid of this member.

[Brent Fulgham]

Comment on attachment 314875 [details]
Patch

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

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:445
> +        if (date < m_earliestRetainedDate)

I'll change this to m_earliestRetainedDate.first()

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:115
> +    WTF::WallTime m_earliestRetainedDate;

I'll get rid of the m_earliestRetainedDate.

[Chris Dumez]

Comment on attachment 314875 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:443
> +    store->setTimeToLiveUserInteraction(0_s);

Is this intentional? Why are we doing this?
Also, this needs to match the default value of the m_timeToLiveUserInteraction member in ResourceLoadStatistics.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:44
> +const Seconds lengthOfDay { 30_s };

lengthOfDay is 30 seconds?

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:139
> +    succeeded = decoder.decodeObjects("operatingDates", dates, [](KeyedDecoder& decoder, WallTime& wallTime) {

Could we update decodeObjects() so that the second parameter is a template parameter? Its code should build for either Vector and Deque since both support append().

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:318
> +    if (hasStatisticsExpired(resourceStatistic.mostRecentUserInteractionTime)) {

Given the name I suggested, my suggestion was to take resourceStatistic in parameter, not resourceStatistic.mostRecentUserInteractionTime.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:434
> +    if (!m_operatingDates.isEmpty() && (WallTime::now() - m_operatingDates.last() < lengthOfDay))

Should this be *before* the removal? Also, I do not know what this check is about yet.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:439
> +    m_earliestRetainedDate = m_operatingDates.first();

Why is this still here?

[Brent Fulgham]

Created attachment 314880 [details]
Patch

[Brent Fulgham]

Comment on attachment 314880 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:443
> +    store->setTimeToLiveUserInteraction(0_s);

It seems weird to have these, if they just set the store to its defaults. Couldn't that be a method on the store, that just sets them back to defaults, rather than requiring the knowledge in two places?

[Chris Dumez]

(In reply to Brent Fulgham from comment #14)
> Comment on attachment 314880 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314880&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:443
> > +    store->setTimeToLiveUserInteraction(0_s);
> 
> It seems weird to have these, if they just set the store to its defaults.
> Couldn't that be a method on the store, that just sets them back to
> defaults, rather than requiring the knowledge in two places?

I totally agree. I just have got around to refactor this part yet. It is on my TODO list though.

[Brent Fulgham]

Created attachment 314882 [details]
Patch

[Chris Dumez]

Comment on attachment 314882 [details]
Patch

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

> Source/WebCore/platform/KeyedCoding.h:130
> +    bool decodeObjects(const String& key, Deque<T>& objects, F&& function)

This seems like an exact copy paste of the method above. Why didn't you use a template parameter for the objects parameter type as I suggested? I think this would avoid the duplication.

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:430
> +    if (!m_operatingDates.isEmpty() && (WallTime::now() - m_operatingDates.last() < 24_h))

Shouldn't this happen before we remove? Otherwise, we'll possibly end up removing a date but not adding one.

[Brent Fulgham]

Created attachment 314883 [details]
Patch

[Brent Fulgham]

Comment on attachment 314882 [details]
Patch

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

>> Source/WebCore/platform/KeyedCoding.h:130
>> +    bool decodeObjects(const String& key, Deque<T>& objects, F&& function)
> 
> This seems like an exact copy paste of the method above. Why didn't you use a template parameter for the objects parameter type as I suggested? I think this would avoid the duplication.

I'm trying to get the template/template syntax right, and I didn't want to risk breaking Vector uses. I'm uploading a new patch.

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:430
>> +    if (!m_operatingDates.isEmpty() && (WallTime::now() - m_operatingDates.last() < 24_h))
> 
> Shouldn't this happen before we remove? Otherwise, we'll possibly end up removing a date but not adding one.

But we might enter this routine a second time during the same 24-hour period. If the m_operatingDates queue is empty, we add the current date. If it's not empty, and the dates are for the same day, then we don't want to add.

I think this is correct as written.

[Brent Fulgham]

Comment on attachment 314882 [details]
Patch

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

>>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:430
>>> +    if (!m_operatingDates.isEmpty() && (WallTime::now() - m_operatingDates.last() < 24_h))
>> 
>> Shouldn't this happen before we remove? Otherwise, we'll possibly end up removing a date but not adding one.
> 
> But we might enter this routine a second time during the same 24-hour period. If the m_operatingDates queue is empty, we add the current date. If it's not empty, and the dates are for the same day, then we don't want to add.
> 
> I think this is correct as written.

I mean is that it seems okay to remove dates if we are already outside the bounds of our allowed operating window. Even if the current date is the same day as the last time stamp, it's okay to throw away the dates that are outside the allowed range of 'operatingDatesWindow' count.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #20)
> Comment on attachment 314882 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314882&action=review
> 
> >>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:430
> >>> +    if (!m_operatingDates.isEmpty() && (WallTime::now() - m_operatingDates.last() < 24_h))
> >> 
> >> Shouldn't this happen before we remove? Otherwise, we'll possibly end up removing a date but not adding one.
> > 
> > But we might enter this routine a second time during the same 24-hour period. If the m_operatingDates queue is empty, we add the current date. If it's not empty, and the dates are for the same day, then we don't want to add.
> > 
> > I think this is correct as written.
> 
> I mean is that it seems okay to remove dates if we are already outside the
> bounds of our allowed operating window. Even if the current date is the same
> day as the last time stamp, it's okay to throw away the dates that are
> outside the allowed range of 'operatingDatesWindow' count.

I spoke to Chris offline, and agree that we might fall below the window if we are right on the edge of a day transition.

[Brent Fulgham]

Created attachment 314887 [details]
Patch

[Chris Dumez]

Comment on attachment 314887 [details]
Patch

r=me if the bots are happy.

[WebKit Commit Bot]

Comment on attachment 314887 [details]
Patch

Clearing flags on attachment: 314887

Committed r219274: <http://trac.webkit.org/changeset/219274>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This caused http/tests/loading/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout.html to time out.

[Chris Dumez]

Comment on attachment 314887 [details]
Patch

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

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.cpp:445
> +    if (m_timeToLiveUserInteraction) {

This is bad because the test (http/tests/loading/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout.html) sets it to 0.

[Chris Dumez]

Reopening because test failure needs to be addressed.

[Chris Dumez]

Created attachment 314913 [details]
Follow-up fix

[Brent Fulgham]

Comment on attachment 314913 [details]
Follow-up fix

Thank you for fixing this!

[Chris Dumez]

Comment on attachment 314913 [details]
Follow-up fix

Clearing flags on attachment: 314913

Committed r219275: <http://trac.webkit.org/changeset/219275>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.