Reduce IPC overhead for message receiver name and message name to 2 bytes

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Reduce IPC overhead for message receiver name and message name to 2 bytes

[Alex Christensen]

Created attachment 397783 [details]
Patch

[Alex Christensen]

Created attachment 397806 [details]
Patch

[Alex Christensen]

Created attachment 397807 [details]
Patch

[Alex Christensen]

Created attachment 397815 [details]
Patch

[Alex Christensen]

Created attachment 397841 [details]
Patch

[Alex Christensen]

Don, could you get this working with CMake/Windows?  I've changed the parameters to the message generators and added MessageNames.h and MessageNames.cpp.

[Sihui Liu]

Comment on attachment 397841 [details]
Patch

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

> Source/WebKit/Scripts/MessageNames.h:33
> +    WebPage = 1
> +    , WebPage = 2

Looks like duplicate?

[Alex Christensen]

That's fine.  That's because the test receivers have multiple receivers with the same name.  That file is just to test the generator.

[Michael Catanzaro]

Looks like it's trying to build GPUProcessMessageReceiver.cpp. Probably missing an #if ENABLE(GPU_PROCESS) check somewhere?

[585/1094] Generating ../../DerivedSources/WebKit/GPUProcessMessageReceiver.cpp, ../../DerivedSources/WebKit/GPUProcessMessages.h, ../../DerivedSources/WebKit/GPUProcessMessagesReplies.h
FAILED: DerivedSources/WebKit/GPUProcessMessageReceiver.cpp DerivedSources/WebKit/GPUProcessMessages.h DerivedSources/WebKit/GPUProcessMessagesReplies.h 
cd /home/ews/worker/GTK-Build-EWS/build/Source/WebKit && /usr/bin/python2.7 /home/ews/worker/GTK-Build-EWS/build/Source/WebKit/Scripts/generate-message-receiver.py GPUProcess/GPUProcess.messages.in --implementation /home/ews/worker/GTK-Build-EWS/build/WebKitBuild/Release/DerivedSources/WebKit/GPUProcessMessageReceiver.cpp --header /home/ews/worker/GTK-Build-EWS/build/WebKitBuild/Release/DerivedSources/WebKit/GPUProcessMessages.h --reply-header /home/ews/worker/GTK-Build-EWS/build/WebKitBuild/Release/DerivedSources/WebKit/GPUProcessMessagesReplies.h
Generating message receiver for --implementation
Traceback (most recent call last):
  File "/home/ews/worker/GTK-Build-EWS/build/Source/WebKit/Scripts/generate-message-receiver.py", line 72, in <module>
    sys.exit(main(sys.argv))
  File "/home/ews/worker/GTK-Build-EWS/build/Source/WebKit/Scripts/generate-message-receiver.py", line 50, in main
    with open('%s/%s.messages.in' % (base_dir, parameter)) as source_file:
IOError: [Errno 20] Not a directory: 'GPUProcess/GPUProcess.messages.in/--implementation.messages.in'

[Alex Christensen]

The point of me cc'ing people who work on GTK was that this will take a bit more effort than I can do just looking at EWS.  generate-message-receiver.py used to be invoked for each .messages.in file with --implementation, --header, and --reply-header in argv, and this requires changing the CMake files to call generate-message-receiver.py with the Source/WebKit directory then a complete list of all the .messages.in files without the .messages.in suffix.  Could someone get that working on Linux?

[Michael Catanzaro]

It's the sort of thing I would have made time for back when I worked at Igalia, but I'm swamped with non-WebKit tasks right now. Somebody else will probably help.

[Don Olmstead]

(In reply to Alex Christensen from comment #10)
> The point of me cc'ing people who work on GTK was that this will take a bit
> more effort than I can do just looking at EWS.  generate-message-receiver.py
> used to be invoked for each .messages.in file with --implementation,
> --header, and --reply-header in argv, and this requires changing the CMake
> files to call generate-message-receiver.py with the Source/WebKit directory
> then a complete list of all the .messages.in files without the .messages.in
> suffix.  Could someone get that working on Linux?

Hey Alex,

One issue I foresee with this particular change is that you're going to hit the windows command line limit of 32767 characters. Can you have it so this script can take the values from a file?

https://support.microsoft.com/en-us/help/830473/command-prompt-cmd-exe-command-line-string-limitation

[Alex Christensen]

The parameters are python (maybe with a full path), path to generate-message-receiver.py, path to Source/WebKit directory, then a list of relative paths to all the .messages.in files, which is currently 5385 characters.  So unless you're using 9kb paths, you should be fine.  If someone runs up against the limit we can fix it, but I don't foresee that happening.

[Alex Christensen]

Adding all Igalia accounts who have committed to WebKit in the last few days.

[Don Olmstead]

Created attachment 398233 [details]
Patch CMake

Changes the CMake code for messages.in files

[Don Olmstead]

Created attachment 398245 [details]
Patch CMake

[Don Olmstead]

Comment on attachment 398245 [details]
Patch CMake

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

I checked with WinCairo and incremental builds work and everything links. There was a bug in the previous patch where it wasn't iterating through the list.

Source/WebKit/UIProcess/LegacySessionStateCodingNone.cpp still doesn't compile but I just commented out the offending code to test the build bits.

> Source/WebKit/CMakeLists.txt:416
> +        list(APPEND _outputs

Think maybe _outputs should be _output_files for symmetry with the _input_files variable.

> Source/WebKit/CMakeLists.txt:429
> +            ${_outputs}

Ditto

[Alex Christensen]

Created attachment 398252 [details]
Patch

[Alex Christensen]

Created attachment 398262 [details]
Patch

[Philippe Normand]

diff --git a/Source/WebKit/UIProcess/LegacySessionStateCodingNone.cpp b/Source/WebKit/UIProcess/LegacySessionStateCodingNone.cpp                                                                                  
index a1ef8d6df672..762f55ed5c9e 100644
--- a/Source/WebKit/UIProcess/LegacySessionStateCodingNone.cpp
+++ b/Source/WebKit/UIProcess/LegacySessionStateCodingNone.cpp
@@ -39,7 +39,7 @@ namespace WebKit {
 RefPtr<API::Data> encodeLegacySessionState(const SessionState& sessionState)
 {
     // FIXME: This should use WTF::Persistence::Encoder instead.
-    IPC::Encoder encoder(MessageName::LegacySessionState, 0);
+    IPC::Encoder encoder(IPC::MessageName::LegacySessionState, 0);
     encoder << sessionState.backForwardListState;
     encoder << sessionState.renderTreeSize;
     encoder << sessionState.provisionalURL;

[Philippe Normand]

Nice palindrome bug BTW :)

[Alex Christensen]

Created attachment 398390 [details]
patch

[Chris Dumez]

Comment on attachment 398390 [details]
patch

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

> Source/WebKit/Platform/IPC/Connection.cpp:1002
> +    auto it = m_workQueueMessageReceivers.find(static_cast<uint8_t>(message->messageReceiverName()));

Why do we have all these new static casts? Can they be avoided?

> Source/WebKit/Platform/IPC/Connection.h:349
> +    using WorkQueueMessageReceiverMap = HashMap<uint8_t, std::pair<RefPtr<WorkQueue>, RefPtr<WorkQueueMessageReceiver>>>;

Can we use a stricter type here than uint8_t?

> Source/WebKit/Platform/IPC/Connection.h:353
> +    using ThreadMessageReceiverMap = HashMap<uint8_t, RefPtr<ThreadMessageReceiver>>;

ditto.

> Source/WebKit/Platform/IPC/MessageReceiverMap.h:59
> +    HashMap<uint8_t, MessageReceiver*> m_globalMessageReceivers;

ditto.

> Source/WebKit/Platform/IPC/MessageReceiverMap.h:61
> +    HashMap<std::pair<uint8_t, uint64_t>, MessageReceiver*> m_messageReceivers;

ditto.

[Alex Christensen]

We could avoid all these casts if we could have a HashMap with an enum type as a key.  I bet we could do it in HashMap by just using std::underlying_type and some static_asserts with isValidKey, but this is the best we can do until that change.  Maybe we should do that change first?

What do you mean by a stricter type?

[Chris Dumez]

(In reply to Alex Christensen from comment #24)
> We could avoid all these casts if we could have a HashMap with an enum type
> as a key.  I bet we could do it in HashMap by just using
> std::underlying_type and some static_asserts with isValidKey, but this is
> the best we can do until that change.  Maybe we should do that change first?
> 
> What do you mean by a stricter type?

I mean the enum type rather than its underlying type.

[Chris Dumez]

(In reply to Alex Christensen from comment #24)
> We could avoid all these casts if we could have a HashMap with an enum type
> as a key.  I bet we could do it in HashMap by just using
> std::underlying_type and some static_asserts with isValidKey, but this is
> the best we can do until that change.  Maybe we should do that change first?

I suggest looking into StrongEnumHashTraits which I believe is used for this purpose.

> 
> What do you mean by a stricter type?

[Alex Christensen]

Created attachment 398631 [details]
Patch

[Chris Dumez]

EWS is very red.

[Alex Christensen]

Created attachment 398641 [details]
Patch

[Alex Christensen]

Created attachment 398644 [details]
Patch

[Chris Dumez]

Comment on attachment 398644 [details]
Patch

r=me

[EWS]

Committed r261254: <https://trac.webkit.org/changeset/261254>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398644 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62949317>

[Ryan Haddad]

(In reply to EWS from comment #32)
> Committed r261254: <https://trac.webkit.org/changeset/261254>
This appears to have broken webkitpy tests on the bots:
IOError: [Errno 2] No such file or directory: '/Volumes/Data/slave/catalina-release-tests-wk1/build/Source/WebKit/Scripts/webkit/test-messages.in'

https://build.webkit.org/builders/Apple-Catalina-Release-WK1-Tests/builds/5594

[Alex Christensen]

http://trac.webkit.org/r261270

[Chris Dumez]

Comment on attachment 398644 [details]
Patch

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

> Source/WebKit/Shared/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:-82
> -    m_client.paymentCoordinatorAddMessageReceiver(*this, Messages::WebPaymentCoordinatorProxy::messageReceiverName(), *this);

Who adds the message receiver now? I suspect this may be the cause of <rdar://problem/63161750>

[Chris Dumez]

(In reply to Chris Dumez from comment #36)
> Comment on attachment 398644 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398644&action=review
> 
> > Source/WebKit/Shared/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:-82
> > -    m_client.paymentCoordinatorAddMessageReceiver(*this, Messages::WebPaymentCoordinatorProxy::messageReceiverName(), *this);
> 
> Who adds the message receiver now? I suspect this may be the cause of
> <rdar://problem/63161750>

It was the reason. Fixing via https://bugs.webkit.org/show_bug.cgi?id=211826.

[Darin Adler]

There were two mistakes in this patch that led to a super-strange problem where generated files were malformed:

1) The rule in DerivedSources.make has multiple targets; so it needs to be a pattern rule, which is the kind of rule that make handles multiple targets correctly for. The rule was a pattern rule before this patch, but since there wasn’t a comment explaining that was important, this patch changed it to a non-pattern rule. This meant that the rule could run multiple times in parallel. Since make re-checks modification dates after running, it doesn’t always run once per generated file but in theory it could! Keith Rollin figured out it was running multiple times and fixed it in bug 215054.

2) There was a typo "addsufix" so the Messages.h files were not properly identified as targets to make. Make silently ignored the entire thing. I noticed the typo was the reason the files weren’t being regenerated on my computer when I deleted one and fixed it in bug 215064.

We should probably follow up by adding a comment to the makefile explaining why the rule needs to be a pattern rule.

[Alex Christensen]

Make is such a strange language.  Thanks for fixing it.