Closed Bug 1397823 Opened 3 years ago Closed 3 years ago

reduce code size for IPC::Message creation, redux

Categories

(Core :: IPC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(4 files)

I feel like we keep doing this, and we win 100-200k every time, but we never
manage to drive the IPC code size down to zero.

Anyway, this is the latest attempt.
_generateMessageConstructor takes a lot of `md.FOO`-style parameters,
which could be derived inside the function by simply passing `md`.
Especially with the upcoming changes to calculate things like reply-ness
of messages, sync-ness, etc, we'd be wanting to pass even more
parameters like `md.FOO`.  So let's just pass `md` in, and then we can
make all the necessary future changes in a single place.
Attachment #8905582 - Flags: review?(wmccloskey)
There's no need to be repeating 'IPC::Message::' prefixes or spreading
around more ExprVar calls than we need here.  Let's try to improve the
signal-to-noise ratio of this code by introducing a helper function to
inject some of the boilerplate for us.
Attachment #8905583 - Flags: review?(wmccloskey)
The current IPC::Message constructor takes a large number of arguments,
three of which--the nesting level, the priority, and the
compression--are almost always constant by virtue of the vast majority
of Message construction being done by auto-generated IPDL code.  But
then we take these constant values into the Message constructor, we
check them for various values, and then based on those values, we
perform a bunch of bitfield operations to store flags based on those
values.  This is wasted work.

Furthermore, for replies to IPDL messages, we'll construct a Message
object, and then call mutating setters on the Message object that will
perform even more bitfield manipulations.  Again, these operations are
performing tasks at runtime that are the same every single time, and use
information we already have at compile time.

The impact of these extra operations is not large, maybe 15-30K of extra
code, depending on platform.  Nonetheless, we can easily make them go
away, and make everything cleaner to boot.

This patch adds a HeaderFlags class that encapsulates all the knowledge
about the various kinds of flags Message needs to know about.  We can
construct HeaderFlags objects with strongly-typed enum arguments for the
various kinds of flags, and the compiler can take care of folding all of
those flags together into a constant when possible (and it is possible
for all the IPDL-generated code that instantiates Messages).  The upshot
is that we do no unnecessary work in the Message constructor itself.  We
can also remove various mutating operations on Message, as those
operations were only there to support post-constructor flag twiddling,
which is no longer necessary.
Attachment #8905584 - Flags: review?(wmccloskey)
Each protocol in IPDL has a bunch of autogenerated functions that
instantiate IPC::Message with various parameters.  Each of these
functions, then:

1) Pays the cost of calling malloc()
2) Setting up various parameters
3) Calling IPC::Message()

There's no reason that we should be duplicating 1) across all of these
autogenerated functions.  In step 2), several of the parameters we're
setting up are common across all or nearly all calls: the message
segment size is almost always zero, and we're always indicating that
IPDL-generated messages should be recorded in telemetry.

Instead of duplicating that code several thousand times, we can add a
small helper function that takes the only interesting parameters for an
IPDL message.  This helper function can then deal with calling malloc in
a single place and setting up the common parameters.  For messages that
require a custom segment size, we'll have to use the old scheme, but
such messages are uncommon.

The previous changes are not required for this scheme to work, but they
do help significantly, as the helper function (Message::IPDLMessage) can
now take four parameters, which ensures that its arguments are passed
solely in registers on Win64 and ARM.  The wins from this change are
also larger than they would be without the previous parts: ~100K on
x86-64 Linux (!)  and ~80K on ARM Android.
Attachment #8905586 - Flags: review?(wmccloskey)
Kan-ru, I'd like to get these landed, maybe even for 57; is it possible for you to review these while Bill is out?
Flags: needinfo?(kchen)
Ah, I missed this. I'll review now but maybe it's too late for 57 :/
Flags: needinfo?(kchen)
Attachment #8905582 - Flags: review?(wmccloskey) → review+
Attachment #8905583 - Flags: review?(wmccloskey) → review+
Attachment #8905584 - Flags: review?(wmccloskey) → review+
Attachment #8905586 - Flags: review?(wmccloskey) → review+
Thanks for the patch! With bug 1397456 we can shave a bit more off :)
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1607f6d53f52
part 1 - move work into _generateMessageConstructor; r=kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7da31ab0b1
part 2 - tidy _generateMessageConstructor enums a little; r=kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/14acffc2e1ec
part 3 - do a better job setting IPC::Message flags; r=kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/447eaf2c0578
part 4 - reduce codesize for IPDL IPC::Message creation; r=kanru
Depends on: 1401305
You need to log in before you can comment on or make changes to this bug.