Bug 227034 – Use more SQL transactions in ResourceLoadStatisticsDatabaseStore

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Use more SQL transactions in ResourceLoadStatisticsDatabaseStore, for performance.

[Chris Dumez]

Created attachment 431454 [details]
Patch

[Chris Dumez]

Created attachment 431457 [details]
Patch

[Chris Dumez]

Created attachment 431465 [details]
Patch

[Chris Dumez]

Created attachment 431468 [details]
Patch

[Chris Dumez]

Created attachment 431470 [details]
Patch

[Chris Dumez]

Created attachment 431474 [details]
Patch

[Chris Dumez]

Created attachment 431475 [details]
Patch

[Kate Cheney]

Comment on attachment 431475 [details]
Patch

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

I think this is a good idea. I'm curious about the pattern. Do we want all functions that interact with the database to use transactions, or is it as-needed for now to improve performance?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:636
> +        if (!transaction.inProgress())

It doesn't look like the code does anything if you call begin() on a transaction already in progress, you could probably remove this if you wanted.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1039
> +String ResourceLoadStatisticsDatabaseStore::ensureAndMakeDomainList(const SQLiteTransaction&, const HashSet<RegistrableDomain>& domainList)

Did you mean to have ASSERT(m_database.transactionInProgress()); here? Otherwise I am not sure why we should include SQLiteTransaction in this function, unless it is to avoid unused parameter errors in release builds, but in that case I think it can be done in insertDomainRelationshipList.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1158
>          updateDataRecordsRemoved(other.registrableDomain, other.dataRecordsRemoved);

Is there a pattern for which functions ASSERT transactions in progress? I notice a few functions in here that don't have them, like updateDataRecordsRemoved and setGrandfathered.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1457
> +                transaction.begin();

Ditto, I think you can just call transaction.begin() with no consequences if it's already in progress.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1761
> +                if (!transaction.inProgress())

Ditto.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1783
> +            if (!transaction.inProgress())

Ditto.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2248
>      auto result = ensureResourceStatisticsForRegistrableDomain(subresourceDomain);

I wonder if we want to pass transaction to ensureResourceStatisticsForRegistrableDomain and have a check in there for a transaction in progress in the function to match your existing pattern?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3172
>      updateOperatingDatesParameters();

updateOperatingDatesParameters() also interacts with the database, maybe we should move transaction.commit(); below that and pass transaction into the updateOperatingDatesParameters() function.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3229
>      updateOperatingDatesParameters();

Ditto.

[Chris Dumez]

(In reply to katherine_cheney from comment #8)
> Comment on attachment 431475 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431475&action=review
> 
> I think this is a good idea. I'm curious about the pattern. Do we want all
> functions that interact with the database to use transactions, or is it
> as-needed for now to improve performance?

The pattern is that you should be using a transaction if you're doing several consecutive *write* statements.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:636
> > +        if (!transaction.inProgress())
> 
> It doesn't look like the code does anything if you call begin() on a
> transaction already in progress, you could probably remove this if you
> wanted.

I know but I felt this looked clearer and repeatedly calling begin(). Also transaction.inProgress() is very cheap since it is inlined.

> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1039
> > +String ResourceLoadStatisticsDatabaseStore::ensureAndMakeDomainList(const SQLiteTransaction&, const HashSet<RegistrableDomain>& domainList)
> 
> Did you mean to have ASSERT(m_database.transactionInProgress()); here?
> Otherwise I am not sure why we should include SQLiteTransaction in this
> function, unless it is to avoid unused parameter errors in release builds,
> but in that case I think it can be done in insertDomainRelationshipList.

Yes, we should likely ASSERT(m_database.transactionInProgress());. I'll add it.

> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1158
> >          updateDataRecordsRemoved(other.registrableDomain, other.dataRecordsRemoved);
> 
> Is there a pattern for which functions ASSERT transactions in progress? I
> notice a few functions in here that don't have them, like
> updateDataRecordsRemoved and setGrandfathered.

I'll double check those two. I usually ASSERT there is a transaction is in progress if more than one write statement happens in a function and the function doesn't start a transaction (because the caller already has one). In such cases, I also add a SQLiteTransaction parameter to make it clear the caller needs to start a transaction.

> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1457
> > +                transaction.begin();
> 
> Ditto, I think you can just call transaction.begin() with no consequences if
> it's already in progress.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1761
> > +                if (!transaction.inProgress())
> 
> Ditto.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1783
> > +            if (!transaction.inProgress())
> 
> Ditto.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2248
> >      auto result = ensureResourceStatisticsForRegistrableDomain(subresourceDomain);
> 
> I wonder if we want to pass transaction to
> ensureResourceStatisticsForRegistrableDomain and have a check in there for a
> transaction in progress in the function to match your existing pattern?

ensureResourceStatisticsForRegistrableDomain() only does a single write statement so it doesn't technically require a transaction. Also, some of the call sites are not writers but readers so they don't currently have a transaction (even after my patch).

> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3172
> >      updateOperatingDatesParameters();
> 
> updateOperatingDatesParameters() also interacts with the database, maybe we
> should move transaction.commit(); below that and pass transaction into the
> updateOperatingDatesParameters() function.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3229
> >      updateOperatingDatesParameters();
> 
> Ditto.

Ok, I'll double check.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to katherine_cheney from comment #8)
> > Comment on attachment 431475 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=431475&action=review
> > 
> > I think this is a good idea. I'm curious about the pattern. Do we want all
> > functions that interact with the database to use transactions, or is it
> > as-needed for now to improve performance?
> 
> The pattern is that you should be using a transaction if you're doing
> several consecutive *write* statements.
> 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:636
> > > +        if (!transaction.inProgress())
> > 
> > It doesn't look like the code does anything if you call begin() on a
> > transaction already in progress, you could probably remove this if you
> > wanted.
> 
> I know but I felt this looked clearer and repeatedly calling begin(). Also
> transaction.inProgress() is very cheap since it is inlined.
> 
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1039
> > > +String ResourceLoadStatisticsDatabaseStore::ensureAndMakeDomainList(const SQLiteTransaction&, const HashSet<RegistrableDomain>& domainList)
> > 
> > Did you mean to have ASSERT(m_database.transactionInProgress()); here?
> > Otherwise I am not sure why we should include SQLiteTransaction in this
> > function, unless it is to avoid unused parameter errors in release builds,
> > but in that case I think it can be done in insertDomainRelationshipList.
> 
> Yes, we should likely ASSERT(m_database.transactionInProgress());. I'll add
> it.
> 
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1158
> > >          updateDataRecordsRemoved(other.registrableDomain, other.dataRecordsRemoved);
> > 
> > Is there a pattern for which functions ASSERT transactions in progress? I
> > notice a few functions in here that don't have them, like
> > updateDataRecordsRemoved and setGrandfathered.
> 
> I'll double check those two. I usually ASSERT there is a transaction is in
> progress if more than one write statement happens in a function and the
> function doesn't start a transaction (because the caller already has one).
> In such cases, I also add a SQLiteTransaction parameter to make it clear the
> caller needs to start a transaction.
> 
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1457
> > > +                transaction.begin();
> > 
> > Ditto, I think you can just call transaction.begin() with no consequences if
> > it's already in progress.
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1761
> > > +                if (!transaction.inProgress())
> > 
> > Ditto.
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1783
> > > +            if (!transaction.inProgress())
> > 
> > Ditto.
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2248
> > >      auto result = ensureResourceStatisticsForRegistrableDomain(subresourceDomain);
> > 
> > I wonder if we want to pass transaction to
> > ensureResourceStatisticsForRegistrableDomain and have a check in there for a
> > transaction in progress in the function to match your existing pattern?
> 
> ensureResourceStatisticsForRegistrableDomain() only does a single write
> statement so it doesn't technically require a transaction. Also, some of the
> call sites are not writers but readers so they don't currently have a
> transaction (even after my patch).
> 
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3172
> > >      updateOperatingDatesParameters();
> > 
> > updateOperatingDatesParameters() also interacts with the database, maybe we
> > should move transaction.commit(); below that and pass transaction into the
> > updateOperatingDatesParameters() function.
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3229
> > >      updateOperatingDatesParameters();
> > 
> > Ditto.
> 
> Ok, I'll double check.

updateOperatingDatesParameters() only seems to be doing SELECT statements, not write statements. That said, it probably doesn't hurt to include them in the transaction.

[Chris Dumez]

(In reply to katherine_cheney from comment #8)
> Comment on attachment 431475 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431475&action=review
> 
> I think this is a good idea. I'm curious about the pattern. Do we want all
> functions that interact with the database to use transactions, or is it
> as-needed for now to improve performance?
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:636
> > +        if (!transaction.inProgress())
> 
> It doesn't look like the code does anything if you call begin() on a
> transaction already in progress, you could probably remove this if you
> wanted.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1039
> > +String ResourceLoadStatisticsDatabaseStore::ensureAndMakeDomainList(const SQLiteTransaction&, const HashSet<RegistrableDomain>& domainList)
> 
> Did you mean to have ASSERT(m_database.transactionInProgress()); here?
> Otherwise I am not sure why we should include SQLiteTransaction in this
> function, unless it is to avoid unused parameter errors in release builds,
> but in that case I think it can be done in insertDomainRelationshipList.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1158
> >          updateDataRecordsRemoved(other.registrableDomain, other.dataRecordsRemoved);
> 
> Is there a pattern for which functions ASSERT transactions in progress? I
> notice a few functions in here that don't have them, like
> updateDataRecordsRemoved and setGrandfathered.

updateDataRecordsRemoved has a single write statement, therefore it doesn't require a transaction and I don't think we should add a transaction parameter. While it wouldn't hurt currently since there is a single call site and that call site does have a transaction, you could imagine another call site that calls it without a transaction and there would be no perf issue since there is a single write statement.

setGrandfathered() definitely needs a transaction parameter though, I'll add it.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1457
> > +                transaction.begin();
> 
> Ditto, I think you can just call transaction.begin() with no consequences if
> it's already in progress.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1761
> > +                if (!transaction.inProgress())
> 
> Ditto.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1783
> > +            if (!transaction.inProgress())
> 
> Ditto.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2248
> >      auto result = ensureResourceStatisticsForRegistrableDomain(subresourceDomain);
> 
> I wonder if we want to pass transaction to
> ensureResourceStatisticsForRegistrableDomain and have a check in there for a
> transaction in progress in the function to match your existing pattern?
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3172
> >      updateOperatingDatesParameters();
> 
> updateOperatingDatesParameters() also interacts with the database, maybe we
> should move transaction.commit(); below that and pass transaction into the
> updateOperatingDatesParameters() function.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3229
> >      updateOperatingDatesParameters();
> 
> Ditto.

[Chris Dumez]

Created attachment 431491 [details]
Patch

[Chris Dumez]

I still check inProgress() before calling begin() in a few cases where we don't unconditionally call begin(). I feel it is clearer as I commented earlier but if you feel strongly about it, let me know and I'll make the change.

[Kate Cheney]

Comment on attachment 431491 [details]
Patch

I don't feel strongly, I think your logic makes sense. Thanks for explaining.

[EWS]

Committed r278916 (238847@main): <https://commits.webkit.org/238847@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/79372370>