Switch from using a 30-day calendar date to a rolling set of 30 uptime events before processing load statistics.
<rdar://problem/33164381>
Created attachment 314810 [details] Patch
Using EWS as my build machine right now.
Created attachment 314860 [details] Patch
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() ?
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?
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.
Created attachment 314875 [details] Patch
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?
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.
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.
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?
Created attachment 314880 [details] Patch
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?
(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.
Created attachment 314882 [details] Patch
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.
Created attachment 314883 [details] Patch
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.
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.
(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.
Created attachment 314887 [details] Patch
Comment on attachment 314887 [details] Patch r=me if the bots are happy.
Comment on attachment 314887 [details] Patch Clearing flags on attachment: 314887 Committed r219274: <http://trac.webkit.org/changeset/219274>
All reviewed patches have been landed. Closing bug.
This caused http/tests/loading/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout.html to time out.
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.
Reopening because test failure needs to be addressed.
Created attachment 314913 [details] Follow-up fix
Comment on attachment 314913 [details] Follow-up fix Thank you for fixing this!
Comment on attachment 314913 [details] Follow-up fix Clearing flags on attachment: 314913 Committed r219275: <http://trac.webkit.org/changeset/219275>