Bug 238608 – IPC::Connection should support diverting all messages to a message queue in other thread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Kimmo Kinnunen]

IPC::Connection should support diverting all messages to a message queue in other thread

[Kimmo Kinnunen]

Created attachment 456237 [details]
Patch

[Simon Fraser (smfr)]

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?

[Kimmo Kinnunen]

Created attachment 456265 [details]
Patch

[Kimmo Kinnunen]

Created attachment 456267 [details]
Patch

[Kimmo Kinnunen]

(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..

[Kimmo Kinnunen]

Created attachment 456354 [details]
Patch

[Chris Dumez]

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.

[Kimmo Kinnunen]

(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.

[Simon Fraser (smfr)]

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.

[Kimmo Kinnunen]

Created attachment 456848 [details]
For landing

[Kimmo Kinnunen]

(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

[Kimmo Kinnunen]

(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!

[Kimmo Kinnunen]

Created attachment 456894 [details]
For landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/91411567>