Bug 238608 - IPC::Connection should support diverting all messages to a message queue in other thread
Summary: IPC::Connection should support diverting all messages to a message queue in o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks: 237674 238516
  Show dependency treegraph
 
Reported: 2022-03-31 07:54 PDT by Kimmo Kinnunen
Modified: 2022-04-26 05:41 PDT (History)
7 users (show)

See Also:


Attachments
Patch (23.22 KB, patch)
2022-03-31 08:06 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (27.23 KB, patch)
2022-03-31 12:10 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (27.56 KB, patch)
2022-03-31 12:15 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (27.76 KB, patch)
2022-04-01 05:46 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
For landing (27.80 KB, patch)
2022-04-06 12:36 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
For landing (28.04 KB, patch)
2022-04-06 23:33 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2022-03-31 07:54:07 PDT
IPC::Connection should support diverting all messages to a message queue in other thread
Comment 1 Kimmo Kinnunen 2022-03-31 08:06:46 PDT
Created attachment 456237 [details]
Patch
Comment 2 Simon Fraser (smfr) 2022-03-31 12:08:11 PDT
Comment on attachment 456237 [details]
Patch

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

> Source/WebKit/Platform/IPC/MessageReceiveQueueMap.h:56
>      using QueueMap = HashMap<std::pair<uint8_t, uint64_t>, StoreType>;
> +    using AnyIDQueueMap = HashMap<uint8_t, StoreType>;

Why don't these use ReceiverName in place of uint8_t?
Comment 3 Kimmo Kinnunen 2022-03-31 12:10:20 PDT
Created attachment 456265 [details]
Patch
Comment 4 Kimmo Kinnunen 2022-03-31 12:15:22 PDT
Created attachment 456267 [details]
Patch
Comment 5 Kimmo Kinnunen 2022-03-31 12:28:34 PDT
(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 456237 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456237&action=review
> 
> > Source/WebKit/Platform/IPC/MessageReceiveQueueMap.h:56
> >      using QueueMap = HashMap<std::pair<uint8_t, uint64_t>, StoreType>;
> > +    using AnyIDQueueMap = HashMap<uint8_t, StoreType>;
> 
> Why don't these use ReceiverName in place of uint8_t?


IIRC originally I kept using the code that was there originally.
I think it may have something to do with non-valid keys or hashing. I can take a look at that in a separate patch..
Comment 6 Kimmo Kinnunen 2022-04-01 05:46:42 PDT
Created attachment 456354 [details]
Patch
Comment 7 Chris Dumez 2022-04-01 14:36:14 PDT
Comment on attachment 456354 [details]
Patch

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

> Source/WebKit/Platform/IPC/Connection.cpp:374
> +    ReceiverMatcher receiverMatcher = ReceiverMatcher::createForLegacyAPI(receiverName, destinationID);

How is this legacy?

> Source/WebKit/Platform/IPC/Connection.cpp:389
> +    ReceiverMatcher receiverMatcher = ReceiverMatcher::createForLegacyAPI(receiverName, destinationID);

ditto.
Comment 8 Kimmo Kinnunen 2022-04-02 00:24:35 PDT
(In reply to Chris Dumez from comment #7)
> Comment on attachment 456354 [details]

> > Source/WebKit/Platform/IPC/Connection.cpp:374
> > +    ReceiverMatcher receiverMatcher = ReceiverMatcher::createForLegacyAPI(receiverName, destinationID);
> 
> How is this legacy?
> 
> > Source/WebKit/Platform/IPC/Connection.cpp:389
> > +    ReceiverMatcher receiverMatcher = ReceiverMatcher::createForLegacyAPI(receiverName, destinationID);
> 
> ditto.

Passing zero id to mean any id is legacy and new interfaces should use receiver matcher to explicitly say if the match should apply for zero only or any id.
Comment 9 Simon Fraser (smfr) 2022-04-06 11:55:02 PDT
Comment on attachment 456354 [details]
Patch

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

>>> Source/WebKit/Platform/IPC/Connection.cpp:374
>>> +    ReceiverMatcher receiverMatcher = ReceiverMatcher::createForLegacyAPI(receiverName, destinationID);
>> 
>> How is this legacy?
> 
> Passing zero id to mean any id is legacy and new interfaces should use receiver matcher to explicitly say if the match should apply for zero only or any id.

Maybe instead of "legacy API", which is opaque unless you know the history of this code, use something like createAllowingCatchAllDestination()

> Source/WebKit/Platform/IPC/Connection.h:247
> +    void removeWorkQueueMessageReceiver(ReceiverName, uint64_t destinationID = 0);

Why don't we use std::optional<destinationID> here to match ReceiverMatcher behavior, making the comment unnecessary.

> Source/WebKit/Platform/IPC/Connection.h:253
> +    void removeThreadMessageReceiver(ReceiverName, uint64_t destinationID = 0);

Ditto

> Source/WebKit/Platform/IPC/MessageReceiveQueueMap.cpp:40
> +    if (!matcher.destinationID) {

I find !has_value() more readable when dealing with optionals with bool or sometimes-zero values.

> Source/WebKit/Platform/IPC/MessageReceiveQueueMap.h:56
> +    using AnyIDQueueMap = HashMap<uint8_t, StoreType>;

Would be nice to add a comment to say that the key is a ReceiverName.

> Source/WebKit/Platform/IPC/ReceiverMatcher.h:41
> +    // Note: destinationID == 0 matches only 0 ids.

Is a 0 ID valid? It would be clearer to be able to ASSERT(destinationID) here.

> Source/WebKit/Platform/IPC/ReceiverMatcher.h:62
> +    std::optional<ReceiverName> receiverName;
> +    std::optional<uint64_t> destinationID;

This struct is going to end up as 128 bytes (or larger?) so maybe you should pass it around by reference.
Comment 10 Kimmo Kinnunen 2022-04-06 12:36:49 PDT
Created attachment 456848 [details]
For landing
Comment 11 Kimmo Kinnunen 2022-04-06 12:47:46 PDT
(In reply to Simon Fraser (smfr) from comment #9)
> Maybe instead of "legacy API", which is opaque unless you know the history
> of this code, use something like createAllowingCatchAllDestination()

Right, but that is then "legitimizing" the pattern. This commit is because we got into a bit of a pickle with overloading the value 0. The intention is to communicate that future uses won't end up calling that, rather using the struct here directly.

I can of course rethink some other name.
 
> 
> > Source/WebKit/Platform/IPC/Connection.h:247
> > +    void removeWorkQueueMessageReceiver(ReceiverName, uint64_t destinationID = 0);
> 
> Why don't we use std::optional<destinationID> here to match ReceiverMatcher
> behavior, making the comment unnecessary.

Because we're in a pickle of not knowing what ALL the callers mean with destinationID == 0.  This would be a change of behavior. I don't want to do in this patch, I'd need to spend time going through all the many callers. I'll spend the time, but not in this patch.
 

> > Source/WebKit/Platform/IPC/ReceiverMatcher.h:41
> > +    // Note: destinationID == 0 matches only 0 ids.
> 
> Is a 0 ID valid? It would be clearer to be able to ASSERT(destinationID)
> here.

Yes, 0 is valid and used. Hence much of the patch.



I'll apply the changes tomorrow
Comment 12 Kimmo Kinnunen 2022-04-06 23:31:54 PDT
(In reply to Simon Fraser (smfr) from comment #9)
> > Source/WebKit/Platform/IPC/ReceiverMatcher.h:62
> > +    std::optional<ReceiverName> receiverName;
> > +    std::optional<uint64_t> destinationID;
> 
> This struct is going to end up as 128 bytes (or larger?) so maybe you should
> pass it around by reference.

Interesting surprise to me I didn't spend any thought on. It's 24 bytes as a struct!
https://godbolt.org/z/hKe4qq51v

unoptionalized same feature would be 16 bytes!
Comment 13 Kimmo Kinnunen 2022-04-06 23:33:16 PDT
Created attachment 456894 [details]
For landing
Comment 14 EWS 2022-04-07 06:36:12 PDT
Committed r292533 (249371@main): <https://commits.webkit.org/249371@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456894 [details].
Comment 15 Radar WebKit Bug Importer 2022-04-07 06:37:15 PDT
<rdar://problem/91411567>