1039898 - [meta] Avoid generating large amounts of proxy accessor methods

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Kitching [:ckitching]]

Accessing a private member of the enclosing class from an inner class results in sadness:
http://developer.android.com/training/articles/perf-tips.html#PackageInner

How much such sadness does Fennec have?

As luck would have it, I spent much of last year writing an optimising compiler for Java, so have a convenient system for performing dataflow analysis on Java programs handy.

So, I wrote a module to print out all private members that are referenced in this way and hence causing wrapper methods to be generated.

I then wrote a sed script to generate a patch I'm now proposing to land. This change reduces final apk size by around 60k (the earlier incorrectly-claimed figure was referring to the change in bytecode size prior to packaging. Whoops). It changes nothing except access modifiers. It's green on try:
https://tbpl.mozilla.org/?tree=Try&rev=4b562c067206

It should also produce a trivial improvement on performance by reducing call overheads (but probably not anywhere particularly hot).

Since my Dropbox is busy killing itslef, there'll be a short delay before I upload the scripts and JOUST output used to generate the patch (you'll probably want to wait for those before reviewing)

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Reduce use of generated proxies

The patch. (version without comments).

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Reduce use of generated proxies (with verbose comments)

And with comments.

I'm unsure which I prefer. Perhaps tweak this one to have a little bit more of a happy medium?

In any case, it seems like we're going to have a hard time preventing regressions without comments...

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8457728 [details] [diff] [review]
Reduce use of generated proxies (with verbose comments)

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

Might I suggest:

* Separate patches for methods and fields, if that helps with the clarity.
* Separate patches for each major subdirectory.
* Doing all of the android-sync stuff upstream, so we don't have to upstream this patch.
* Improving spacing, if we can.

An alternative to improving spacing is to make the comment smaller. This is a common idiom:

  /* package */ boolean doFoo() {

so we could either do

  /* package: inner */ boolean ...

or 

  /* inner-access */ protected boolean ...

I'd like to be able to walk through some of these and eliminate the need for the inner class access, though.

::: mobile/android/base/GeckoAccessibility.java
@@ +44,4 @@
>      // Used to store the JSON message and populate the event later in the code path.
> +    // Referenced from inner class: do not make private.
> +    protected static JSONObject sEventMessage = null;
> +    // Referenced from inner class: do not make private.

I'd rather this had sane line spacing.

Comment 4

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #3)
> * Separate patches for methods and fields, if that helps with the clarity.

Good plan!

> * Separate patches for each major subdirectory.

Very good plan. The patches are now rather nice and bitesized. Since the review load can now plausibly be broken up (via assignment to people experienced in the corresponding area) your suggestion of going through and seeing where we can eliminate uses of inner classes entirely becomes a lot more reasonable.

> * Doing all of the android-sync stuff upstream, so we don't have to upstream
> this patch.

Naturally. Tweaked the script in such a way that the sync stuff is in separate patches I can import upstream in due course.

> * Improving spacing, if we can.
> 
> An alternative to improving spacing is to make the comment smaller. This is
> a common idiom:
> 
>   /* package */ boolean doFoo() {
> 
> so we could either do
> 
>   /* package: inner */ boolean ...
> 
> or 
> 
>   /* inner-access */ protected boolean ...

I like this idea, though I think it'd maybe be worth a mailing list post to explain what it means? Otherwise people are liable to think "Oooh, this could be private, let's make it so" and undo what we've done here.
Certainly, sticking a comment in like that looks way nicer than what I had before!

> ::: mobile/android/base/GeckoAccessibility.java
> @@ +44,4 @@
> >      // Used to store the JSON message and populate the event later in the code path.
> > +    // Referenced from inner class: do not make private.
> > +    protected static JSONObject sEventMessage = null;
> > +    // Referenced from inner class: do not make private.
> 
> I'd rather this had sane line spacing.

I think the changes now make this suggestion redundant?


Now Dropbox has finished exploding, you can find the script and output used to generate this patch here:
https://www.dropbox.com/sh/cm8cgav0htsrna3/AADDW5Vn-ZMm74hKq3mBG_31a

The script takes in the output from JOUST (when configured to not do any optimisations but just run the proxy detector analysis module I wrote for this purpose) and produces a series of pairs of files. 
Each pair consists of a "names" file (a list of field or method names) and a "files" file (a list of file names).
For each pair of files, a line from each is read in, and the element by that name in that file is modified appropriately using sed magic.

In principle, a name collision would lead to a redundant modification. Since it still compiles, I claim that any such alterations made here are harmless.

As for the correctness of the analysis that produced the JOUST output being processed, you can review the new tree visitor here:
https://bitbucket.org/ckitching/joust/src/fa21045c099571f54d5d02844137f38a717dac22/src/main/java/joust/optimisers/proxydetect/ProxyDetectVisitor.java

A BaseTranslator consists of a bunch of visitSomething methods, one for each type of AST node. The default action of these methods is to call other such methods in a way that produces a depth-first AST traversal. It is thus sufficient for users to override only the methods in which they wish to do interesting work.
(It also provides various tools for modifying the AST, but we don't care about that).

So, this class only processes nodes of type AJCMethodDecl: method declarations. Since this one is configured to run after desugaring:
https://bitbucket.org/ckitching/joust/src/6466dd0228cdebdfb2b869c7cf3a4c1155198840/src/main/java/joust/JOUST.java
(Whoops, I seem to have comitted some stupid commenting there >.> )

the visitor runs after javac has generated the accessor methods we're trying to avoid.
As the generated methods follow a naming convention, the hack used is to find all methods that have a name starting with "access$".
We then dig into its body to identify what it does. The body will either be a call to another method (for a proxy method call) or a "return someField" statement, so we identify which and print out the name of the symbol it's referring to. That's the field or method we need to tweak so javac no longer generates a method for it.
The ensuing output is something like:
https://www.dropbox.com/s/lpfuf8yj8h4m3z5/rawJOUSTOutput

Which forms the input to the hastly-thrown-together shell scripts.


A last interesting aside is the contents of "notOmg":
https://www.dropbox.com/s/srgjdb18fmuhguv/notOmg

This is stuff outside the o.m.g package that was nevertheless found to be accessed from a generated getter or proxy method. These can't be so trivially fixed (as the things in question cannot simply be made nonprivate). To fix these we need to be a little cleverer, if it's possible at all.

Patches to follow shortly.

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Part 1: Reduce use of generated getters in 'base' code.

(That is, code that is just in mobile/android/base and isn't further categorised)

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Part 2: Reduce use of generated proxy methods in 'base' code.

Comment 7

[Chris Kitching [:ckitching]]

Attachment: Part 3: Reduce use of generated getters in 'home' code.

About:home sure likes its event listeners!

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Part 4: Reduce use of generated proxy methods in 'home' code.

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Part 5: Reduce use of generated getters in 'widget' code.

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Part 6: Reduce use of generated proxy methods in 'widget' code.

Comment 11

[Chris Kitching [:ckitching]]

Attachment: Part 7: Reduce use of generated getters in 'gfx' code.

Comment 12

[Chris Kitching [:ckitching]]

Attachment: Part 8: Reduce use of generated proxy methods in 'gfx' code.

Comment 13

[Chris Kitching [:ckitching]]

Attachment: Part 9: Reduce use of generated getters in 'preferences' code.

Comment 14

[Chris Kitching [:ckitching]]

Attachment: Part 10: Reduce use of generated proxy methods in 'preferences' code.

Starting to regret having not automated this part of the task, now.

Comment 15

[Chris Kitching [:ckitching]]

Attachment: Part 11: Reduce use of generated getters in 'toolbar' code.

Comment 16

[Chris Kitching [:ckitching]]

Attachment: Part 12: Reduce use of generated proxy methods in 'toolbar' code.

Comment 17

[Chris Kitching [:ckitching]]

Attachment: Part 13: Reduce use of generated getters in 'fxa' code.

Comment 18

[Chris Kitching [:ckitching]]

Attachment: Part 14: Reduce use of generated proxy methods in 'fxa' code.

Comment 19

[Chris Kitching [:ckitching]]

Attachment: Part 15: Reduce use of generated getters in 'tabspanel' code.

Comment 20

[Chris Kitching [:ckitching]]

Attachment: Part 16: Reduce use of generated proxy methods in 'tabspanel' code.

Comment 21

[Chris Kitching [:ckitching]]

Attachment: Part 17: Reduce use of generated getters in 'background' code.

Comment 22

[Chris Kitching [:ckitching]]

Attachment: Part 18: Reduce use of generated proxy methods in 'background' code.

Comment 23

[Chris Kitching [:ckitching]]

Attachment: Part 19: Reduce use of generated getters in 'prompts' code.

Comment 24

[Chris Kitching [:ckitching]]

Attachment: Part 20: Reduce use of generated proxy methods in 'prompts' code.

Comment 25

[Chris Kitching [:ckitching]]

Attachment: Part 21: Reduce use of generated getters in other code.

The miscellaneous stuff that didn't seem to have enough changes in any one directory to warrant their own patch.

Comment 26

[Chris Kitching [:ckitching]]

Attachment: Part 22: Reduce use of generated proxy methods in other code.

Comment 27

[Chris Kitching [:ckitching]]

Attachment: Part 23: Reduce use of generated getters in 'sync' code.

The sync stuff got generated as patches, so might as well include it for your perusal. Or you can wait until I submit it upstream if you like.

Comment 28

[Chris Kitching [:ckitching]]

Attachment: Part 24: Reduce use of generated proxy methods in 'sync' code.

Comment 29

[Chris Kitching [:ckitching]]

Attachment: Part 25: Reduce use of generated getters in 'webrtc' code.

Comment 30

[Chris Kitching [:ckitching]]

Attachment: Part 26: Reduce use of generated proxy methods in 'webrtc' code.

This concludes the patchset.

As I said before, it may prove beneficial to shunt the reviews onto other people who are equipped to perform a more detailed analysis of each use and refactor more effectively.
After all, reduction of uses of inner classes represents another chance for saving binary size, though can't be done with a dirty optimiser hack and lots of shell scripts, alas.

My sincerest apologies for the avalanche of bugmail.

Comment 31

[Chris Kitching [:ckitching]]

Since this version includes some changes not in the first version (notably webrtc is now covered, as I made the script slightly cleverer), and has been rather restructured, let's do another try push to make sure I didn't bork anything (some Reflection edgecase or something isn't impossible):

https://tbpl.mozilla.org/?tree=Try&rev=a68b6484a900

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 8458453 [details] [diff] [review]
Part 1: Reduce use of generated getters in 'base' code.

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

I didn't review GeckoEditable yet, but I think it's probably broken.

Filed a bunch of blocking bugs, and I have a stack of alternative fixes for some of these files.

::: mobile/android/base/ANRReporter.java
@@ +47,5 @@
>      private static final String PING_CHARSET = "utf-8";
>  
>      private static final ANRReporter sInstance = new ANRReporter();
>      private static int sRegisteredCount;
> +    /* inner-access */ Handler mHandler;

Filed Bug 1041024.

::: mobile/android/base/BaseGeckoInterface.java
@@ +26,5 @@
>      // Bug 908755: Implement LocationListener
>      // Bug 908756: Implement Tabs.OnTabsChangedListener
>      // Bug 908760: Implement GeckoEventResponder
>  
> +    /* inner-access */ Context mContext;

No: see part 1a.

::: mobile/android/base/BrowserApp.java
@@ +147,5 @@
>      private View mBrowserSearchContainer;
>  
>      public ViewFlipper mViewFlipper;
>      public ActionModeCompatView mActionBar;
> +    /* inner-access */ BrowserToolbar mBrowserToolbar;

We can almost get rid of this by appropriate interface augmentation. But alas.

Please add a small block comment and split these members into their own 'section', explaining that they're accessed by inner classes -- typically event listeners -- and so need to be package-scoped for efficiency.

@@ +148,5 @@
>  
>      public ViewFlipper mViewFlipper;
>      public ActionModeCompatView mActionBar;
> +    /* inner-access */ BrowserToolbar mBrowserToolbar;
> +    /* inner-access */ ToolbarProgressView mProgressView;

No: see part 1b.

@@ +194,5 @@
>      // We'll ask for feedback after the user launches the app this many times.
>      private static final int FEEDBACK_LAUNCH_COUNT = 15;
>  
>      // Stored value of the toolbar height, so we know when it's changed.
> +    /* inner-access */ int mToolbarHeight = 0;

Move with the others (but keep the comment).

@@ +199,4 @@
>  
>      // Stored value of whether the last metrics change allowed for toolbar
>      // scrolling.
> +    /* inner-access */ boolean mDynamicToolbarCanScroll = false;

I don't see any use of this member from within an inner class. Check your working?

@@ +213,5 @@
>      // The animator used to toggle HomePager visibility has a race where if the HomePager is shown
>      // (starting the animation), the HomePager is hidden, and the HomePager animation completes,
>      // both the web content and the HomePager will be hidden. This flag is used to prevent the
>      // race by determining if the web content should be hidden at the animation's end.
> +    /* inner-access */ boolean mHideWebContentOnAnimationEnd = false;

Err, if this is trying to prevent a race, shouldn't it be `volatile`?

@@ +215,5 @@
>      // both the web content and the HomePager will be hidden. This flag is used to prevent the
>      // race by determining if the web content should be hidden at the animation's end.
> +    /* inner-access */ boolean mHideWebContentOnAnimationEnd = false;
> +
> +    /* inner-access */ DynamicToolbar mDynamicToolbar = new DynamicToolbar();

Move with the others.

::: mobile/android/base/ContactService.java
@@ +76,5 @@
>  
>      private final EventDispatcher mEventDispatcher;
>  
> +    /* inner-access */ String mAccountName;
> +    /* inner-access */ String mAccountType;

Filed Bug 1041037.

::: mobile/android/base/EditBookmarkDialog.java
@@ +28,5 @@
>   * methods.
>   */
>  public class EditBookmarkDialog {
>      private static final String LOGTAG = "GeckoEditBookmarkDialog";
> +    /* inner-access */ Context mContext;

Fixed in Part 1c.

::: mobile/android/base/FilePickerResultHandler.java
@@ +38,5 @@
>      private static final String UPLOADS_DIR = "uploads";
>  
>      protected final FilePicker.ResultHandler handler;
> +    /* inner-access */ final int tabId;
> +    /* inner-access */ final File cacheDir;

Part 1d.

::: mobile/android/base/FormAssistPopup.java
@@ +37,5 @@
>  import java.util.Arrays;
>  import java.util.Collection;
>  
>  public class FormAssistPopup extends RelativeLayout implements GeckoEventListener {
> +    /* inner-access */ Context mContext;

1e

::: mobile/android/base/GeckoAccessibility.java
@@ +43,2 @@
>      // Used to store the JSON message and populate the event later in the code path.
> +    /* inner-access */ static JSONObject sEventMessage = null;

Have to assume that this is only accessed from the UI thread. *sigh*

::: mobile/android/base/GeckoInputConnection.java
@@ +51,5 @@
>          "org.mozilla.gecko.tests.components.GeckoViewComponent$TextInput";
>  
>      private static final int INLINE_IME_MIN_DISPLAY_SIZE = 480;
>  
> +    /* inner-access */ static Handler sBackgroundHandler;

Filed Bug 1041082.

::: mobile/android/base/LightweightTheme.java
@@ +34,5 @@
>  public class LightweightTheme implements GeckoEventListener {
>      private static final String LOGTAG = "GeckoLightweightTheme";
>  
>      private Application mApplication;
> +    /* inner-access */ Handler mHandler;

1f.

::: mobile/android/base/MemoryMonitor.java
@@ +54,5 @@
>      }
>  
>      private final PressureDecrementer mPressureDecrementer;
>      private int mMemoryPressure;
> +    /* inner-access */ boolean mStoragePressure;

1g.

::: mobile/android/base/PrefsHelper.java
@@ +22,5 @@
>  public final class PrefsHelper {
>      private static final String LOGTAG = "GeckoPrefsHelper";
>  
>      private static boolean sRegistered = false;
> +    /* inner-access */ static final SparseArray<PrefHandler> sCallbacks = new SparseArray<PrefHandler>();

1h.

::: mobile/android/base/Tab.java
@@ +40,3 @@
>      private String mBaseDomain;
>      private String mUserSearch;
> +    /* inner-access */ String mTitle;

Move all of these into the same place.

Comment 33

[Richard Newman [:rnewman]]

Attachment: Part 1a: eliminating enclosing instance member access in BaseGeckoInterface

Comment 34

[Richard Newman [:rnewman]]

Attachment: Part 1b: eliminate enclosing instance member access of mProgressView in BrowserApp

Comment 35

[Richard Newman [:rnewman]]

Attachment: Part 1c: eliminate enclosing instance member access in EditBookmarkDialog

Comment 36

[Richard Newman [:rnewman]]

Attachment: Part 1d: eliminate enclosing instance member access in FilePickerResultHandler

Comment 37

[Richard Newman [:rnewman]]

Attachment: Part 1e: eliminate enclosing instance member access in FormAssistPopup

Comment 38

[Richard Newman [:rnewman]]

Attachment: Part 1f: eliminate enclosing instance member access in LightweightTheme

Comment 39

[Richard Newman [:rnewman]]

Attachment: Part 1g: make MemoryMonitor thread-safe

Comment 40

[Richard Newman [:rnewman]]

Attachment: Part 1h: eliminate enclosing instance member access in PrefsHelper

Comment 41

[Chris Kitching [:ckitching]]

Holy crap. What have I done? :P

(In reply to Richard Newman [:rnewman] from comment #32)
> Filed a bunch of blocking bugs, and I have a stack of alternative fixes for
> some of these files.

Very good plan. The existence of these accesses is something of an antipattern, so "proper" fixes are of course much preferable to the generated ones.
I'm delighted to see that this has produced useful spinoff bugs. It seems we may end up fixing a fair few stability problems as fallout from this. Hooray!

> @@ +199,4 @@
> >  
> >      // Stored value of whether the last metrics change allowed for toolbar
> >      // scrolling.
> > +    /* inner-access */ boolean mDynamicToolbarCanScroll = false;
> 
> I don't see any use of this member from within an inner class. Check your
> working?

That's not in the JOUST output nor any of the pairing files that the patch was generated from. It can't possibly be here!

Wat.

Please hold fire on subsequent reviews until I figure out what's happened here. This isn't the only such erroneous inclusion. Something has gone a bit wrong :/ .


General comment: Let's imagine I have some code like this:

public class C {
    private int magic = 12;

    public void doStuff() {
        final int cake = magic;

        Runnable runner = new Runnable() {
            @Override
            public void run() {
                System.out.println(cake);
            }
        };
    }
}

I see you're using this pattern in a few places to fix access to the private field.

This technique is flawed in a similar way to directly accessing the private field. 

The example code gets translated to something equivalent to the following before compilation:

public class C {
    private int magic = 12;

    public void doStuff() {
        final int cake = magic;

        C$1 runner = new C$1(this, cake);
    }
}

class C$1 extends Runnable {
    Runnable runner = new Runnable() {

    private C this$0;      // The usual backreference you get from an anonymous inner class.
    private int val$cake;  // Oh look, that "local variable".

    public C$1(C backref, int cake) {
        this$0 = backref;
        val$cake = cake;
    }

    @Override
    public void run() {
        System.out.println(val$cake);
    }
};

For each final local variable accessed inside the inner class, we generate a field on the inner class and a constructor param to assign it. 

The ensuing cost in terms of binary size is about half as much as that of the accessor methods (as compared to just declaring the field package-access and directly touching it).
On the other hand, this approach increases the app's memory footprint: we now have an extr

Had we declared "magic" as package access above, then the Runnable looks like this:

public class C$1 extends Runnable {
    Runnable runner = new Runnable() {

    private C this$0;

    public C$1(C backref) {
        this$0 = backref;
    }

    @Override
    public void run() {
        System.out.println(this$0.magic);
    }
};


To summarise:

accessor methods: Huge binary, slow access, only one copy of the data.
local variable: Medium-sized binary, fast access, slower construction of inner class instance, duplicated data.
package-access: Smallest binary, fast access, one copy of data (but namespace pollution).


I therefore encourage you to avoid the use of the local-variable technique where possible, particularly in cases where multiple instances of an inner class are due to exist (because then you end up copying the data many times).

Comment 42

[Chris Kitching [:ckitching]]

Comment on attachment 8459069 [details] [diff] [review]
Part 1e: eliminate enclosing instance member access in FormAssistPopup

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

::: mobile/android/base/FormAssistPopup.java
@@ +38,5 @@
>  import java.util.Collection;
>  
>  public class FormAssistPopup extends RelativeLayout implements GeckoEventListener {
> +    private final Context mContext;
> +    private final Animation mAnimation;

mAnimation: Another one that sneaked into my patches "impossibly". Hmph.

@@ +362,5 @@
>  
>          public AutoCompleteListAdapter(Context context, int textViewResourceId) {
>              super(context, textViewResourceId);
>  
> +            mInflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);

Hooray!

Comment 43

[Chris Kitching [:ckitching]]

(In reply to Chris Kitching [:ckitching] from comment #42)
> mAnimation: Another one that sneaked into my patches "impossibly". Hmph.

No it isn't, I can't read. 

> @@ +199,4 @@
> >  
> >      // Stored value of whether the last metrics change allowed for toolbar
> >      // scrolling.
> > +    /* inner-access */ boolean mDynamicToolbarCanScroll = false;
> 
> I don't see any use of this member from within an inner class. Check your
> working?

Figured it out: JOUST detected mDynamicToolbar, and a bug in the regex caused this to lead to both mDynamicToolbar and mDynamicToolbarCanScroll being changed (because prefix).

Changed the regex to require a space or semicolon after the name, uploading all affected patches shortly. Sorry for the confusion. Well spotted!

Panic over: Feel free to continue reading through, just beware of the possibility of prefix collisions until the new patches come out.

Comment 44

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #41)

> I therefore encourage you to avoid the use of the local-variable technique
> where possible, particularly in cases where multiple instances of an inner
> class are due to exist (because then you end up copying the data many times).

They're actually not equivalent (unless we're talking about different things).

Capturing in a `final` local outside the declaration of an inner class has different concurrency properties than package-scoped access to an enclosed member from within arbitrary methods inside the inner class.

It's better to capture a final in a controlled threading scenario than it is to require synchronized or other thread-safe access to a member, no?

Comment 45

[Chris Kitching [:ckitching]]

Attachment: Part 22 (V2): Reduce use of generated proxy methods in other code.

Found a bug in the regex: As well as false-positiving anything with a name that prefix-matched with a real match, it was also false-negativeing anything with generics.

I'll now upload the corrected ensuing patchset. (I also went and automated more of the patch generation step, making this even more amusingly easy to unbitrot).

(I'll get to your review comments in a moment, hold on)

Comment 46

[Chris Kitching [:ckitching]]

Attachment: Part 21 (V2): Reduce use of generated getters in 'other' code.

Comment 47

[Chris Kitching [:ckitching]]

Attachment: Part 19 (V2): Reduce use of generated getters in 'prompts' code.

Comment 48

[Chris Kitching [:ckitching]]

Attachment: Part 17 (V2): Reduce use of generated getters in 'background' code.

Comment 49

[Chris Kitching [:ckitching]]

Attachment: Part 15 (V2): Reduce use of generated getters in 'tabspanel' code.

Comment 50

[Chris Kitching [:ckitching]]

Attachment: Part 12 (V2): Reduce use of generated proxy methods in 'toolbar' code.

Comment 51

[Chris Kitching [:ckitching]]

Attachment: Part 11 (V2): Reduce use of generated getters in 'toolbar' code.

Comment 52

[Chris Kitching [:ckitching]]

Attachment: Part 8 (V2): Reduce use of generated proxy methods in 'gfx' code.

Comment 53

[Chris Kitching [:ckitching]]

Attachment: Part 7 (V2): Reduce use of generated getters in 'gfx' code.

Comment 54

[Chris Kitching [:ckitching]]

Attachment: Part 5 (V2): Reduce use of generated getters in 'widget' code.

Comment 55

[Chris Kitching [:ckitching]]

Attachment: Part 3 (V2): Reduce use of generated getters in 'home' code.

Comment 56

[Chris Kitching [:ckitching]]

Attachment: Part 2 (V2): Reduce use of generated proxy methods in 'base' code.

Sans the first part, this completes the changes due to fixing of the regexes. There should be no more erroneously-included members in these diffs.

Comment 57

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #44)
> (In reply to Chris Kitching [:ckitching] from comment #41)
> 
> > I therefore encourage you to avoid the use of the local-variable technique
> > where possible, particularly in cases where multiple instances of an inner
> > class are due to exist (because then you end up copying the data many times).
> 
> They're actually not equivalent (unless we're talking about different
> things).
> 
> Capturing in a `final` local outside the declaration of an inner class has
> different concurrency properties than package-scoped access to an enclosed
> member from within arbitrary methods inside the inner class.
> 
> It's better to capture a final in a controlled threading scenario than it is
> to require synchronized or other thread-safe access to a member, no?

Absolutely! This is what I meant by "Where possible". I didn't make that especially clear. Sorry!

Best of all is to just make the previously-private field a package-final field. That way you get the best of both worlds (though, again, this isn't generally possible).

My intent was to let you know about the hidden cost you might not have known about. This is a slightly strange way for Java to implement this feature. Apologies for the miscommunication (or if I'm teaching my grandmother how to suck eggs)

Comment 58

[Chris Kitching [:ckitching]]

Comment on attachment 8459071 [details] [diff] [review]
Part 1g: make MemoryMonitor thread-safe

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

Of this, I have only "AAAAAAAAAAAAAAAAAAAA" to say.

Comment 59

[Chris Kitching [:ckitching]]

Comment on attachment 8459070 [details] [diff] [review]
Part 1f: eliminate enclosing instance member access in LightweightTheme

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

While we're here, let's replace like 150:

mIsLight = (luminance > 110) ? true : false;

With:

mIsLight = luminance > 110

...Since you're already doing a fair bit of general cleanup.

::: mobile/android/base/LightweightTheme.java
@@ +73,5 @@
>      }
>  
>      @Override
>      public void handleMessage(String event, JSONObject message) {
> +        final Handler handler = mHandler;          // To avoid access to private enclosing member.

Since you've managed to make mHandler final, why not just access it directly and avoid the copy?

Comment 60

[Chris Kitching [:ckitching]]

Comment on attachment 8459068 [details] [diff] [review]
Part 1d: eliminate enclosing instance member access in FilePickerResultHandler

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

Looks good. More copying to eat into our improvement in binary size, but it does clarify things conceptually.

Consider additional cleanup:

Line 115: Redundant return statement.

Line 196:
                String fileExt = null;
 Redundant initialiser to "null"

Comment 61

[Chris Kitching [:ckitching]]

Comment on attachment 8459066 [details] [diff] [review]
Part 1c: eliminate enclosing instance member access in EditBookmarkDialog

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

::: mobile/android/base/EditBookmarkDialog.java
@@ -28,4 @@
>   * methods.
>   */
>  public class EditBookmarkDialog {
> -    private static final String LOGTAG = "GeckoEditBookmarkDialog";

Presumably Eclipse is telling you, as IDEA is telling me, that this is never used.

It's right, but this is also something Proguard is capable of doing for us at compile-time. Maybe worth leaving in for ease of addition of future log statements?

@@ +207,5 @@
>                      @Override
>                      public Void doInBackground(Void... params) {
>                          String newUrl = locationText.getText().toString().trim();
>                          String newKeyword = keywordText.getText().toString().trim();
> +                        BrowserDB.updateBookmark(cr, id, newUrl, nameText.getText().toString(), newKeyword);

You'd do very slightly better in terms of both space and speed to replace cr with "context.getContentResolver()" and then remove its declaration. You'd only be copying one variable in, need less bytecode to express it, and use slightly less memory.

*shrug*. I don't think having the extra variable lends any additional clarity. Up to you.

Comment 62

[Chris Kitching [:ckitching]]

Comment on attachment 8459072 [details] [diff] [review]
Part 1h: eliminate enclosing instance member access in PrefsHelper

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

::: mobile/android/base/PrefsHelper.java
@@ +62,5 @@
>          if (sRegistered) {
>              return;
>          }
>  
> +        final SparseArray<PrefHandler> callbacks = sCallbacks;

Since sCallbacks is final, I'm again unsure what's gained by doing the copy vs. just accessing it directly. Copying the reference isn't going to help you with future concurrency problems here, as the reference cannot ever change.

Comment 63

[Chris Kitching [:ckitching]]

Comment on attachment 8459065 [details] [diff] [review]
Part 1b: eliminate enclosing instance member access of mProgressView in BrowserApp

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

::: mobile/android/base/BrowserApp.java
@@ +1060,5 @@
>              mDynamicToolbarCanScroll = true;
>          }
>  
>          final View toolbarLayout = mViewFlipper;
> +        final ToolbarProgressView progressView = mProgressView;

That's fine, but can't we declare mProgressView final and avoid the copy anyway?

... onCreate only happens once, right? Or have I forgotten an edgecase from Android's activity lifecycle.

I think we should be aiming to make fields final with direct access where possible, and using copying only where it either substantially aids clarity or is necessary (because the thing changes and we want to snapshot it, or need the copy semantics for some other reason).

Comment 64

[Chris Kitching [:ckitching]]

Attachment: Part 1 (V2): Reduce use of generated getters in 'base' code.

(In reply to Richard Newman [:rnewman] from comment #32)
> I didn't review GeckoEditable yet, but I think it's probably broken.

Righto. Note also that this second edition (due to the regex fix) now also processes the following members that it did not touch before:

org.mozilla.gecko.AndroidGamepadManager.sGamepads
org.mozilla.gecko.AndroidGamepadManager.sPendingGamepads
org.mozilla.gecko.CrashReporter.mExtrasStringMap
org.mozilla.gecko.NotificationClient.mUpdatesMap

It no longer touches (due to the prefix-matching fix) the following:

org.mozilla.gecko.BaseGeckoInteface.mDynamicToolbarCanScroll
org.mozilla.gecko.BaseGeckoInterface.mBrowserHealthReporter
org.mozilla.gecko.Tab.mThumbnailBitmap

Should be smoother sailing from here...

> @@ +213,5 @@
> >      // The animator used to toggle HomePager visibility has a race where if the HomePager is shown
> >      // (starting the animation), the HomePager is hidden, and the HomePager animation completes,
> >      // both the web content and the HomePager will be hidden. This flag is used to prevent the
> >      // race by determining if the web content should be hidden at the animation's end.
> > +    /* inner-access */ boolean mHideWebContentOnAnimationEnd = false;
> 
> Err, if this is trying to prevent a race, shouldn't it be `volatile`?

I think you might just get away with it not being so because both events of the PropertyAnimator occur on the same thread (right?).

Certainly, that's a bit fragile. Volatileifying it.


r?-ing again because of the new fields. Maybe you'll find eight more bugs :P.

Comment 65

[Chris Kitching [:ckitching]]

... In my haste I managed to trample on the assignment flag. Whoops.

Comment 66

[Richard Newman [:rnewman]]

> That's fine, but can't we declare mProgressView final and avoid the copy
> anyway?
>
> ... onCreate only happens once, right? Or have I forgotten an edgecase from
> Android's activity lifecycle.

Stuff that's set in onCreate can't be final; it's not a constructor from the perspective of Java's memory model.

Comment 67

[Richard Newman [:rnewman]]

Let's avoid the fat bug of hell by splitting this bug apart. I'll file new bugs for the dependencies already identified in attachments.

Comment 68

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #66)
> > That's fine, but can't we declare mProgressView final and avoid the copy
> > anyway?
> >
> > ... onCreate only happens once, right? Or have I forgotten an edgecase from
> > Android's activity lifecycle.
> 
> Stuff that's set in onCreate can't be final; it's not a constructor from the
> perspective of Java's memory model.

*facepalm*

I shouldn't review in the middle of the night. You're right, as usual. :P


(In reply to Richard Newman [:rnewman] from comment #67)
> Let's avoid the fat bug of hell by splitting this bug apart. I'll file new
> bugs for the dependencies already identified in attachments.

That's an excellent idea. This lot'll bitrot to hell if we try and land them all at once, and this gives even more opportunities for splitting the workload between multiple people.

This one really, *really* scope-crept.

Comment 69

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #68)

> This one really, *really* scope-crept.

Not really scope creep: this was a deliberate and actionable opportunity for an audit, and it's bearing fruit.

Comment 70

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #59)

> mIsLight = luminance > 110
...
> Since you've managed to make mHandler final, why not just access it directly
> and avoid the copy?

Both done.

Comment 71

[Nathan Froyd [:froydnj]]

Do we have any plans to yell at people (preferably breaking the build) when they reintroduce this sort of thing?  Otherwise, somebody will have to do this work every six months or so.

Comment 72

[Richard Newman [:rnewman]]

(In reply to Nathan Froyd (:froydnj) from comment #71)
> Do we have any plans to yell at people (preferably breaking the build) when
> they reintroduce this sort of thing?  Otherwise, somebody will have to do
> this work every six months or so.

I wouldn't say concrete plans, but it's theoretically possible to do so. It might involve adding a separate analysis phase to the build.

Comment 73

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #72)
> (In reply to Nathan Froyd (:froydnj) from comment #71)
> > Do we have any plans to yell at people (preferably breaking the build) when
> > they reintroduce this sort of thing?  Otherwise, somebody will have to do
> > this work every six months or so.
> 
> I wouldn't say concrete plans, but it's theoretically possible to do so. It
> might involve adding a separate analysis phase to the build.

It's so much more than "theoretically possible". How do you think I generated these patches in the first place? I've got a patch set that kludges my experimental optimiser into the Fennec build process which I then use to find the problem areas. The same approach was used on bug 1041836.

I'd be more than happy to spend a weekend or three turning this into something that won't make nalexander try and throw me out the window. 
Clearly we don't want to run any optimisations (because landing an optimiser an idiot threw together in six months is *clearly* a good idea >.>), but it could be used to run static analysis on the Java AST, printing warnings (exactly as it did here), and maybe then failing the build based on what it finds.

The framework I have permits the insertion of arbitrary Runnables into any phase of javac's pipeline, with R/W access to the AST. This allows one to do a great many things, though I suggest the only safe things to do at this point are read-only analysis. Maybe we might want to think about some code generation later, but that's far less useful than detection of antipatterns at compile-time.

We could straightforwardly design a module that detects Cursors that may not always be closed, saving us headaches and crashes. We can detect wasteful antipatterns such as these private inner accesses or those redundant initialisers. We could scan for dataflow anomalies such as unused local variables which often represent programmer error... The possibilities are endless, and the obscenely complicated first step of "how the hell do we even get access to the AST to start thinking about this anyway?" comes for free.

I can haz project? (secondary to my other one, naturally).

Comment 74

[Richard Newman [:rnewman]]

Comment on attachment 8459127 [details] [diff] [review]
Part 1 (V2): Reduce use of generated getters in 'base' code.

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

r+, please uplift to a-s.

Comment 75

[Richard Newman [:rnewman]]

Comment on attachment 8459116 [details] [diff] [review]
Part 2 (V2): Reduce use of generated proxy methods in 'base' code.

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

I apologize; I think I want the member and method patches combined again :(

::: mobile/android/base/GeckoAppShell.java
@@ +366,3 @@
>          GeckoEditable editable = new GeckoEditable();
>          // install the gecko => editable listener
>          editableListener = editable;

Inline this method directly into the runnable, make `editableListener` volatile and inner-access, and eliminate the temporary `editable` var. And correct the comment.

::: mobile/android/base/Tab.java
@@ +738,5 @@
>              // ignore
>          }
>      }
>  
> +    /* inner-access */ void clearThumbnailFromDB() {

Just inline this method.

::: mobile/android/base/TextSelection.java
@@ +108,5 @@
>              "TextSelection:Update",
>              "TextSelection:DraggingHandle");
>      }
>  
> +    /* inner-access */ TextSelectionHandle getHandle(String name) {

mStartHandle etc. already need to be inner-access for a bunch of runnables. And getHandle is only called in one place, so inline it.

@@ +192,1 @@
>  	String itemsString = items.toString();

Fix tabs here.

@@ +213,5 @@
>          if (context instanceof ActionModeCompat.Presenter) {
>              final ActionModeCompat.Presenter presenter = (ActionModeCompat.Presenter) context;
>              presenter.endActionModeCompat();
>          }
>  	mCurrentItems = null;

I believe mCurrentItems needs to be volatile.

Comment 76

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #75)
> I apologize; I think I want the member and method patches combined again :(

So I heard you like bugmail...

> ::: mobile/android/base/Tab.java
> @@ +738,5 @@
> >              // ignore
> >          }
> >      }
> >  
> > +    /* inner-access */ void clearThumbnailFromDB() {
> 
> Just inline this method.

This one's a little complicated, so I'll shift it inside the anonymous inner class but keep it as a Runnable: seems to help clarity (and Proguard can properly inline it into run() from there if it wants to).


> ::: mobile/android/base/TextSelection.java
> @@ +108,5 @@
> >              "TextSelection:Update",
> >              "TextSelection:DraggingHandle");
> >      }
> >  
> > +    /* inner-access */ TextSelectionHandle getHandle(String name) {
> 
> mStartHandle etc. already need to be inner-access for a bunch of runnables.
> And getHandle is only called in one place, so inline it.

Called from two places, so likewise just shifting it into the inner class instead of inlining it.


This class seems to have some dead code, which is odd, too. mEventDispatcher is assigned in the ctor but never used, 
and the activity parameter of the ctor is never used, too.

I'll file a fresh bug to clean that crap up.

> @@ +192,1 @@
> >  	String itemsString = items.toString();
> 
> Fix tabs here.

Those are actual *tab characters*.

Wat.

Interested in a cleanup bug to fix every instance of those and replace them with spaces? Seems tab characters have sneaked in all over the place.

Comment 77

[Chris Kitching [:ckitching]]

Um.

"... inner class but keep it as a Runnable: seems to help clarity..."

should read

"... inner class but keep it as a method: seems to help clarity..."

E_INSUFFICIENT_CAFFIENE

Comment 78

[Chris Kitching [:ckitching]]

Attachment: Reduce use of proxy methods and generated getters in 'webrtc' code

Comment 79

[Chris Kitching [:ckitching]]

Attachment: Evil hack to simplify obsoleting a million patches.

Comment 80

[Chris Kitching [:ckitching]]

Attachment: Part 1/13: Reduce use of proxy methods and generated getters in 'base' code

Comment 81

[Chris Kitching [:ckitching]]

Attachment: Part 2/13: Reduce use of proxy methods and generated getters in 'home' code

Comment 82

[Chris Kitching [:ckitching]]

Attachment: Part 3/13: Reduce use of proxy methods and generated getters in 'widget' code

Comment 83

[Chris Kitching [:ckitching]]

Attachment: Part 4/13: Reduce use of proxy methods and generated getters in 'gfx' code

Comment 84

[Chris Kitching [:ckitching]]

Attachment: Part 5/13: Reduce use of proxy methods and generated getters in 'preferences' code

Comment 85

[Chris Kitching [:ckitching]]

Attachment: Part 6/13: Reduce use of proxy methods and generated getters in 'toolbar' code

Comment 86

[Chris Kitching [:ckitching]]

Attachment: Part 7/13: Reduce use of proxy methods and generated getters in 'fxa' code

Comment 87

[Chris Kitching [:ckitching]]

Attachment: Part 8/13: Reduce use of proxy methods and generated getters in 'tabspanel' code

Comment 88

[Chris Kitching [:ckitching]]

Attachment: Part 9/13: Reduce use of proxy methods and generated getters in 'background' code

Comment 89

[Chris Kitching [:ckitching]]

Attachment: Part 10/13: Reduce use of proxy methods and generated getters in 'prompts' code

Comment 90

[Chris Kitching [:ckitching]]

Attachment: Part 11/13: Reduce use of proxy methods and generated getters in 'other' code

Comment 91

[Chris Kitching [:ckitching]]

Attachment: Part 12/13: Reduce use of proxy methods and generated getters in 'sync' code

Comment 92

[Chris Kitching [:ckitching]]

Attachment: Part 13/13: Reduce use of proxy methods and generated getters in 'webrtc' code

Comment 93

[Chris Kitching [:ckitching]]

Okay, new patches up. The new "Part 1" contains all the review comments from the old 1/2, so should just be a rubber-stamp. As requested, I've merged the fields/methods down.

Also, added support to the script for an "ignore" list of fields, meaning things you pull out and fix manually are just omitted from the generated patches.
... And then wrote a wrapper script for bzexport to make uploading them all less painful.

Muwhaha. :P

Comment 94

[Richard Newman [:rnewman]]

Comment on attachment 8463749 [details] [diff] [review]
Part 13/13: Reduce use of proxy methods and generated getters in 'webrtc' code

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

::: media/webrtc/trunk/webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java
@@ +40,5 @@
>  // uncontended.
>  public class VideoCaptureAndroid implements PreviewCallback, Callback {
>    private final static String TAG = "WEBRTC-JC";
>  
> +  /* inner-access */ Camera camera;  // Only non-null while capturing.

Add a comment that this member is only accessed via synchronized methods.

@@ +52,5 @@
>    // potentially stalling the capturer if it runs out of buffers to write to).
>    private final int numCaptureBuffers = 3;
>    // Needed to start/stop/rotate camera.
>    private AppStateListener mAppStateListener;
> +  /* inner-access */ int mCaptureRotation;

mCapture* are a concurrency issue waiting to happen. Please ping the blame owners.

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViEAndroidGLES20.java
@@ +28,5 @@
>  import org.mozilla.gecko.mozglue.WebRTCJNITarget;
>  
>  public class ViEAndroidGLES20 extends GLSurfaceView
>          implements GLSurfaceView.Renderer {
> +    /* inner-access */ static String TAG = "WEBRTC-JR";

Make final.

Comment 95

[Richard Newman [:rnewman]]

Comment on attachment 8463747 [details] [diff] [review]
Part 11/13: Reduce use of proxy methods and generated getters in 'other' code

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

r+ with changes. Leave MatrixBlobCursor and Clipboard.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +94,2 @@
>      private final ConfigurationProvider configProvider;
> +    /* inner-access */ final EventDispatcher dispatcher;

Move the package-scoped things together.

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +47,5 @@
>      private boolean mEnabled = true;
>      private Drawable mIcon;
>      private int mIconRes;
>      private GeckoActionProvider mActionProvider;
> +    /* inner-access */ GeckoMenu mMenu;

I made this final in a patch in my queue. And move it with its friend.

::: mobile/android/base/sqlite/MatrixBlobCursor.java
@@ +42,5 @@
>   */
>  public class MatrixBlobCursor extends AbstractCursor {
>  
>      private final String[] columnNames;
> +    /* inner-access */ Object[] data;

Set and expanded in two places, which probably ends up fine, but a rowbuilder fills it, and being a cursor it can be accessed from any thread...

This is probably a concurrency bug to fix. Filed Bug 1045916.

::: mobile/android/base/util/Clipboard.java
@@ +13,5 @@
>  
>  import java.util.concurrent.SynchronousQueue;
>  
>  public final class Clipboard {
> +    /* inner-access */ static Context mContext;

Filed Bug 1045921.

@@ +102,5 @@
>      public static void clearText() {
>          setText(null);
>      }
>  
> +    /* inner-access */ static android.content.ClipboardManager getClipboardManager(Context context) {

Nope: let's lift this call out of the setText runnable.

@@ +108,5 @@
>          // which is a subclass of android.text.ClipboardManager.
>          return (android.content.ClipboardManager) mContext.getSystemService(Context.CLIPBOARD_SERVICE);
>      }
>  
> +    /* inner-access */ static android.text.ClipboardManager getDeprecatedClipboardManager(Context context) {

Same.

::: mobile/android/base/util/WebActivityMapper.java
@@ +148,5 @@
>              }
>          }
>      }
>  
> +    /* inner-access */ static void optPutExtra(String key, String extraName, JSONObject data, Intent intent) {

Move optPutExtra into SendMapping and keep it private.

::: mobile/android/base/webapp/WebappImpl.java
@@ +42,5 @@
>      private URI mOrigin;
>      private TextView mTitlebarText;
>      private View mTitlebar;
>  
> +    /* inner-access */ View mSplashscreen;

Comment that this must only be accessed from the UI thread.

Comment 96

[Gian-Carlo Pascutto [:gcp]]

Filed bug 1046275 for the issues raised with the WebRTC code.

Comment 97

[Richard Newman [:rnewman]]

Comment on attachment 8463738 [details] [diff] [review]
Part 3/13: Reduce use of proxy methods and generated getters in 'widget' code

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

Split out TwoWayView for Lucas to give a second review and uplift.

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +297,5 @@
>       * that the very first read succeeds and subsequent reads can be performed
>       * only after a call to {@link #persistHistoricalDataIfNeeded()} followed by change
>       * of the share records.
>       */
> +    /* inner-access */ boolean mCanReadHistoricalData = true;

Comment that this is protected by mInstanceLock.

Fix the one place that it isn't: in PersistHistoryAsyncTask.

@@ +321,5 @@
>  
>      /**
>       * Flag whether to reload the activities for the current intent.
>       */
> +    /* inner-access */ boolean mReloadActivities;

Same: onReceive should take the lock.

@@ +1227,5 @@
>       * Mozilla: Adapted significantly
>       */
>      private static final String LOGTAG = "GeckoActivityChooserModel";
>      private final class DataModelPackageMonitor extends BroadcastReceiver {
> +        /* inner-access */ Context mContext;

volatile.

::: mobile/android/base/widget/ArrowPopup.java
@@ +24,5 @@
>  import android.widget.RelativeLayout;
>  
>  public abstract class ArrowPopup extends PopupWindow {
> +    /* inner-access */ View mAnchor;
> +    /* inner-access */ ImageView mArrow;

Ugh, not thread safe. Leave this, but file.

DoorHangerPopup runnable ... addDoorHanger > ArrowPopup.init
OnSizeChangedListener

@@ +153,5 @@
>          public interface OnSizeChangedListener {
>              public void onSizeChanged();
>          }
>  
> +        /* inner-access */ OnSizeChangedListener mListener;

Should probably be volatile, too.

::: mobile/android/base/widget/BasicColorPicker.java
@@ +42,5 @@
>                                                                        Color.BLACK);
>  
>      private static Drawable mCheckDrawable;
> +    /* inner-access */ int mSelected;
> +    final /* inner-access */ ColorPickerListAdapter mAdapter;

Switch the final and comment.

::: mobile/android/base/widget/TwoWayView.java
@@ +197,1 @@
>      private int mSelectorPosition;

Clean up this kind of layout: cluster within each group, move to start or end of group.

Comment 98

[Richard Newman [:rnewman]]

Chris: please split out the bits that I've already reviewed, address comments, and land in a separate bug. I doubt all 13 patches will get reviewed and stick within a short enough timeframe to make me happy with a single bug.

Comment 99

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #98)
> Chris: please split out the bits that I've already reviewed, address
> comments, and land in a separate bug. I doubt all 13 patches will get
> reviewed and stick within a short enough timeframe to make me happy with a
> single bug.

An excellent idea: I think I suggested it at one point :P

I'll split each patch into its own bug chaining off this one and land the reviewed ones. (which also may make it clearer where bugs that fork off this one are coming from, instead of just pointing them to the huge splurge of 13 patches here).

Comment 100

[Chris Kitching [:ckitching]]

(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #97)
> ::: mobile/android/base/widget/ActivityChooserModel.java
> @@ +297,5 @@
> >       * that the very first read succeeds and subsequent reads can be performed
> >       * only after a call to {@link #persistHistoricalDataIfNeeded()} followed by change
> >       * of the share records.
> >       */
> > +    /* inner-access */ boolean mCanReadHistoricalData = true;
> 
> Comment that this is protected by mInstanceLock.

It's unclear why mInstanceLock exists. synchronized(this), anyone?

> Fix the one place that it isn't: in PersistHistoryAsyncTask.

This is a footgun. readHistoricalDataIfNeeded needs to be called while holding the lock, which it always is (for now), but this is not documented.
Sane solution: Eliminate mInstanceLock and make the methods that use it to cover their entire bodies synchronized. This will cause readHistoricalDataIfNeeded to be a synchronized method, making more obvious the locks held here. As it stands, it's just waiting for someone to come along and call it while not holding the lock, and we've got a party on our hands.

Bug 1051720. I'm going to neglect the concurrency issues you raised here entirely and resolve them in that bug. Let's not entangle something slightly nontrivial with the very trivial access modifier changes (which may be a pain in the arse to backout)

Comment 101

[Chris Kitching [:ckitching]]

Attachment: More mass-obsoletion.

Comment 102

[Lucas Rocha (:lucasr)]

Guys, is this really worth it as a general guideline? Google recommends avoiding the wrappers in performance critical parts of the app. I find all these /* inner-access */ comments quite distracting and vague. I'd like to see a more compelling argument in favor of these changes.

At first sight, this is looking like a rather distracting nice-to-have micro-optimization.

Comment 103

[Gian-Carlo Pascutto [:gcp]]

I agree with this sentiment, and it's why I refactored my affected code rather than deal with this :-/

Comment 104

[Richard Newman [:rnewman]]

I think it's worthwhile in two senses:

* It kills a few hundred KB of compiled bytecode, resulting in a 60KB APK reduction. That's worthwhile, considering that this is an automated transformation. We're not doing this as a performance enhancement (which is Google's motivation) -- we're doing this because we have so much code that we can save non-trivial package size!

* Access to enclosing private members is sometimes indicative of concurrency bugs -- simply skimming these patches has already found some. For me, that makes the reviewing 'cost' of this small, because we're improving stability as a side effect.


Given that we rarely otherwise use package scope, and it seems to be annoying, I'd be inclined to drop the annotation comment and just switch the scoping, relying on culture to communicate why.

The alternative to that is for Chris to finish up an automated phase of the build that will do this kind of stuff (and ProGuard annotations, and...) and use that instead.

Comment 105

[Lucas Rocha (:lucasr)]

(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #104)
> The alternative to that is for Chris to finish up an automated phase of the
> build that will do this kind of stuff (and ProGuard annotations, and...) and
> use that instead.

I like this. Sounds like a more future-proof way to go. I'd prefer us to just use Java features as usual and let the build process cleanup/optimize things where appropriate.

Comment 106

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #105)

> I like this. Sounds like a more future-proof way to go. I'd prefer us to
> just use Java features as usual and let the build process cleanup/optimize
> things where appropriate.

More future-proof in that it catches future regressions, for sure. The downsides:

* Effort spent in building the processor
* The code we ship would be noticeably different from the code we edit.

Those aren't deal-breakers, but they certainly shunted me towards the simpler path in the short term.

Comment 107

[Chris Kitching [:ckitching]]

(In reply to Lucas Rocha (:lucasr) from comment #105)
> (In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #104)
> > The alternative to that is for Chris to finish up an automated phase of the
> > build that will do this kind of stuff (and ProGuard annotations, and...) and
> > use that instead.
> 
> I like this. Sounds like a more future-proof way to go. I'd prefer us to
> just use Java features as usual and let the build process cleanup/optimize
> things where appropriate.

(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #106)
> (In reply to Lucas Rocha (:lucasr) from comment #105)
> 
> > I like this. Sounds like a more future-proof way to go. I'd prefer us to
> > just use Java features as usual and let the build process cleanup/optimize
> > things where appropriate.
> 
> More future-proof in that it catches future regressions, for sure. The
> downsides:
> 
> * Effort spent in building the processor
> * The code we ship would be noticeably different from the code we edit.
> 
> Those aren't deal-breakers, but they certainly shunted me towards the
> simpler path in the short term.

The code I'm exploiting to generate these patches was written as part of my final year research project at Cambridge. It was thrown together in three months, and as best I can tell it works.
It's probably not wise to land an experimental optimiser and rely on it to safely rewrite your ASTs on a major project like this. I'd love to do it, but it'll be a huge maintenance burden (remember that you're losing the only employee who currently understands the code in about 8 weeks. You probably don't want 17k LoC that nobody's worked with before sitting around...), and a big risk (optimiser bug eats your AST? Oh dear).

I have therefore been working towards rephrasing the optimiser as an analysis-only system to spit out warnings about common problems (and then using sed scripts to build patches from these warnings, as I did here). We may choose to fail builds in response to some classes of problem. The ensuing codebase is very much smaller than the optimiser (down to 6k now, and still much to prune).

This gives us most of the benefits and few of the risks associated with having an optimiser. It does limit the optimisations we can do, but not too severely: most of those which we'd want to do that are unpleasant to do on source are done by Proguard anyway. Any automatic detection of problems is much better than none.

This approach eliminates the risk of the optimiser corrupting the AST and killing your builds. If it's printing stupid warnings just turn it off.

Detecting problems and reporting them is usually much simpler than running an optimisation based on them: another reason why the volume of code is dramatically reduced (as well as the fact I deleted most of the optimisation modules, such as CSE and LICM). Current work has yielded a system which can detect these generated methods, redundant ctor/clinit initialisers and (shortly) unclosed cursors. The actual analysis modules turn out to be fairly pleasant to write - the module responsible for detecting the problems this bug aims to fix is found here, for example:
https://bitbucket.org/ckitching/joust/src/fa21045c099571f54d5d02844137f38a717dac22/src/main/java/joust/optimisers/proxydetect/ProxyDetectVisitor.java?at=default

Once I've completed the code-pruning, should I submit this for review somewhere? (this will probably take at least a month, given how low down my TODO list it is). What I have right now *works*, but it's in need of some refactoring to finish disentangling it with some of the now-uninteresting code.


I agree with the sentiment that it's better to fix problems like this at compile-time, but in this case there's a strong correlation between the existence of this problem and the existence of other problems (check out the spinoff bugs!). Given that, and the risks/complexity of actually fixing it in the AST, generating patches from the warnings output by the analyser module seemed the best way forward. (and appears to be paying off)

I am very keen to land an analyser. Another handy thing it'd allow is the resolution of Bug 1048651 (very easily, too, which is neat). Another added bonus is that it'd involve refactoring the existing annotation processor as.. an annotation processor. This would unbreak builds on Mac OSX JDK 1.6, which are intermittently broken due to a bug in the Reflection API (that's also mostly-completed in my patch queue).

Finally: Beware of "using Java features as normal". A surprising number of them are implemented *impressively* badly by the compiler (such as my now much-overused example of when autoboxing can use 21 bytecode instructions to perform "x++").

Comment 108

[Lucas Rocha (:lucasr)]

Just a heads up: we've discussed this in the front-end meeting last week and the consensus was that we should not land these changes until we all agree on the general direction (a couple members of the team don't agree with certain aspects of this move).

We'll be discussing this in the front-end meeting once rnewman is back from PTO.

Comment 109

[Chris Kitching [:ckitching]]

Attachment: Reduce use of proxy methods and generated getters in 'other' code

Comment 110

[Chris Kitching [:ckitching]]

Oh, (In reply to Chris Kitching [:ckitching] from comment #109)
> Created attachment 8486834 [details] [diff] [review]
> Reduce use of proxy methods and generated getters in 'other' code

Oh, that ended up here.

... Wow.


Talk about the never-ending job: here's the new stuff that regressed since we landed Bug 1051706.
... On the bright side, the program still works.

Comment 111

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.