Bug 174487 – Better deal with changes to the ResourceLoadStatisticsStore file on disk

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

[Chris Dumez]

Better deal with changes to the ResourceLoadStatisticsStore file on disk. We should merge the data on disk with the one we have in memory instead of merely replacing our in-memory data. This avoids data loss when re-syncing from disk.

[Chris Dumez]

Created attachment 315396 [details]
Patch

[Chris Dumez]

Created attachment 315398 [details]
Patch

[Brent Fulgham]

Comment on attachment 315398 [details]
Patch

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

Looks good! R=me

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:130
> +    int m_monthDay { 0 }; // [1, 31].

Is this really 1-based? If we are, why is t it initialized with a 1?

[Chris Dumez]

Comment on attachment 315398 [details]
Patch

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

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:130
>> +    int m_monthDay { 0 }; // [1, 31].
> 
> Is this really 1-based? If we are, why is t it initialized with a 1?

Yes, dayInMonthFromDayInYear() returns a value that is 1-based. The default constructor does not construct a valid date so I do not think it matters that we initialize all members to 0 though. The default constructor is only used for the KeyedDecoding and we would not use the date if the decoding failed.

[WebKit Commit Bot]

Comment on attachment 315398 [details]
Patch

Clearing flags on attachment: 315398

Committed r219499: <http://trac.webkit.org/changeset/219499>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 315398 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:141
> +    std::merge(existingDates.begin(), existingDates.end(), newDates.begin(), newDates.end(), mergedDates.begin());

I’d be tempted to assert std::is_sorted before calling std::merge.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:147
> +    // Remove duplicate dates.
> +    removeRepeatedElements(mergedDates);
> +
> +    // Drop old dates until the Vector size reaches operatingDatesWindow.
> +    while (mergedDates.size() > operatingDatesWindow)
> +        mergedDates.remove(0);

(When researching this, I discovered that Vector::removeRepeatedElements calls resize instead of calling shrink. It should call shrink.)

This code is unnecessarily inefficient. We should resize only once, not multiple times, and we should reduce the amount we move elements in the vector. Here is the almost identical code I would write that resizes the vector at most once and moves items in the vector at most twice:

    // Remove duplicate dates, then throw away all but the newest dates that fit in the window size.
    auto begin = mergedDates.begin();
    auto end = std::unique(begin, mergedDates.end());
    if (end - begin > operatingDatesWindow)
        end = std::move(end - operatingDatesWindow, end, begin);
    mergedDates.shrink(end - begin);