Benoit Girard (:BenWa) Assignee
	Description • 8 years ago

Please refer the checkerboarding log. I've noticed this a bit with the minimap but with the checkoard log it's easier to see.

STR1: Scroll very aggressively on planet.mozilla.org using a mac trackpad.

1) Watch the requested display port (yellow) around event 19. At event 32 it goes backwards.
2) Watch the painted display port (green) at 52 it jumps backwards.

Going backwards at 52 means we wasted a lot of time painting something useless.

This is probably something to do with event coalescing and displayport prediction not handling this case very well.

	Benoit Girard (:BenWa) Assignee
	Comment 1 • 8 years ago

Here's a gif of the raw log playback to quickly get an idea of the problem.

	Benoit Girard (:BenWa) Assignee
	Comment 2 • 8 years ago

I had a quick look at this. I believe this is because the up-to-date UpdateFrame messages are getting queued behind the paint events. We used to have IPC compression on UpdateFrame however it was removed for sub-APZ because IPC compression was not flexible enough and was doing the wrong thing leading to bugs:
https://lists.mozilla.org/pipermail/dev-platform/2014-March/003708.html

We have two possible solutions:
1) Improve event compression and turn it back on similar to what botond suggested to dev.platform.
2) Finish https://bugzilla.mozilla.org/show_bug.cgi?id=1236035#c0

I believe #1 is easier and less risk however #2 is a better solution. #1 will still lead to a bad paint if we don't get an UpdateFrame message before the paint so we have nothing to compress to before the paint, #2 wouldn't suffer from this problem.

I'd like to explore if #1 is very simple to implement because it might be worth doing until we can do #2. If it's not simple then we should just do #2.

	Benoit Girard (:BenWa) Assignee
	Comment 3 • 8 years ago

I implemented a new IPDL compression mode that compressed to the first message instead of the last. I noticed that when we compress we're also checkerboarding at the same time which supports my theory above.

However I'm still seeing paints for stale displayport. I'll keep looking into why.

	Benoit Girard (:BenWa) Assignee
	Comment 4 • 8 years ago

Even with the 'compressfirst' mode I implemented we still end up with the required messaged sitting in the queue waiting to be processed and we paint using a stale displayport.

In this log we paint with (0,89322.1) when we have (16,86421.1) in the queue. That's a 2900 pixel difference which is more than my screen height and can account for a lot more checkerboarding than required.

	Benoit Girard (:BenWa) Assignee
	Comment 5 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/37695/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37695/

	Benoit Girard (:BenWa) Assignee
	Comment 6 • 8 years ago

The current set of patches don't provide a way to remove the message. We will probably want a way to do that.

	Benoit Girard (:BenWa) Assignee
	Comment 7 • 8 years ago

Comment on attachment 8725925 [details]
MozReview Request: Bug 1242609 - Implement PeekMessage to get some messages earlier. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37695/diff/1-2/

	Benoit Girard (:BenWa) Assignee
	Comment 8 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/37989/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37989/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 9 • 8 years ago

Comment on attachment 8726437 [details]
MozReview Request: Bug 1242609 - Use PeekMessages to get the most recent DisplayPort request. r?kats

https://reviewboard.mozilla.org/r/37989/#review34675

Some comments/tweaks below but for the most part it looks good. I couldn't think of any other flaws in the design, although it's probably worth discussing with Botond as well on Monday to make sure he agrees.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp:42
(Diff revision 1)
> -AdjustDisplayPortForScrollDelta(mozilla::layers::FrameMetrics& aFrameMetrics,
> +APZCCallbackHelper::AdjustDisplayPortForScrollDelta(
> +                      mozilla::layers::FrameMetrics& aFrameMetrics,
> -                                const CSSPoint& aActualScrollOffset)
> +                      const CSSPoint& aActualScrollOffset)

If you're reindenting these I'd prefer you indent them as 4 spaces from the start of the line rather than an arbitary "halfway down the line"

::: layout/base/nsLayoutUtils.h:189
(Diff revision 1)
> +  /**
> +   * Go through the IPC Channel and immediatly process any applicable
> +   * UpdateFrame message. The aim is to paint the most up-to-date
> +   * displayport without waiting for these message to go through
> +   * the message queue.
> +   */
> +  static void UpdateAPZMetrics();

This comment implies that we will actually process the UpdateFrame which is not really true. I'd prefer the first sentence read as "Go through the IPC Channel and update displayport margins for content elements based on UpdateFrame messages."

Also s/immediatly/immediately/ if you choose to keep that word.

I think the function might also be better named "UpdateDisplayPortMarginsFromPendingMessages" even though it's longer it's more reflective of what it's doing.

::: layout/base/nsLayoutUtils.cpp:9015
(Diff revision 1)
> +static void UpdateFrameMetrics(FrameMetrics& aMetrics) {

Rename to UpdateDisplayPortMarginsForPendingMetrics?

::: layout/base/nsLayoutUtils.cpp:9028
(Diff revision 1)
> +  // Pres shell check
> +  // up to line 243
> +  // apzScrollOffset and the gecko one aFrame->GetScrollPosition().
> +  // we can call SetDisplayPortMargisn with an adjusted frame metrics
> +  // adjust the DP margin by the delta in the scrolloffsets
> +  //
> +  //

Drop this comment, seems left over from when we were discussing things.

::: layout/base/nsLayoutUtils.cpp:9035
(Diff revision 1)
> +  if (gfxPrefs::APZAllowZooming()) {

Add "&& aMetrics.IsRootContent()" to this condition - we only want to do this resolution check for the root content document's metrics.

::: layout/base/nsLayoutUtils.cpp:9043
(Diff revision 1)
> +  nsIScrollableFrame* frame = nsLayoutUtils::FindScrollableFrameFor(aMetrics.GetScrollId());
> +

We need to add the generation/main-thread-scroll checks here as we discussed:

if (frame->IsProcessingAsyncScroll() ||
    nsLayoutUtils::CanScrollOriginClobberApz(frame->LastScrollOrigin()) ||
    frame->LastSmoothScrollOrigin()) {
  // If these conditions are true, then the UpdateFrame
  // message may be ignored by the main-thread, so we
  // shouldn't update the displayport based on it.
  return;
}

::: layout/base/nsLayoutUtils.cpp:9059
(Diff revision 1)
> +void

/* static */ at the start of line

::: layout/base/nsLayoutUtils.cpp:9074
(Diff revision 1)
> +
> +        return;

Unnecessary return statement since the function returns void

::: layout/base/nsRefreshDriver.cpp:1671
(Diff revision 1)
> +  // We want to process any pending APZ metrics ahead of their positions
> +  // in the queue. This will prevent us from spending precious time
> +  // painting a stale displayport.
> +  nsLayoutUtils::UpdateAPZMetrics();

I would move this call down a few more lines, to just after the UpdateForDeviceReset(). I think we want to count it as part of the tick duration, for one thing, and secondly we want to do it as late as possible to get the most benefit from it.

	Benoit Girard (:BenWa) Assignee
	Comment 10 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> This comment implies that we will actually process the UpdateFrame which is
> not really true. I'd prefer the first sentence read as "Go through the IPC
> Channel and update displayport margins for content elements based on
> UpdateFrame messages."
> 
> Also s/immediatly/immediately/ if you choose to keep that word.
> 
> I think the function might also be better named
> "UpdateDisplayPortMarginsFromPendingMessages" even though it's longer it's
> more reflective of what it's doing.
> 

That name is much better. I also added 'The messages are left in the queue and will be fully processed when dequeued.' since I felt it was a bit ambiguous what happened to the messages after.

> 
> Drop this comment, seems left over from when we were discussing things.

Ahh opps, I forgot to self-review this before posting :(.

	Benoit Girard (:BenWa) Assignee
	Comment 11 • 8 years ago

Compile error from bug 1253678 is blocking this patch.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 12 • 8 years ago

Comment on attachment 8725925 [details]
MozReview Request: Bug 1242609 - Implement PeekMessage to get some messages earlier. r?billm

https://reviewboard.mozilla.org/r/37695/#review34821

::: ipc/glue/MessageChannel.h:111
(Diff revision 2)
> +    // Call aInvoke for each pending message of type aId.

Please put something to the effect of "You must get permission from an IPC peer to use this function." It effectively forces people to do their own deserialization, which is pretty dangerous.

::: ipc/glue/MessageChannel.cpp:771
(Diff revision 2)
> +MessageChannel::PeekMessages(msgid_t aMsgId, mozilla::Function<void(Message& aMsg)> aInvoke) {

Can we pass the Message const?

::: ipc/glue/MessageChannel.cpp:772
(Diff revision 2)
> +    for (MessageQueue::iterator it = mPending.begin(); it != mPending.end(); ) {

Why not put the it++ here?

	Benoit Girard (:BenWa) Assignee
	Comment 13 • 8 years ago

(In reply to Bill McCloskey (:billm) from comment #12)
> Comment on attachment 8725925 [details]
> MozReview Request: Bug 1242609 - Implement PeekMessage to get some messages
> earlier. r?billm
> 
> https://reviewboard.mozilla.org/r/37695/#review34821
> 
> ::: ipc/glue/MessageChannel.h:111
> (Diff revision 2)
> > +    // Call aInvoke for each pending message of type aId.
> 
> Please put something to the effect of "You must get permission from an IPC
> peer to use this function." It effectively forces people to do their own
> deserialization, which is pretty dangerous.

Yea, I was thinking of adding Getter on the messages via the IPDL.py generator. This would be safe but would cause a lot of code bloat for something that's rarely use. So I decided to use custom deserialization. But yea I'm not overly happy about it because it means that if we change the IDL we might not update that location and won't see a compile error so it's not great.

I'll add the warning.

> 
> ::: ipc/glue/MessageChannel.cpp:771
> (Diff revision 2)
> > +MessageChannel::PeekMessages(msgid_t aMsgId, mozilla::Function<void(Message& aMsg)> aInvoke) {
> 
> Can we pass the Message const?

I'll check. If the IPC::Read function is const then there wont be a problem.

> 
> ::: ipc/glue/MessageChannel.cpp:772
> (Diff revision 2)
> > +    for (MessageQueue::iterator it = mPending.begin(); it != mPending.end(); ) {
> 
> Why not put the it++ here?

Just mirroring something else but I'll change it.

	Benoit Girard (:BenWa) Assignee
	Comment 14 • 8 years ago

Comment on attachment 8725925 [details]
MozReview Request: Bug 1242609 - Implement PeekMessage to get some messages earlier. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37695/diff/2-3/

	Benoit Girard (:BenWa) Assignee
	Comment 15 • 8 years ago

Comment on attachment 8726437 [details]
MozReview Request: Bug 1242609 - Use PeekMessages to get the most recent DisplayPort request. r?kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37989/diff/1-2/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 16 • 8 years ago

Comment on attachment 8726437 [details]
MozReview Request: Bug 1242609 - Use PeekMessages to get the most recent DisplayPort request. r?kats

https://reviewboard.mozilla.org/r/37989/#review34951

::: layout/base/nsLayoutUtils.cpp:9071
(Diff revision 2)
> +  nsIScrollableFrame* frame = nsLayoutUtils::FindScrollableFrameFor(aMetrics.GetScrollId());

This still needs the scroll generation checks like I mentioned before (I reopened the issue in MozReview since it's not fixed)

::: layout/base/nsLayoutUtils.cpp:9079
(Diff revision 2)
> +  // CHECK PRESSHELL ID

Is this a TODO?

	Benoit Girard (:BenWa) Assignee
	Comment 18 • 8 years ago

Comment on attachment 8725925 [details]
MozReview Request: Bug 1242609 - Implement PeekMessage to get some messages earlier. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37695/diff/3-4/

	Benoit Girard (:BenWa) Assignee
	Comment 19 • 8 years ago

Comment on attachment 8726437 [details]
MozReview Request: Bug 1242609 - Use PeekMessages to get the most recent DisplayPort request. r?kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37989/diff/2-3/

	Benoit Girard (:BenWa) Assignee
	Comment 20 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7fc920894f0

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 21 • 8 years ago

Comment on attachment 8726437 [details]
MozReview Request: Bug 1242609 - Use PeekMessages to get the most recent DisplayPort request. r?kats

https://reviewboard.mozilla.org/r/37989/#review36375

This looks good to me. The only other suggestion I have is to maybe guard this with a pref? apz.peek_messages or something, so we can disable it if needed.

	Benoit Girard (:BenWa) Assignee
	Comment 22 • 8 years ago

Comment on attachment 8725925 [details]
MozReview Request: Bug 1242609 - Implement PeekMessage to get some messages earlier. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37695/diff/4-5/

	Benoit Girard (:BenWa) Assignee
	Comment 23 • 8 years ago

Comment on attachment 8726437 [details]
MozReview Request: Bug 1242609 - Use PeekMessages to get the most recent DisplayPort request. r?kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37989/diff/3-4/

	Benoit Girard (:BenWa) Assignee
	Comment 24 • 8 years ago

Done, and queued for landing

	Pulsebot
	Comment 25 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5863b88e10cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a6e000c7a42

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5863b88e10cc
https://hg.mozilla.org/mozilla-central/rev/6a6e000c7a42

	Jim Mathies [:jimm]
	Comment 27 • 8 years ago

I don't understand why we put this comment here - 

114     // XXX: You must get permission from an IPC peer to use this function
115     //      since it requires custom deserialization and re-orders events.

AFAICT this method doesn't cause "custom deserialization and re-orders events", the risk here is that callers might need to do these things in processing msgs. Correct? Simple uses of this (searching for a particular message) are completely safe. (See bug 1255968)