Bug 235236

Summary: Make LocalStorage prewarming async
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: 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 Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch for landing none

Description Sihui Liu 2022-01-14 10:16:45 PST
...
Comment 1 Sihui Liu 2022-01-14 11:01:58 PST
Created attachment 449186 [details]
Patch
Comment 2 Sihui Liu 2022-01-14 11:46:03 PST
Created attachment 449194 [details]
Patch
Comment 3 Sihui Liu 2022-01-16 18:49:57 PST
Created attachment 449300 [details]
Patch
Comment 4 Sihui Liu 2022-01-17 12:05:23 PST
Created attachment 449344 [details]
Patch
Comment 5 Sihui Liu 2022-01-17 13:06:06 PST
Created attachment 449346 [details]
Patch
Comment 6 Sihui Liu 2022-01-17 15:22:41 PST
Created attachment 449351 [details]
Patch
Comment 7 Chris Dumez 2022-01-18 14:24:25 PST
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 8 Sihui Liu 2022-01-18 15:58:03 PST
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...
Comment 9 Sihui Liu 2022-01-18 16:01:49 PST
Created attachment 449441 [details]
Patch
Comment 10 Sihui Liu 2022-01-18 16:02:46 PST
Created attachment 449442 [details]
Patch
Comment 11 Sihui Liu 2022-01-18 16:10:47 PST
Created attachment 449443 [details]
Patch
Comment 12 Sihui Liu 2022-01-18 16:12:20 PST
Created attachment 449444 [details]
Patch
Comment 13 Sihui Liu 2022-01-18 16:26:19 PST
Created attachment 449447 [details]
Patch
Comment 14 Sihui Liu 2022-01-18 16:42:04 PST
Created attachment 449449 [details]
Patch
Comment 15 Chris Dumez 2022-01-20 08:21:35 PST
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.
Comment 16 Sihui Liu 2022-01-20 08:35:45 PST
Created attachment 449579 [details]
Patch for landing
Comment 17 Sihui Liu 2022-01-20 08:39:11 PST
(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().
Comment 18 EWS 2022-01-20 09:25:19 PST
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].
Comment 19 Radar WebKit Bug Importer 2022-01-20 09:26:19 PST
<rdar://problem/87833142>