Summary: | Delay initialization of quota users until the first quota request | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||||||||
Component: | Service Workers | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | alecflett, beidson, cdumez, cgarcia, commit-queue, ews-watchlist, ggaren, jsbell, rniwa, sihui_liu, sroberts, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=196650 https://bugs.webkit.org/show_bug.cgi?id=196651 |
||||||||||||||||||||||||
Attachments: |
|
Description
youenn fablet
2019-04-01 15:15:32 PDT
Created attachment 366534 [details]
Patch
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? Created attachment 366538 [details]
Patch
Created attachment 366549 [details]
Patch
Created attachment 366610 [details]
Patch
Created attachment 366614 [details]
Patch
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 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? > 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. > > 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?
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. Created attachment 366950 [details]
Patch
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. 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 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. 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 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. 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
Created attachment 366971 [details]
Patch
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 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 on attachment 366971 [details] Patch Clearing flags on attachment: 366971 Committed r244112: <https://trac.webkit.org/changeset/244112> All reviewed patches have been landed. Closing bug. *** Bug 196228 has been marked as a duplicate of this bug. *** *** Bug 193976 has been marked as a duplicate of this bug. *** |