Bug 226277 – Make CacheStorageEngine use the same WorkQueue as StorageQuotaManager

• Component: WebKit Misc.
• Status: NEW

[Sihui Liu]

...

[Sihui Liu]

Created attachment 429771 [details]
Patch

[Chris Dumez]

Comment on attachment 429771 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Make one WorkQueue per SessionStorageQuotaManager and use it for both StorageQuotaManagar and CacheStorageEngine.

typo: StorageQuotaManagar

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2519
> +RefPtr<WorkQueue> NetworkProcess::storageSessionQueue(PAL::SessionID sessionID)

Do different storage sessions really need different work queues or could they all share the same static WorkQueue. Not that we'd be less likely to have threading bugs if they shared the same queue.

> Source/WebKit/NetworkProcess/NetworkProcess.h:349
> +    RefPtr<WorkQueue> storageSessionQueue(PAL::SessionID);

Seems like this should be const.

[Sihui Liu]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 429771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429771&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        Make one WorkQueue per SessionStorageQuotaManager and use it for both StorageQuotaManagar and CacheStorageEngine.
> 
> typo: StorageQuotaManagar
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2519
> > +RefPtr<WorkQueue> NetworkProcess::storageSessionQueue(PAL::SessionID sessionID)
> 
> Do different storage sessions really need different work queues or could
> they all share the same static WorkQueue. Not that we'd be less likely to
> have threading bugs if they shared the same queue.
> 

'Not that' or 'Note that'? It means opposite things :)!

My thought is if they share the same queue, requests from persistent session will slow down the ones from ephemeral session, which do not need disk access. Requests from persistent session are probably fine as the bottleneck is probably disk I/O even if they don't share and sharing means less locking.

> > Source/WebKit/NetworkProcess/NetworkProcess.h:349
> > +    RefPtr<WorkQueue> storageSessionQueue(PAL::SessionID);
> 
> Seems like this should be const.

[Chris Dumez]

(In reply to Sihui Liu from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Comment on attachment 429771 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=429771&action=review
> > 
> > > Source/WebKit/ChangeLog:8
> > > +        Make one WorkQueue per SessionStorageQuotaManager and use it for both StorageQuotaManagar and CacheStorageEngine.
> > 
> > typo: StorageQuotaManagar
> > 
> > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2519
> > > +RefPtr<WorkQueue> NetworkProcess::storageSessionQueue(PAL::SessionID sessionID)
> > 
> > Do different storage sessions really need different work queues or could
> > they all share the same static WorkQueue. Not that we'd be less likely to
> > have threading bugs if they shared the same queue.
> > 
> 
> 'Not that' or 'Note that'? It means opposite things :)!
> 
> My thought is if they share the same queue, requests from persistent session
> will slow down the ones from ephemeral session, which do not need disk
> access. Requests from persistent session are probably fine as the bottleneck
> is probably disk I/O even if they don't share and sharing means less locking.

That assumes we have parallel work for both persistent and non-persistent sessions. I personally don't think that's super common.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Sihui Liu from comment #3)
> > (In reply to Chris Dumez from comment #2)
> > > Comment on attachment 429771 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=429771&action=review
> > > 
> > > > Source/WebKit/ChangeLog:8
> > > > +        Make one WorkQueue per SessionStorageQuotaManager and use it for both StorageQuotaManagar and CacheStorageEngine.
> > > 
> > > typo: StorageQuotaManagar
> > > 
> > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2519
> > > > +RefPtr<WorkQueue> NetworkProcess::storageSessionQueue(PAL::SessionID sessionID)
> > > 
> > > Do different storage sessions really need different work queues or could
> > > they all share the same static WorkQueue. Not that we'd be less likely to
> > > have threading bugs if they shared the same queue.
> > > 
> > 
> > 'Not that' or 'Note that'? It means opposite things :)!

Darn, I make this typo all the time :(

> > 
> > My thought is if they share the same queue, requests from persistent session
> > will slow down the ones from ephemeral session, which do not need disk
> > access. Requests from persistent session are probably fine as the bottleneck
> > is probably disk I/O even if they don't share and sharing means less locking.
> 
> That assumes we have parallel work for both persistent and non-persistent
> sessions. I personally don't think that's super common.

[youenn fablet]

Comment on attachment 429771 [details]
Patch

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

> Source/WebCore/storage/StorageQuotaManager.cpp:52
>      ASSERT(!isMainThread());

It seems ok to remove requestSpaceOnMainThread.
If the plan for IDB to call requestSpaceOnBackgroundThread in the shared work queue, that will lead to good simplifications.
But, IIRC, that was the hard part due to the IDB was implemented. Have you tried doing so?

> Source/WebKit/NetworkProcess/NetworkProcess.h:513
> +                return WebCore::StorageQuotaManager::create(quota, WTFMove(usageGetter), WTFMove(quotaIncreaseRequester), WTFMove(queue));

Quota management is independent per origin.
If one page of origin A triggers a prompt, it should not block page of origin B to also trigger the prompt.
Conceptually, it seems StorageQuotaManager should have its own queue and not share it with other managers.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:275
> +    m_ioQueue = m_networkProcess->storageSessionQueue(m_sessionID);

This queue is mostly used to read/write cache infrastructure files, like list of cache names, salt, origin...
The actual storage of cache entries is implemented through NetworkCache::Storage which is using concurrent queues for performance reasons.
I am not sure we are solving what we think we are solving with this patch. Computation of actual disk size will always be racy if there are ongoing operations.
Ultimately, if the web page wants to optimise prompting, it would be up to the web page to make careful sequential operations instead of doing several operations in parallel that may or may not hit the quota.

That said, removing requestSpaceOnMainThread is fine by me.

[Sihui Liu]

(In reply to youenn fablet from comment #6)
> Comment on attachment 429771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429771&action=review
> 
> > Source/WebCore/storage/StorageQuotaManager.cpp:52
> >      ASSERT(!isMainThread());
> 
> It seems ok to remove requestSpaceOnMainThread.
> If the plan for IDB to call requestSpaceOnBackgroundThread in the shared
> work queue, that will lead to good simplifications.
> But, IIRC, that was the hard part due to the IDB was implemented. Have you
> tried doing so?

Yes, I tried to make IDB use WorkQueue (first step is creating a dedicated thread for JS serialization https://bugs.webkit.org/show_bug.cgi?id=226228).

>
> > Source/WebKit/NetworkProcess/NetworkProcess.h:513
> > +                return WebCore::StorageQuotaManager::create(quota, WTFMove(usageGetter), WTFMove(quotaIncreaseRequester), WTFMove(queue));
> 
> Quota management is independent per origin.
> If one page of origin A triggers a prompt, it should not block page of
> origin B to also trigger the prompt.
> Conceptually, it seems StorageQuotaManager should have its own queue and not
> share it with other managers.

Is it possible to show multiple quota prompts at the same time? If user can only see/handle one prompt at one time, it might be Okay. 

I use one queue per session as it simplifies things, e.g. fetching/deleting data for origins will be one task on the shared queue instead of multiple tasks on different queues, or when we stop storage activities (e.g. for suspension), we may just need to suspend the shared queue.

> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:275
> > +    m_ioQueue = m_networkProcess->storageSessionQueue(m_sessionID);
> 
> This queue is mostly used to read/write cache infrastructure files, like
> list of cache names, salt, origin...
> The actual storage of cache entries is implemented through
> NetworkCache::Storage which is using concurrent queues for performance
> reasons.
> I am not sure we are solving what we think we are solving with this patch.
> Computation of actual disk size will always be racy if there are ongoing
> operations.
> Ultimately, if the web page wants to optimise prompting, it would be up to
> the web page to make careful sequential operations instead of doing several
> operations in parallel that may or may not hit the quota.
> 
> That said, removing requestSpaceOnMainThread is fine by me.

Yes I think to make the disk usage accurate we should do all the writes at the same queue, or at least we should ensure granted writes happen before next quota check, which likely requires architectural change in Cache storage.

If necessary, we can also update the size file (based on estimate) at the shared queue, and correct size file periodically based on results from other queues of NetworkCache::Storage.

[Sihui Liu]

Created attachment 429904 [details]
Patch

[Sihui Liu]

Created attachment 429909 [details]
Patch

[Chris Dumez]

API test failure looks related.

[Sihui Liu]

Created attachment 430022 [details]
Patch

[youenn fablet]

Comment on attachment 430022 [details]
Patch

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

> Source/WebCore/storage/StorageQuotaManager.cpp:45
> +    , m_workQueue(queue)

WTFMove

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:251
> +    if (!m_ioQueue || !m_networkProcess) {

It does not seem like m_ioQueue can be null now.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:262
> +    m_ioQueue->dispatch([storageQuotaManager, spaceRequested, callback = WTFMove(callback)]() mutable {

storageQuotaManager  = WTFMove(storageQuotaManager)

We should probably add a comment here to state why we have to hop in to ioQueue to request space.

> Tools/ChangeLog:8
> +        Update the test expectation as with this change cache request from one origin may block that from another.

I don't think we want to have that behavior.
If we go with that change, we should be able to have requests from different origins run in parallel.
As discussed offline, either with a queue per quota manager, or as you proposed, quota manager handling several callbacks in parallel.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:83
> +    NSLog(@"sihuil: decideDatabaseQuotaForSecurityOrigin sets receivedQuotaDelegateCalled true");

to be removed here and below.

[Radar WebKit Bug Importer]

<rdar://problem/78769703>