[webkit-reviews] review denied: [Bug 212034] Support operating dates in ResourceLoadStatisticsDatabaseStore : [Attachment 399776] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied katherine_cheney at apple.com's
request for review:
Bug 212034: Support operating dates in ResourceLoadStatisticsDatabaseStore
https://bugs.webkit.org/show_bug.cgi?id=212034

Attachment 399776: Patch

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




--- Comment #8 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 399776
  --> https://bugs.webkit.org/attachment.cgi?id=399776
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:2
86
> +    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.