Having more control over when an IPDL message is compressed

[Botond Ballo]

Hello dev-platform,

I recently fixed an APZ bug [1] that was caused by an IPDL message,
PBrowser::UpdateFrame, being compressed when it shouldn't have been.

I think the compression was correct back when we didn't have subframe
scrolling, and so all messages pertained to the same frame, in which
case it's appropriate to only act on the most recent message. With
subframe scrolling, subsequent messages could apply to different
frames, and dropping all but the most recent message could lead to a
frame missing an important update.

I fixed the bug by getting rid of the compression, but it occurred to
me that if we had a mechanism for specifying that two messages
applying to the same frame could be suppressed, but two messages
applying to different frames could not, we'd probably want to use it.

Is there any interest in adding such a mechanism to IPDL?

Here's a straw man proposal for how it could work. Obviously, the
syntax is not the interesting part and I'm happy to change it to
whatever other syntax people might like:

In addition to having a 'compress' attribute, have a 'compress_if'
attribute that takes a function name as argument. Such a function
would be expected to be provided in C++ code, to have 2*n arguments,
where 'n' is the argument to the message, and to return 'bool'
indicating whether or not two messages with those arguments should
be suppressed.

For example:

RetType myMessage(Foo foo, Bar bar) compress_if(CompressMyMessage);

And then one would provide:

bool CompressMyMessage(const Foo& foo1, const Bar& bar1,
const Foo& foo2, const Bar& bar2)
{
// whatever
}

which would decide whether or not myMessage(foo1, bar1) and
myMessage(foo2, bar2) should be compressed.

For example, for UpdateFrame we could do:

UpdateFrame(FrameMetrics frame) compress_if(CompressUpdateFrame);

and provide:

bool CompressUpdateFrame(const FrameMetrics& frame1,
const FrameMetrics& frame2)
{
// Two UpdateFrame messages can be compressed if they
// apply to the same frame.
return frame1.mScrollId == frame2.mScrollId;
}

I can think of at least one other use case in code that I'm
currently working on: for bug 976605 [2], I'd find it
convenient to conditionally compress PBrowser::RealTouchMoveEvent,
to make sure that certain touch-move events (those with a
particular flag set which is important for the child side to
know about) are not compressed away.

Presumably there will be other use cases in other protocols as well.

Any thoughts about this / interest in this?

Cheers,
Botond

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=984673
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=976605

[Trevor Saunders]

On Mon, Mar 24, 2014 at 03:18:19PM -0700, Botond Ballo wrote:
> Hello dev-platform,
>
> I recently fixed an APZ bug [1] that was caused by an IPDL message,
> PBrowser::UpdateFrame, being compressed when it shouldn't have been.
>
> I think the compression was correct back when we didn't have subframe
> scrolling, and so all messages pertained to the same frame, in which
> case it's appropriate to only act on the most recent message. With
> subframe scrolling, subsequent messages could apply to different
> frames, and dropping all but the most recent message could lead to a
> frame missing an important update.
>
> I fixed the bug by getting rid of the compression, but it occurred to
> me that if we had a mechanism for specifying that two messages
> applying to the same frame could be suppressed, but two messages
> applying to different frames could not, we'd probably want to use it.
>
> Is there any interest in adding such a mechanism to IPDL?

seems reasonable enough.

> Here's a straw man proposal for how it could work. Obviously, the
> syntax is not the interesting part and I'm happy to change it to
> whatever other syntax people might like:
>
> In addition to having a 'compress' attribute, have a 'compress_if'
> attribute that takes a function name as argument. Such a function
> would be expected to be provided in C++ code, to have 2*n arguments,
> where 'n' is the argument to the message, and to return 'bool'
> indicating whether or not two messages with those arguments should
> be suppressed.
>
> For example:
>
> RetType myMessage(Foo foo, Bar bar) compress_if(CompressMyMessage);
>
> And then one would provide:
>
> bool CompressMyMessage(const Foo& foo1, const Bar& bar1,
> const Foo& foo2, const Bar& bar2)
> {
> // whatever
> }

There's a part of me that wants to do this with a well known name for a
static method on the actor (though I guess that's tricky since the code
gen doesn't currently know the static type of the actor) but then you
could write

bool
FooChild::CoalesceMyMessage(const Bar& aM1, const bar& aM2)
{
return something or other;
}

Then we could use this same mechanism to implement always compressing,
but maybe that's too magical...

Trev

>
> which would decide whether or not myMessage(foo1, bar1) and
> myMessage(foo2, bar2) should be compressed.
>
> For example, for UpdateFrame we could do:
>
> UpdateFrame(FrameMetrics frame) compress_if(CompressUpdateFrame);
>
> and provide:
>
> bool CompressUpdateFrame(const FrameMetrics& frame1,
> const FrameMetrics& frame2)
> {
> // Two UpdateFrame messages can be compressed if they
> // apply to the same frame.
> return frame1.mScrollId == frame2.mScrollId;
> }
>
> I can think of at least one other use case in code that I'm
> currently working on: for bug 976605 [2], I'd find it
> convenient to conditionally compress PBrowser::RealTouchMoveEvent,
> to make sure that certain touch-move events (those with a
> particular flag set which is important for the child side to
> know about) are not compressed away.
>
> Presumably there will be other use cases in other protocols as well.
>
> Any thoughts about this / interest in this?
>
> Cheers,
> Botond
>
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=984673
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=976605

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Kyle Huey]

On Tue, Mar 25, 2014 at 6:18 AM, Botond Ballo <bba...@mozilla.com> wrote:
> Hello dev-platform,
>
> I recently fixed an APZ bug [1] that was caused by an IPDL message,
> PBrowser::UpdateFrame, being compressed when it shouldn't have been.
>
> I think the compression was correct back when we didn't have subframe
> scrolling, and so all messages pertained to the same frame, in which
> case it's appropriate to only act on the most recent message. With
> subframe scrolling, subsequent messages could apply to different
> frames, and dropping all but the most recent message could lead to a
> frame missing an important update.
>
> I fixed the bug by getting rid of the compression, but it occurred to
> me that if we had a mechanism for specifying that two messages
> applying to the same frame could be suppressed, but two messages
> applying to different frames could not, we'd probably want to use it.
>
> Is there any interest in adding such a mechanism to IPDL?
>

> Here's a straw man proposal for how it could work. Obviously, the
> syntax is not the interesting part and I'm happy to change it to
> whatever other syntax people might like:
>
> In addition to having a 'compress' attribute, have a 'compress_if'
> attribute that takes a function name as argument. Such a function
> would be expected to be provided in C++ code, to have 2*n arguments,
> where 'n' is the argument to the message, and to return 'bool'
> indicating whether or not two messages with those arguments should
> be suppressed.
>
> For example:
>
> RetType myMessage(Foo foo, Bar bar) compress_if(CompressMyMessage);
>
> And then one would provide:
>
> bool CompressMyMessage(const Foo& foo1, const Bar& bar1,
> const Foo& foo2, const Bar& bar2)
> {
> // whatever
> }
>

The name "compression" is unfortunate here. It took me some time to
understand what the real issue is.

That said, it seems like the "clean" architectural solution here is to
create a new actor for every combination of parameters that you do not
want to "compress" messages for, and leave as function parameters
every parameter that you do want to compress messages for. That may
not scale well, but in this case I think it would just mean creating
an actor for every (sub?)frame. Have we tried that? Is the
performance cost too high?

- Kyle

[Botond Ballo]

> The name "compression" is unfortunate here. It took me some time to
> understand what the real issue is.

Agreed. (Any ideas for a better word? For me 'supersede' comes to mind,
as in a newer message supersedes an older one so the older one can be
dropped, but I don't think that would be very clear either.)

> That said, it seems like the "clean" architectural solution here is to
> create a new actor for every combination of parameters that you do not
> want to "compress" messages for, and leave as function parameters
> every parameter that you do want to compress messages for. That may
> not scale well, but in this case I think it would just mean creating
> an actor for every (sub?)frame. Have we tried that? Is the
> performance cost too high?

Interesting idea. My intuition tells me that this would come with a lot
of overhead, as right now we have a single TabChild per browser tab
(persistent across loading new pages in the tab), while scrollable
frames are specific to each page (and sometimes come and go while
navigating a single page). It also seems like having to coordinate the
creation and destruction of these actors would add complexity, but I'm
not very familiar with IPDL so I could be wrong.

My immediate interest in this has diminished somewhat as my other use
case for conditional compression (PBrowser::RealTouchMoveEvent) has
gone away due to a change in approach for bug 976605, and I don't think
the overhead of not compressing multiple UpdateFrame messages for the
same frame is enough to justify doing it just for that. I'll keep my
eyes open for other uses cases, though, and keep your suggestion of
creating more actors in mind.

Cheers,
Botond

[Boris Zbarsky]

On 3/26/14 10:13 PM, Botond Ballo wrote:
>> The name "compression" is unfortunate here. It took me some time to
>> understand what the real issue is.
>
> Agreed. (Any ideas for a better word? For me 'supersede' comes to mind,
> as in a newer message supersedes an older one so the older one can be
> dropped, but I don't think that would be very clear either.)

"coalesced"?

-Boris