Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 7 years ago

For keyboard APZ scrolling we do this thing where every input event received on the main thread sends a scroll focus update message to APZ. This is scheduled via an empty transaction at [1] and the information is propagated to the layer manager at [2]. In cases where this is the only thing in the empty transaction, it's still a lot of work to do the empty transaction. I suspect this is what causes CPU usage to sit at around 20% when moving the mouse around on a static page, as reported in bug 1425916.

We can improve this by using a dedicated PLayerTransaction message for this update, instead of an empty transaction. We already do this with webrender [3], because we actually don't support proper empty transactions there.

[1] https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/layout/base/PresShell.cpp#6820-6825
[2] https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/layout/base/PresShell.cpp#6367
[3] https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/gfx/layers/wr/WebRenderLayerManager.cpp#207

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 7 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 7 years ago

So I spent some more time looking into this, and I'm not sure if my understanding of the current code is correct. I was expecting that every mousemove event would cause the code at [1] to run, which would then always send a transaction (either empty or full) to the compositor with the new sequence number, so that it would be ready for an keyboard events that arrive immediately after.

However, what actually seems to be happening is that after we build the empty transaction, we end up taking the early-exit condition at [2], which means we never actually forward the transaction to the compositor and so APZ ends up still having a stale sequence number. This state remains until we have enough stuff in the transaction to not take that early-exit condition. Generally this doesn't take very long, it looks like we do end up forwarding a transaction within less than a second of the mouse movement stopping. But as far as I can tell this isn't working the way it was intended to work, and it means that we're doing a lot of unnecessary work with all the SchedulePaint() calls that don't end up sending the new sequence number to the compositor.

Ryan, do you recall if this behaviour is expected?

[1] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/layout/base/PresShell.cpp#6909-6913
[2] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/gfx/layers/ipc/ShadowLayers.cpp#674

	Ryan Hunt [:rhunt]
	Comment 3 • 7 years ago

My memory on this is a little fuzzy. I think that my intent was for an empty transaction to be sent for each sequence number update, but I also seem to remember that it would coalesce them as you have discovered, and that seemed to work okay so I settled for that.

I could see this being an issue, I think if we sent out an empty transaction like I intended for every input event, it would've caused too much overhead to land. But you're right that just building an empty transaction and not sending it is too much overhead as well. For some reason I thought the coalescing happened before we built the transaction, but that's probably wrong.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 7 years ago

(In reply to Ryan Hunt [:rhunt] from comment #3)
> For some reason I thought the
> coalescing happened before we built the transaction, but that's probably
> wrong.

There is a level of coalescing that happens before we build the transaction too. I recall that we took that level of coalescing into account, and I think that part is working as intended. In that level of coalescing, we're supposed to squash the sequence number update into a full transaction if there is one happening, or an empty composite-only transaction if there isn't a full transaction happening.

However I think this behaviour that I'm observing is basically an oversight in our original design. It would probably manifest as falling back to main-thread keyboard scrolling in some cases where we might otherwise have been able to do APZ scrolling, but the window in which this happens is probably pretty small so we haven't noticed it in practice.

Anyway, that clears up my confusion. I'll see if there's a simpler way of reducing this CPU usage with this in mind.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 7 years ago

So after giving this some more thought it seems to me that if we just remove the SchedulePaint call [1] it should have no effect on correctness and reduce the CPU usage. If, in a given refresh driver tick, that SchedulePaint call is the only request to schedule a paint, then we'll end up doing all the work to compute an empty transaction, but then throw it away at the early-exit that I mentioned in comment 2. The only way we don't take the early-exit path is if there are other actual changes in the transaction, and in that case we'll be getting other calls to SchedulePaint from wherever those changes were made.

[1] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/layout/base/PresShell.cpp#6913

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 7 years ago

I took out the SchedulePaint but it didn't really seem to help the CPU usage numbers, at least on my OS X machine using Activity Monitor to watch CPU usage. I'll put the patch up anyway but we might have to look more into bug 1425916 and bug 1427113 to figure out what's going on there.

	Ryan Hunt [:rhunt]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8941165 [details]
Bug 1426653 - Stop calling SchedulePaint uselessly when bumping the APZ focus sequence number.

https://reviewboard.mozilla.org/r/211434/#review217268

	Pulsebot
	Comment 9 • 7 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/516bfb708680
Stop calling SchedulePaint uselessly when bumping the APZ focus sequence number. r=rhunt

	Raul Gurzau (:RaulG)
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/516bfb708680