Bug 196467 - Delay initialization of quota users until the first quota request
Summary: Delay initialization of quota users until the first quota request
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 193976 196228 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-04-01 15:15 PDT by youenn fablet
Modified: 2019-04-10 15:47 PDT (History)
12 users (show)

See Also:


Attachments
Patch (11.53 KB, patch)
2019-04-02 13:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (11.60 KB, patch)
2019-04-02 14:19 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (16.86 KB, patch)
2019-04-02 16:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (16.92 KB, patch)
2019-04-03 08:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (17.01 KB, patch)
2019-04-03 09:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (9.38 KB, patch)
2019-04-08 10:33 PDT, youenn fablet
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.91 MB, application/zip)
2019-04-08 11:58 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.57 MB, application/zip)
2019-04-08 11:59 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.93 MB, application/zip)
2019-04-08 12:10 PDT, EWS Watchlist
no flags Details
Patch (9.40 KB, patch)
2019-04-08 13:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-04-01 15:15:32 PDT
Delay initialization of quota users until the first quota request.
Comment 1 youenn fablet 2019-04-02 13:56:09 PDT
Created attachment 366534 [details]
Patch
Comment 2 Chris Dumez 2019-04-02 14:04:27 PDT
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?
Comment 3 youenn fablet 2019-04-02 14:19:04 PDT
Created attachment 366538 [details]
Patch
Comment 4 youenn fablet 2019-04-02 16:43:23 PDT
Created attachment 366549 [details]
Patch
Comment 5 youenn fablet 2019-04-03 08:43:40 PDT
Created attachment 366610 [details]
Patch
Comment 6 youenn fablet 2019-04-03 09:36:56 PDT
Created attachment 366614 [details]
Patch
Comment 7 Chris Dumez 2019-04-04 09:50:11 PDT
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?
Comment 8 Geoffrey Garen 2019-04-04 10:23:29 PDT
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?
Comment 9 youenn fablet 2019-04-04 11:23:41 PDT
> 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.
Comment 10 Geoffrey Garen 2019-04-04 13:06:34 PDT
> > 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?
Comment 11 youenn fablet 2019-04-05 11:14:21 PDT
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.
Comment 12 youenn fablet 2019-04-08 10:33:29 PDT
Created attachment 366950 [details]
Patch
Comment 13 EWS Watchlist 2019-04-08 11:58:47 PDT
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.
Comment 14 EWS Watchlist 2019-04-08 11:58:49 PDT
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
Comment 15 EWS Watchlist 2019-04-08 11:59:57 PDT
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.
Comment 16 EWS Watchlist 2019-04-08 11:59:59 PDT
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
Comment 17 EWS Watchlist 2019-04-08 12:10:40 PDT
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.
Comment 18 EWS Watchlist 2019-04-08 12:10:42 PDT
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
Comment 19 youenn fablet 2019-04-08 13:08:46 PDT
Created attachment 366971 [details]
Patch
Comment 20 Chris Dumez 2019-04-10 08:58:47 PDT
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.
Comment 21 youenn fablet 2019-04-10 09:04:00 PDT
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.
Comment 22 WebKit Commit Bot 2019-04-10 09:31:03 PDT
Comment on attachment 366971 [details]
Patch

Clearing flags on attachment: 366971

Committed r244112: <https://trac.webkit.org/changeset/244112>
Comment 23 WebKit Commit Bot 2019-04-10 09:31:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2019-04-10 09:32:46 PDT
<rdar://problem/49778189>
Comment 25 youenn fablet 2019-04-10 15:47:16 PDT
*** Bug 196228 has been marked as a duplicate of this bug. ***
Comment 26 youenn fablet 2019-04-10 15:47:36 PDT
*** Bug 193976 has been marked as a duplicate of this bug. ***