732576 - MAPLE: make getViewTransform fast

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make getViewTransform fast

This removes the logging, locking and allocations from getViewTransform.

This reduces the time spent from an median of 6.3ms to 0.15ms

We use a new scheme where the view transform is immutable and the member variable containing it is atomically overwritten. So we may get a slightly old view transform but this won't be a problem.

Comment 2

[Joe Drew (not getting mail)]

4,200%

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Include ImmutableViewportMetrics

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Oops, I computed the wrong median. It should've been 0.061ms.

Comment 5

[Joe Drew (not getting mail)]

10,400%

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 602501 [details] [diff] [review]
Include ImmutableViewportMetrics

Review of attachment 602501 [details] [diff] [review]:
-----------------------------------------------------------------

I have a bunch of nits/minor issues below, but overall the approach is good. We'll need additional cleanup patches later but those should wait until we land the other patches (bug 732091 and bug 732564), and the cleanup can even be done as part of bug 727352. r- for now but it should be simple enough to address the comments below.

::: mobile/android/base/GeckoApp.java
@@ +1407,4 @@
>                  if (tab == null)
>                      return;
>  
> +                ViewportMetrics targetViewport = new ViewportMetrics(mLayerController.getViewportMetrics());

If you change PluginLayoutParameters to take an ImmutableViewportMetrics instead of a ViewportMetrics you shouldn't need to create a mutable version here. It's only a few lines of code that needs to be changed, so I'd say do it.

@@ +1602,4 @@
>      }
>  
>      public void repositionPluginViews(Tab tab, boolean setVisible) {
> +        ViewportMetrics targetViewport = new ViewportMetrics(mLayerController.getViewportMetrics());

Ditto.

::: mobile/android/base/Makefile.in
@@ +136,4 @@
>    gfx/TileLayer.java \
>    gfx/ViewTransform.java \
>    gfx/ViewportMetrics.java \
> +  gfx/ImmutableViewportMetrics.java \

Place in lexicographic sort order.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +379,5 @@
> +    /* This functions needs to be fast because it is called by the compositor every frame.
> +     * It avoids taking any locks or allocating any objects. We keep around a
> +     * mCurrentViewTransform so we don't need to allocate a new ViewTransform
> +     * everytime we're called. */
> +    ViewTransform mCurrentViewTransform;

Make mCurrentViewTransform private and move it up to the top of the file with the other instance variable declarations.

::: mobile/android/base/gfx/ImmutableViewportMetrics.java
@@ +1,3 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Use MPL 2.0 license block now.

@@ +39,5 @@
> +
> +import android.graphics.PointF;
> +import android.graphics.RectF;
> +import org.mozilla.gecko.gfx.FloatSize;
> +import org.mozilla.gecko.gfx.ViewportMetrics;

Imports are not needed for classes in the same package (yeah, there are already spurious imports in some of the other files but we shouldn't add more).

::: mobile/android/base/gfx/LayerController.java
@@ +82,5 @@
> +
> +    /* This is volatile so that we can read and write to it from different threads.
> +     * We avoid synchronization to make getting the viewport metrics from
> +     * the compositor as cheap as possible. The viewport is immutable so
> +     * we don't need to worry about anyone mutating it while we're reading from it. */

I think comment needs to be a little more detailed with respect to describing what is allowed. Specifically it needs to say that:
(1) reading the mViewportMetrics variable from any thread is fine without synchronization
(2) writing to the mViewportMetrics variable requires synchronizing on the layer controller object
(3) whenever reading multiple fields from mViewportMetrics without synchronization (i.e. in case 1 above) you should always first grab a local copy of the reference, and then use that because mViewportMetrics might get reassigned in between reading the different fields.
The other stuff about reading it as cheaply as possible from the compositor and it being immutable is fine.

@@ +233,2 @@
>  
> +        Log.d(LOGTAG, "setViewportSize: " + viewportMetrics);

For consistency, move the ImmutableViewportMetrics creation to after the log line.

@@ +264,1 @@
>              return;

Call getPageSize() on mViewportMetrics and don't create the mutable version if we take the early-return path.

@@ +289,1 @@
>          Log.d(LOGTAG, "setViewportMetrics: " + mViewportMetrics);

Log viewport instead of mViewportMetrics here, since ImmutableViewportMetrics doesn't have a sane toString(). Same for any other places in this file we're still logging mViewportMetrics (e.g. scaleWithFocus). Alternatively, add a toString() to ImmutableViewportMetrics that prints out the fields. That might actually be better in case we need to dump it in other places later.

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +49,4 @@
>  import org.mozilla.gecko.gfx.IntSize;
>  import org.mozilla.gecko.gfx.LayerController;
>  import org.mozilla.gecko.gfx.RectUtils;
> +import org.mozilla.gecko.gfx.ImmutableViewportMetrics;

Import not needed here.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Oh wow splinter sucks at giving the right context.

> @@ +233,2 @@
> >  
> > +        Log.d(LOGTAG, "setViewportSize: " + viewportMetrics);
> 
> For consistency, move the ImmutableViewportMetrics creation to after the log
> line.

This is in the setViewportSize() function.

> @@ +264,1 @@
> >              return;
> 
> Call getPageSize() on mViewportMetrics and don't create the mutable version
> if we take the early-return path.

This is in the setPageSize() function.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Kartikaya Gupta (:kats) from comment #6)
> 
> Make mCurrentViewTransform private and move it up to the top of the file
> with the other instance variable declarations.

I put it there because ideally it would only be visible to getViewTransform() but java doesn't support that. If you think it makes more sense to move it up I can do that.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: Include ImmutableViewportMetrics v2

Addresses all review comments except the one about the location of
ViewTransform mCurrentViewTransform;

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 603295 [details] [diff] [review]
Include ImmutableViewportMetrics v2

Review of attachment 603295 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I think you should mCurrentViewTransform up to the top of the file for consistency with general Java coding standards. I agree it would be useful to limit the scope but we can't do that in Java. Also in GeckoLayerClient.getViewTransform() please take out the log lines entirely rather than just commenting them out (if BenWa hasn't taken them out already).

Comment 11

[Kartikaya Gupta (email:[e-mail])]

This already landed.

http://hg.mozilla.org/projects/maple/rev/730e026a2179