Last Comment Bug 748649 - Add PLayers async NoSwapUpdate
: Add PLayers async NoSwapUpdate
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Benoit Girard (:BenWa)
: Milan Sreckovic [:milan]
Depends on: 747811 754056
Blocks: 749062 829747
  Show dependency treegraph
Reported: 2012-04-24 20:55 PDT by Benoit Girard (:BenWa)
Modified: 2013-01-11 13:15 PST (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (6.61 KB, patch)
2012-04-24 20:56 PDT, Benoit Girard (:BenWa)
cjones.bugs: review+
Details | Diff | Splinter Review
patch (6.91 KB, patch)
2012-05-01 12:24 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
bug 747811 work-around (1.69 KB, patch)
2012-05-01 12:36 PDT, Benoit Girard (:BenWa)
ajuma.bugzilla: review+
Details | Diff | Splinter Review
bug 747811 work-around (2.00 KB, patch)
2012-05-01 13:00 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-04-24 20:55:45 PDT
With the introduction of TiledThebesLayer we no longer have to swap for that layer type. This patch introduce an async NoSwapUpdate when we can guarantee that no EditReply will be generated for the transaction.

I haven't test this on mobile but I will early tomorrow. It work well on Mac. FYI we block content on average 5-15ms per layers update and most update on sites without a changing canvas will benefit from this change.

This may also be an option to move towards async layers update one layer type/back end a time without a sweeping refactor.
Comment 1 Benoit Girard (:BenWa) 2012-04-24 20:56:37 PDT
Created attachment 618162 [details] [diff] [review]
Comment 2 Benoit Girard (:BenWa) 2012-04-25 14:31:00 PDT
The risk is moderate and the performance win is significant over many paints (esp. smaller paints).
Comment 3 Benoit Girard (:BenWa) 2012-04-25 15:33:52 PDT
This should make you happy Chris, to support this properly we need to fix bug 747811 otherwise a get a race. Patch is already underway. Let's get this reviewed in the meantime since it wont effect this patch in the slightest.
Comment 4 JP Rosevear [:jpr] 2012-04-27 11:58:02 PDT
We don't require this at the moment, but we may come back to it if we still aren't good enough and it shakes out on m-c first.
Comment 5 Benoit Girard (:BenWa) 2012-04-27 23:11:53 PDT
If you don't have time to look at the patch review Chris do you think Ali could review it? He knows the shadow layers code very well.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-27 23:46:32 PDT
I'll be able to look tomorrow.  Something came up today.
Comment 7 Benoit Girard (:BenWa) 2012-04-30 13:25:05 PDT
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-30 18:22:31 PDT
Comment on attachment 618162 [details] [diff] [review]

>diff --git a/gfx/layers/ipc/PLayers.ipdl b/gfx/layers/ipc/PLayers.ipdl

>+  // We don't need to send a sync transaction if
>+  // no transaction operate require a swap.
>+  async NoSwapUpdate(Edit[] cset, bool isFirstPaint);

Nit: |UpdateNoSwap()|.

Yuck ... I thought the isFirstPaint turd went away :/.

r=me with naming change.  Looks good!
Comment 9 Benoit Girard (:BenWa) 2012-05-01 12:24:24 PDT
Created attachment 620015 [details] [diff] [review]
Comment 10 Benoit Girard (:BenWa) 2012-05-01 12:36:36 PDT
Created attachment 620028 [details] [diff] [review]
bug 747811 work-around
Comment 11 Ali Juma [:ajuma] 2012-05-01 12:42:56 PDT
Comment on attachment 620028 [details] [diff] [review]
bug 747811 work-around

Review of attachment 620028 [details] [diff] [review]:

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +131,5 @@
>  void
>  TiledThebesLayerOGL::PaintedTiledLayerBuffer(const BasicTiledLayerBuffer* mTiledBuffer)
>  {
>    mMainMemoryTiledBuffer = *mTiledBuffer;
> +  delete mTiledBuffer;

Assuming this is also temporary, add a comment here too.
Comment 12 Benoit Girard (:BenWa) 2012-05-01 13:00:30 PDT
Created attachment 620034 [details] [diff] [review]
bug 747811 work-around
Comment 15 Matt Brubeck (:mbrubeck) 2012-05-08 14:22:42 PDT
It looks like this caused a 53% improvement in the DHTML NoChrome benchmark on Android Native.  Should we consider taking this on Aurora?
Comment 16 Benoit Girard (:BenWa) 2012-05-08 15:57:17 PDT
It seems to be it:[[19,63,20]]&sel=none&displayrange=7&datatype=running

I'm guessing this benchmark does a lot of small-ish painting? I'd be willing to uplift this if it bakes well on trunk.

Note You need to log in before you can comment on or make changes to this bug.