Bug 196467 – Delay initialization of quota users until the first quota request

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Delay initialization of quota users until the first quota request.

[youenn fablet]

Created attachment 366534 [details]
Patch

[Chris Dumez]

Comment on attachment 366534 [details]
Patch

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

> Source/WebCore/storage/StorageQuotaManager.cpp:61
> +    Vector<StorageQuotaUser*> usersToInitialize;

Since this is a Vector of raw pointers, what makes sure that the user is still alive...

> Source/WebCore/storage/StorageQuotaManager.cpp:69
> +        askUserToInitialize(*user);

... here?

[youenn fablet]

Created attachment 366538 [details]
Patch

[youenn fablet]

Created attachment 366549 [details]
Patch

[youenn fablet]

Created attachment 366610 [details]
Patch

[youenn fablet]

Created attachment 366614 [details]
Patch

[Chris Dumez]

Comment on attachment 366614 [details]
Patch

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

Brady / Sihui should probably take a look at the IDB changes.

> Source/WebCore/ChangeLog:18
> +        Now that aborting a transaction may be delayed due to users not being initialized, there may be cases where

I am personally not confident the changes to IDB are right. It seems it would be a lot safer to not delay transaction aborting to maintain pre-existing behavior as much as possible.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:-172
> -    ASSERT(m_database);

we are changing pre-existing IDB assertions. Given how fragile this code is, I am not convinced this is a good idea.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:66
> +    if (m_server)

This looks fragile / strange. We have a transaction potentially without a database but with a IDBServer?

[Geoffrey Garen]

Comment on attachment 366614 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        This will make sure we do not load Cache API information in memory until actually necessary.

What's the motivation here? Did we measure a performance issue?

[youenn fablet]

> I am personally not confident the changes to IDB are right. It seems it
> would be a lot safer to not delay transaction aborting to maintain
> pre-existing behavior as much as possible.

This is a pre-existing issue of delaying some tasks in case a quota check is needed.
This patch merely makes the issue more visible by potentially delaying these tasks not only when hitting quota but also at start-up.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:-172
> > -    ASSERT(m_database);
> 
> we are changing pre-existing IDB assertions. Given how fragile this code is,
> I am not convinced this is a good idea.

We did the same change in a previous patch below.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:66
> > +    if (m_server)
> 
> This looks fragile / strange. We have a transaction potentially without a
> database but with a IDBServer?

Well, the code is already fragile.
Previously, we were registering a transaction to its IDBServer through the database connection that has a weak pointer to its database that has a ref to its IDBServer.

Now, we are making sure to directly get the IDBServer at construction time to be sure that whatever happens between database connection and database, we will be able to unregister the transaction from the IDBServer.

If that helps, I can split the patches in smaller bits.

> > +        This will make sure we do not load Cache API information in memory until actually necessary.
> 
> What's the motivation here? Did we measure a performance issue?

The goal is to go to a model where read operations on start-up might not require to have initialized users. That way, you can read IDB without initially loading all Cache API.

As said above, this optimization is not creating any new issue.
It somehow increases the risk of existing issues to happen.

[Geoffrey Garen]

> > What's the motivation here? Did we measure a performance issue?
> 
> The goal is to go to a model where read operations on start-up might not
> require to have initialized users. That way, you can read IDB without
> initially loading all Cache API.

Do we have a way to measure the performance benefit of this change?

[youenn fablet]

I have moved the cache storage and IDB bits separately.
Whether we want to delay or not, I think we should do these two bits.

[youenn fablet]

Created attachment 366950 [details]
Patch

[EWS Watchlist]

Comment on attachment 366950 [details]
Patch

Attachment 366950 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11807911

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 366958 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366950 [details]
Patch

Attachment 366950 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11807637

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 366960 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366950 [details]
Patch

Attachment 366950 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11807979

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 366963 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[youenn fablet]

Created attachment 366971 [details]
Patch

[Chris Dumez]

Comment on attachment 366971 [details]
Patch

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

> Source/WebCore/storage/StorageQuotaManager.cpp:68
> +    for (auto* user : usersToInitialize) {

What guarantees that StorageQuotaUser objects are still alive as you iterate? You thought it was worth copying them into a new vector but you keep raw pointers to them.

[youenn fablet]

Comment on attachment 366971 [details]
Patch

Thanks for the review.

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

>> Source/WebCore/storage/StorageQuotaManager.cpp:68
>> +    for (auto* user : usersToInitialize) {
> 
> What guarantees that StorageQuotaUser objects are still alive as you iterate? You thought it was worth copying them into a new vector but you keep raw pointers to them.

The m_pendingInitializationUsers.contains(user) is ensuring this.

The principle is that each quota user at creation time will add themselves to their quota manager and remove themselves at destruction time.
I will refactor the code to move the relevant management code from StorageQuotaUser subclasses into StorageQuotaUser.

[WebKit Commit Bot]

Comment on attachment 366971 [details]
Patch

Clearing flags on attachment: 366971

Committed r244112: <https://trac.webkit.org/changeset/244112>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49778189>

[youenn fablet]

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

[youenn fablet]

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