Bug 212034 – Support operating dates in ResourceLoadStatisticsDatabaseStore

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

We should add an Operating Dates table to the database schema.

[Radar WebKit Bug Importer]

<rdar://problem/63349242>

[Kate Cheney]

Created attachment 399658 [details]
Patch

[Kate Cheney]

Created attachment 399681 [details]
Patch

[John Wilander]

Comment on attachment 399681 [details]
Patch

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

I like the tests. Was there something done to migrate the schema here?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3027
> +Optional<Seconds> ResourceLoadStatisticsDatabaseStore::statisticsExpirationTime() const

Looking at this function name and body, it's unclear what it does and what the scope of it is. It's const so nothing changes and it looks like a getter. But statisticsExpirationTime sounds like either the expiration time for an individual piece of statistics, in which case I'd expect the function to take a parameter, or some global expiration time which I don't think exists. If you explain to me what it does or is for, maybe we can come up with a better name together.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3157
> +void ResourceLoadStatisticsDatabaseStore::insertExpiredStatisticForTesting(const RegistrableDomain& domain, bool hasUserInteraction, bool isScheduledForAllButCookieDataRemoval, bool isPrevalent)

I wonder if we should break out these test functions into a friend class at some point. I don't know if that's something we do in WebKit but it sure would be nice to declutter the production logic.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1201
> +

Were these 7 functions just moved from the base class? I know your change log entry says "Moves functions out of ResourceLoadStatisticsStore …" but it doesn't list which. Please make it more explicit in the change log since it looks like a big chunk of new functionality on the surface of it.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.cpp:211
> +    // Debug mode has a prepopulated memory store.

Nice.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:-57
> -constexpr Seconds operatingTimeWindowForLiveOnTesting { 1_h };

Please comment in the change log that these are moved to the header file.

[Kate Cheney]

(In reply to John Wilander from comment #4)
> Comment on attachment 399681 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399681&action=review
> 
> I like the tests. Was there something done to migrate the schema here?
> 

No additional migration code necessary, I recently added support for adding new tables and migrating existing data from the old tables over :)

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3027
> > +Optional<Seconds> ResourceLoadStatisticsDatabaseStore::statisticsExpirationTime() const
> 
> Looking at this function name and body, it's unclear what it does and what
> the scope of it is. It's const so nothing changes and it looks like a
> getter. But statisticsExpirationTime sounds like either the expiration time
> for an individual piece of statistics, in which case I'd expect the function
> to take a parameter, or some global expiration time which I don't think
> exists. If you explain to me what it does or is for, maybe we can come up
> with a better name together.
> 

Yes, definitely. This is used to clear expired user interactions in ResourceLoadStatisticsDatabaseStore::registrableDomainsToDeleteOrRestrictWebsiteDataFor(). It checks for the least recent Operating Date and will signal the store to clear UI that happened before that time. We could perhaps update the name to be "getLeastRecentOperatingDate()" or something of the sort. Or, we could have it take a statistic and return a bool deciding whether it was expired or not.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3157
> > +void ResourceLoadStatisticsDatabaseStore::insertExpiredStatisticForTesting(const RegistrableDomain& domain, bool hasUserInteraction, bool isScheduledForAllButCookieDataRemoval, bool isPrevalent)
> 
> I wonder if we should break out these test functions into a friend class at
> some point. I don't know if that's something we do in WebKit but it sure
> would be nice to declutter the production logic.
> 

Agreed. Most of the setters in this file are for testing only.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1201
> > +
> 
> Were these 7 functions just moved from the base class? I know your change
> log entry says "Moves functions out of ResourceLoadStatisticsStore …" but it
> doesn't list which. Please make it more explicit in the change log since it
> looks like a big chunk of new functionality on the surface of it.
> 
> > 

Yes, will specify in ChangeLog.

Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsPersistentStorage.cpp:211
> > +    // Debug mode has a prepopulated memory store.
> 
> Nice.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:-57
> > -constexpr Seconds operatingTimeWindowForLiveOnTesting { 1_h };
> 
> Please comment in the change log that these are moved to the header file.

Will do!

[Kate Cheney]

Comment on attachment 399681 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3027
>>> +Optional<Seconds> ResourceLoadStatisticsDatabaseStore::statisticsExpirationTime() const
>> 
>> Looking at this function name and body, it's unclear what it does and what the scope of it is. It's const so nothing changes and it looks like a getter. But statisticsExpirationTime sounds like either the expiration time for an individual piece of statistics, in which case I'd expect the function to take a parameter, or some global expiration time which I don't think exists. If you explain to me what it does or is for, maybe we can come up with a better name together.
> 
> Yes, definitely. This is used to clear expired user interactions in ResourceLoadStatisticsDatabaseStore::registrableDomainsToDeleteOrRestrictWebsiteDataFor(). It checks for the least recent Operating Date and will signal the store to clear UI that happened before that time. We could perhaps update the name to be "getLeastRecentOperatingDate()" or something of the sort. Or, we could have it take a statistic and return a bool deciding whether it was expired or not.

I just noticed statisticsExpirationTime() is unused in the memory store, so I will delete that.

[Kate Cheney]

Created attachment 399776 [details]
Patch

[Brent Fulgham]

Comment on attachment 399776 [details]
Patch

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

I think this looks good - -and thank you for building such good test infrastructure. But I think you are doing way too many DB reads for this purpose. Instead, I think you should check and store these values at launch (and when you change them), but otherwise avoid hitting the DB. r- to fix that performance issue, but otherwise this looks great!

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3027
> +Optional<Seconds> ResourceLoadStatisticsDatabaseStore::getLeastRecentOperatingDate() const

I prefer the original "statisticsExperationTime" name :-)

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3034
> +    if (m_countOperatingDatesStatement.step() != SQLITE_ROW) {

We call this method every time we do a sweep of the data for stuff to delete (every few seconds), but this count only changes once a day, in includeTodayAsOperatingDateIfNecessary. I think we should compute its value in that method, and refer to it here.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3040
> +    const unsigned operatingDatesSize = m_countOperatingDatesStatement.getColumnInt(0);

I think this should be a member variable we compute in includeTodayAsOperatingDateIfNecessary and refer to elsewhere.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3068
> +    auto operatingDatesSize = m_countOperatingDatesStatement.getColumnInt(0);

I think this should be a member variable we can refer to later.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3080
> +        resetStatement(m_getMostRecentOperatingDateStatement);

It's almost like we need a scoped destructing object that resets a statement when leaving a method like this. Then you wouldn't have to have these cleanup steps everywhere.
But we probably only call this once a day -- it might be simpler to just have this statement created, prepared, and used in this method as a local.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3088
> +        if (m_deleteLeastRecentOperatingDatesStatement.bindInt(1, rowsToPrune) != SQLITE_OK

This statement is used rarely, too. I wonder if we couldn't just build it from a string right here and execute it, rather than having a member to hold it (and remember to reset it at the end, etc.).

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3096
> +    if (m_insertOperatingDateStatement.bindInt(1, today.year()) != SQLITE_OK

Ditto. We insert at most one time a day, so I'm not sure the complexity of storing this statement for later use is paid for in performance benefit.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3125
> +    if (m_countOperatingDatesStatement.step() != SQLITE_ROW) {

This method gets called every time we update cookie blocking, which might be very frequent. But our count of operating dates only changes once a day. So I think making this database call every time we hit this function is wrong.

I suggest that you store the operating date count as a member, updating it each time you call  includeTodayAsOperatingDateIfNecessary

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3142
> +        resetStatement(m_getLeastRecentOperatingDateStatement);

I suggest you grab this value in includeTodayAsOperatingDateIfNecessary, and store it to be checked here. (It only changes once a day, blah blah blah ...) :-)

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3175
> +    }

If you make the changes I suggest, you will need to re-read the count (or just update the member variables) here.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:286
> +    mutable WebCore::SQLiteStatement m_getLeastRecentOperatingDateStatement;

I think we could get rid of a few of these stored queries, since they are only run once a day.

[Kate Cheney]

Created attachment 399870 [details]
Patch

[Brent Fulgham]

Comment on attachment 399870 [details]
Patch

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

This looks great! r=me.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2428
> +    Optional<Seconds> expirationDateTime = statisticsExpirationTime();

Oh gosh. What a typo! Thanks! :-)

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3059
> +        double daysToSubtract = Seconds::fromHours(24 * i).value();

If we have to do this frequently, we should consider making a 'Seconds::fromDays()' helper. But not right now.

[EWS]

Committed r261963: <https://trac.webkit.org/changeset/261963>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399870 [details].

[Brent Fulgham]

*** Bug 211775 has been marked as a duplicate of this bug. ***