Bug 174111 – [WK2] ResourceLoadStatistics should batch its writes

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/33115894>

[Brent Fulgham]

Created attachment 314652 [details]
Patch

[Chris Dumez]

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?

[Chris Dumez]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 314668 [details]
Patch

[Chris Dumez]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 314726 [details]
Patch

[Chris Dumez]

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 ?

[Alex Christensen]

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?

[Chris Dumez]

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.

[Chris Dumez]

Comment on attachment 314726 [details]
Patch

reversing r+ as I think we should address Alex's feedback in this patch.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 314758 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Chris Dumez]

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

[Brent Fulgham]

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. :-(

[Brent Fulgham]

Committed r219220: <http://trac.webkit.org/changeset/219220>