Summary: | Make LocalStorage prewarming async | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, webkit-bug-importer | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||
Bug Blocks: | 234925 | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Sihui Liu
2022-01-14 10:16:45 PST
Created attachment 449186 [details]
Patch
Created attachment 449194 [details]
Patch
Created attachment 449300 [details]
Patch
Created attachment 449344 [details]
Patch
Created attachment 449346 [details]
Patch
Created attachment 449351 [details]
Patch
Comment on attachment 449351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449351&action=review > Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:35 > + static std::atomic<uint64_t> currenIdentifier(0); (0) is unnecessary. Does this really get called from different threads? I see that you're using atomic here and generateThreadSafe() below. > Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:36 > + return ++currenIdentifier; typo: current > Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:50 > + ASSERT(!m_listeners.contains(connection) || m_listeners.get(connection) == identifier); Could you clarify why this new assertion is true? A connection identifies a WebProcess. What prevents having several StorageAreaMapIdentifier within a single WebProcess (especially when several tabs share the same WebProcess)? > Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:82 > + HashMap<IPC::Connection::UniqueID, StorageAreaMapIdentifier> m_listeners; I am surprised we can have a map from IPC::Connection/WebProcess to a single StorageAreaMapIdentifier. > Source/WebKit/WebProcess/WebProcess.cpp:1192 > + for (auto key : copyToVector(m_storageAreaMaps.keys())) The fact that we call copyToVector() here seems to indicate that m_storageAreaMaps can change while we iterate. > Source/WebKit/WebProcess/WebProcess.cpp:1193 > + m_storageAreaMaps.get(key)->disconnect(); however, you fail to null check the value returned by m_storageAreaMaps.get(key) here so this doesn't look safe? > Source/WebKit/WebProcess/WebProcess.cpp:1702 > + m_storageAreaMaps.add(identifier, storageAreaMap); Here, the code seems to indicate we can have several StorageAreaMapIdentifiers within a single WebProcess. Comment on attachment 449351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449351&action=review >> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:35 >> + static std::atomic<uint64_t> currenIdentifier(0); > > (0) is unnecessary. > > Does this really get called from different threads? I see that you're using atomic here and generateThreadSafe() below. Will remove. Each session has a storage thread. >> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:36 >> + return ++currenIdentifier; > > typo: current oh, will change. >> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:50 >> + ASSERT(!m_listeners.contains(connection) || m_listeners.get(connection) == identifier); > > Could you clarify why this new assertion is true? > > A connection identifies a WebProcess. What prevents having several StorageAreaMapIdentifier within a single WebProcess (especially when several tabs share the same WebProcess)? Nothing prevents several StorageAreaMaps in a web process, we just don't have multiple StorageAreaMaps in a web process pointing to the same StorageArea... So previously StorageArea should receive only one sync connect message from one web process, but now it may receives two: one async and one sync. One the second sync message, the listener (IPC::Connection) should already be added, and in this case the message should come from the same StorageAreaMap in the process. >> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:82 >> + HashMap<IPC::Connection::UniqueID, StorageAreaMapIdentifier> m_listeners; > > I am surprised we can have a map from IPC::Connection/WebProcess to a single StorageAreaMapIdentifier. In web process, a StorageAreaMap can be identified by {session, origin, storageType} or StorageAreaMapIdentifier. In network process, a StorageArea can be identified by {session, origin, storageType} or StorageAreaIdentifier. Therefore, there should not be two StorageAreaMaps in a web process pointing to the same StorageArea in a web process (a StorageArea should not record two StorageAreaMapIdentifiers for one IPC::Connection.) >> Source/WebKit/WebProcess/WebProcess.cpp:1193 >> + m_storageAreaMaps.get(key)->disconnect(); > > however, you fail to null check the value returned by m_storageAreaMaps.get(key) here so this doesn't look safe? Yes, should add a null check. >> Source/WebKit/WebProcess/WebProcess.cpp:1702 >> + m_storageAreaMaps.add(identifier, storageAreaMap); > > Here, the code seems to indicate we can have several StorageAreaMapIdentifiers within a single WebProcess. That's true a web process can have multiple StorageAreaMapIdentifiers, that's why StorageArea(network process) needs to send back StorageAreaMapIdentifier in the messages to let web process know which StorageAreaMap has change... Created attachment 449441 [details]
Patch
Created attachment 449442 [details]
Patch
Created attachment 449443 [details]
Patch
Created attachment 449444 [details]
Patch
Created attachment 449447 [details]
Patch
Created attachment 449449 [details]
Patch
Comment on attachment 449449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449449&action=review r=me with nit. Should probably monitor PLT bots after this lands given that this changes a PLT optimization. > Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:52 > + m_listeners.set(connection, identifier); Why set() and not add()? add() is more efficient and based on your assertions above, it seems add() would suffice. Created attachment 449579 [details]
Patch for landing
(In reply to Chris Dumez from comment #15) > Comment on attachment 449449 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449449&action=review > > r=me with nit. Should probably monitor PLT bots after this lands given that > this changes a PLT optimization. Sure > > > Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:52 > > + m_listeners.set(connection, identifier); > > Why set() and not add()? add() is more efficient and based on your > assertions above, it seems add() would suffice. Changed to use add(). Committed r288298 (246219@main): <https://commits.webkit.org/246219@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449579 [details]. |