Summary: | Perform FileSystemSyncAccessHandle operations in web process | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | ggaren, sihui_liu, webkit-bug-importer, youennf | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 231706 | ||||||||||||||||||||||
Attachments: |
|
Description
Sihui Liu
2021-10-22 08:39:31 PDT
Created attachment 442645 [details]
Patch
Created attachment 442647 [details]
Patch
Seems like a nice approach. Comment on attachment 442647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442647&action=review > Source/WebCore/ChangeLog:8 > + truncate(), getSize() and flush() operations will actually be sync operations. For read/write, we have to block but for other methods, we should try not to block. I would tend to implement the meat of these operations in a single WorkerGlobalScope work queue and use promises to signal when we are back from the work queue. > Source/WebKit/ChangeLog:9 > + file handle, pass it to web process and close it. We probably need to have a dedicated network process crash handling (in this patch or as a follow-up) since the WebProcess sync handle might continue working while the new network process will not know of this pre-existing sync handle. Created attachment 442789 [details]
Patch
Comment on attachment 442789 [details] Patch Looks almost ready, I'd like to understand whether close should also hop to the work queue. View in context: https://bugs.webkit.org/attachment.cgi?id=442789&action=review > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:75 > + result = Exception { UnknownError }; It seems we could just return success here and create the Exception Object in worker as needed. Ditto for the other methods. > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:83 > + workerThread->runLoop().postTask(WTFMove(task)); I do not think we need a specific task: workerThread->runLoop().postTask([...] { ... {); should work just fine. Ditto below. > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:159 > + for (auto& promise : pendingPromises) { Is that correct? I would have expected that close would be queued on the work queue so that the following would actually work fine: handle.flush(); // no await handle.close(); // flush first, then close. To make it work, that would mean calling closeFile within a WorkerGlobalScope::postFileSystemStorageTask task. In any case, we should be able to write a test and validate this with other implementations. Created attachment 443025 [details]
Patch
Created attachment 443039 [details]
Patch
Created attachment 443040 [details]
Patch
Created attachment 443045 [details]
Patch
Created attachment 443050 [details]
Patch
Comment on attachment 443050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443050&action=review > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:159 > +{ Might be worth adding ASSERT(!isClosingOrClosed()) since we are calling this from several places. > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:163 > + }); return here to remove the else. > Source/WebCore/workers/WorkerGlobalScope.cpp:82 > + static NeverDestroyed<Ref<WorkQueue>> queue(WorkQueue::create("Shared File System Storage Queue", WorkQueue::QOS::Utility)); Not really sure what the best QOS value is. On one hand, we want to be fast but we do not want too many fast queues. Maybe default is fine here. > Source/WebCore/workers/WorkerGlobalScope.cpp:83 > + return queue.get().copyRef(); Seems wasteful to return a Ref<>. We could return a WorQueue&, or define the static queue directly in WorkerGlobalScope::postFileSystemStorageTask Comment on attachment 443050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443050&action=review >> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:159 >> +{ > > Might be worth adding ASSERT(!isClosingOrClosed()) since we are calling this from several places. hmmm closeBackend() can happen after isClosingOrClosed is true; see FileSystemSyncAccessHandle::~FileSystemSyncAccessHandle(). >> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:163 >> + }); > > return here to remove the else. Sure. >> Source/WebCore/workers/WorkerGlobalScope.cpp:82 >> + static NeverDestroyed<Ref<WorkQueue>> queue(WorkQueue::create("Shared File System Storage Queue", WorkQueue::QOS::Utility)); > > Not really sure what the best QOS value is. On one hand, we want to be fast but we do not want too many fast queues. > Maybe default is fine here. Okay, will change to Default. >> Source/WebCore/workers/WorkerGlobalScope.cpp:83 >> + return queue.get().copyRef(); > > Seems wasteful to return a Ref<>. We could return a WorQueue&, or define the static queue directly in WorkerGlobalScope::postFileSystemStorageTask Sure. Created attachment 443778 [details]
Patch for landing
Committed r285566 (244074@main): <https://commits.webkit.org/244074@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443778 [details]. |