Currently the ResourceLoadStatistics code writes its data file whenever it changes state. This causes it to make frequent writes to disk, which uses CPU and energy. Instead, we should batch writes so they only happen on a set interval.
<rdar://problem/33115894>
Created attachment 314652 [details] Patch
Comment on attachment 314652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314652&action=review > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:62 > +constexpr Seconds timeBetweenSyncs = Seconds::fromMinutes(5); constexpr Seconds timeBetweenSyncs { 5_min }; > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:174 > + if (WallTime::now() - m_lastStatisticsFileSyncTime < timeBetweenSyncs) m_lastStatisticsFileSyncTime may be a *read* time, right? If so, should we really use this for this purpose?
Comment on attachment 314652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314652&action=review >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:174 >> + if (WallTime::now() - m_lastStatisticsFileSyncTime < timeBetweenSyncs) > > m_lastStatisticsFileSyncTime may be a *read* time, right? If so, should we really use this for this purpose? If not, I would suggest having a lastWriteTime and use MonotonicTime for this new one.
Comment on attachment 314652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314652&action=review >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:62 >> +constexpr Seconds timeBetweenSyncs = Seconds::fromMinutes(5); > > constexpr Seconds timeBetweenSyncs { 5_min }; Cool! >>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:174 >>> + if (WallTime::now() - m_lastStatisticsFileSyncTime < timeBetweenSyncs) >> >> m_lastStatisticsFileSyncTime may be a *read* time, right? If so, should we really use this for this purpose? > > If not, I would suggest having a lastWriteTime and use MonotonicTime for this new one. Yes -- I'll add an additional Monotonic timestamp and check that before writing.
Created attachment 314668 [details] Patch
Comment on attachment 314668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314668&action=review General question about the design, is this OK to ignore a write request when it is too early *without* scheduling a write? We're basically relying on the fact that something later we cause a write to occur again so that the current data gets dumped to disk. In the network cache, we use a timer to schedule the write to avoid this problem. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:63 > +constexpr Seconds timeBetweenSyncs { 5_min }; minimumStatisticsFileWriteInterval ? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:175 > + if (MonotonicTime::now() - m_lastWriteTime < timeBetweenSyncs) Wouldn't it make more sense to have this check in writeStoreToDisk() or is there some reason we do not want to throttle the other call site of writeStoreToDisk() ? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:152 > + WTF::MonotonicTime m_lastWriteTime; For consistency with the other member, I would call this: m_lastStatisticsWriteTime.
Comment on attachment 314668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314668&action=review >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:63 >> +constexpr Seconds timeBetweenSyncs { 5_min }; > > minimumStatisticsFileWriteInterval ? Sure. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:175 >> + if (MonotonicTime::now() - m_lastWriteTime < timeBetweenSyncs) > > Wouldn't it make more sense to have this check in writeStoreToDisk() or is there some reason we do not want to throttle the other call site of writeStoreToDisk() ? Sure -- that's a good idea. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:152 >> + WTF::MonotonicTime m_lastWriteTime; > > For consistency with the other member, I would call this: m_lastStatisticsWriteTime. Will do.
Created attachment 314726 [details] Patch
Comment on attachment 314726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314726&action=review r=me with changes. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:40 > +#include <WebCore/Timer.h> This does not look needed. Also, we should never be using a WebCore Timer in the UIProcess. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:376 > + Seconds delayUntil = minimumStatisticsFileWriteInterval - timeSinceLastWrite + 1_s; Why the + 1_s ? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:377 > + m_statisticsQueue->dispatchAfter(delayUntil, [this] { I would capture protectedThis here. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:380 > + m_doesHaveDelayedWrite = true; Can we move this before the dispatch just to nbc on the safe side? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:398 > + m_doesHaveDelayedWrite = false; What would you think of m_didScheduleWrite ?
Comment on attachment 314726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314726&action=review > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:64 > +constexpr Seconds minimumStatisticsFileWriteInterval { 5_min }; Does this mean that if someone opens the browser, does some things, and closes it in less than 5 min, then it won't be written to disk?
Comment on attachment 314726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314726&action=review >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:64 >> +constexpr Seconds minimumStatisticsFileWriteInterval { 5_min }; > > Does this mean that if someone opens the browser, does some things, and closes it in less than 5 min, then it won't be written to disk? This is a fair point. WebResourceLoadStatisticsStore::applicationWillTerminate() is supposed to take care of this. However, we cannot really afford to wait for up to 5 minutes in WebResourceLoadStatisticsStore::applicationWillTerminate(). We should probably add some logic in WebResourceLoadStatisticsStore::applicationWillTerminate() to dump to disk if m_doesHaveDelayedWrite is true.
Comment on attachment 314726 [details] Patch reversing r+ as I think we should address Alex's feedback in this patch.
Comment on attachment 314726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314726&action=review >>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:64 >>> +constexpr Seconds minimumStatisticsFileWriteInterval { 5_min }; >> >> Does this mean that if someone opens the browser, does some things, and closes it in less than 5 min, then it won't be written to disk? > > This is a fair point. WebResourceLoadStatisticsStore::applicationWillTerminate() is supposed to take care of this. However, we cannot really afford to wait for up to 5 minutes in WebResourceLoadStatisticsStore::applicationWillTerminate(). We should probably add some logic in WebResourceLoadStatisticsStore::applicationWillTerminate() to dump to disk if m_doesHaveDelayedWrite is true. Yes -- putting the 5 minute check only in the cases driven by the statistics processing loop might be better. That way, 'applicationWillTerminate' does the right thing when we shut down. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:176 > writeStoreToDisk(); ... so I think we should check before this 'writeStoreToDisk'... > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:223 > writeStoreToDisk(); ... and before this 'writeStoreToDisk' (or just get rid of it, because this doesn't seem to be used anywhere). >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:376 >> + Seconds delayUntil = minimumStatisticsFileWriteInterval - timeSinceLastWrite + 1_s; > > Why the + 1_s ? I was worried about the math calculating a value just under 5 minutes, and not meeting the 5 minute criteria when this delayed write fired (e.g., we *just* missed the last 5 minute mark, and might land just under the 5 minute mark at the next write attempt.) >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:380 >> + m_doesHaveDelayedWrite = true; > > Can we move this before the dispatch just to nbc on the safe side? Yes. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:398 >> + m_doesHaveDelayedWrite = false; > > What would you think of m_didScheduleWrite ? Sure -- that's fine.
Created attachment 314758 [details] Patch
I did testing with the five-minute delay timer, and found that terminating the process did kill the pending queue, or at least the presence of the pending block did not prevent application termination. Some (silly) logging below: >>>>>> I'm not gonna save for another 278.128 seconds. ###### I'm terminating, but I'm still supposed to wait another 277.429 seconds. !!!!!!! Totally terminating, with another 277.429 seconds to go. But I'm writing anyway and exiting.
Comment on attachment 314758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314758&action=review > Source/WebKit2/ChangeLog:14 > + knowledge in multiple places. I should also mention here that the termination handler was not generating a write, so I fixed that as well.
Comment on attachment 314758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314758&action=review r=me > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:316 > + writeStoreToDisk(); Shouldn't we only do this only if there is a pending write? (i.e. if m_didScheduleWrite is true) ? > Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:-110 > - WTF::Function<void()> m_writePersistentStoreHandler; This was completely unused? :S
Comment on attachment 314758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314758&action=review >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:316 >> + writeStoreToDisk(); > > Shouldn't we only do this only if there is a pending write? (i.e. if m_didScheduleWrite is true) ? That's a good point. I'll adjust that before landing. No point wasting a write on exit if we don't need it. >> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsStore.h:-110 >> - WTF::Function<void()> m_writePersistentStoreHandler; > > This was completely unused? :S I know. :-(
Committed r219220: <http://trac.webkit.org/changeset/219220>