MAPLE: make getViewTransform fast

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, blocking-fennec1.0 +)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 602499 [details] [diff] [review]
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.
Assignee: nobody → jmuizelaar
Attachment #602499 - Flags: review?(bugmail.mozilla)
4,200%
(Assignee)

Comment 3

6 years ago
Created attachment 602501 [details] [diff] [review]
Include ImmutableViewportMetrics
Attachment #602501 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

6 years ago
Attachment #602499 - Attachment is obsolete: true
Attachment #602499 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 4

6 years ago
Oops, I computed the wrong median. It should've been 0.061ms.
10,400%
(Assignee)

Updated

6 years ago
Blocks: 732486
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.
Attachment #602501 - Flags: review?(bugmail.mozilla) → review-
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.
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 603295 [details] [diff] [review]
Include ImmutableViewportMetrics v2

Addresses all review comments except the one about the location of
ViewTransform mCurrentViewTransform;
Attachment #602501 - Attachment is obsolete: true
Attachment #603295 - Flags: review?(bugmail.mozilla)
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).
Attachment #603295 - Flags: review?(bugmail.mozilla) → review+
blocking-fennec1.0: --- → ?
This already landed.

http://hg.mozilla.org/projects/maple/rev/730e026a2179
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
blocking-fennec1.0: ? → +
status-firefox15: --- → fixed

Updated

5 years ago
status-firefox14: --- → fixed

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.