732177 - Clean up handleMessage() in GeckoApp

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Sriram Ramasubramanian [:sriram]]

The handleMessage() handling message from Gecko in GeckoApp seems to be like a big blob of text. It's hard to understand what is happening with each message in the very long if..else structure.

I suggest doing something like,

switch(getEvent(event)) {
  case GeckoEvent.DOCUMENT_START:
   ...
  case GeckoEvent.DOCUMENT_STOP:
}

where getEvent() uses a HashMap<String, int> to return the event type. This seems much better than individual string comparisons -- given that this particular function is the "heart" of all message passing.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Aaah, that looks so clean! ;)

As mentioned in previous comment, now everything moves to a switch-case block. This is more visually readable, and string comparisons in HashMaps are faster (I guess).

I know you wouldn't like adding spaces in mapping.put() ;). But I tried it without spaces and it was a mess. :( Please let me add spaces there just for this.

Comment 2

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

Comment on attachment 602182 [details] [diff] [review]
Patch

You don't need to manually do "new Integer" everywhere; java will do that for you. Also calling containsKey followed by get does the lookup twice; you can just use get and check foe null.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Ah, I wanted to change the | new Integer | cast. I forgot. Will change and upload a new patch.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This has the changes mentioned by kats.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 602489 [details] [diff] [review]
Patch

The original intent of handleMessage was to move the handlers to code that better encapsulated the activity. We moved some to Tabs, Awesomebar and GeckoPreferences. Maybe we need to move more out of GeckoApp.

Not ready to see this land. Need more reason to make the change. Lastly, the patch depends on bug 727307 which I am also not happy about.

Comment 6

[Brian Nicholson (:bnicholson)]

In the past, we've discussed options for improving the efficiency of message handling -- one option would be to replace the message strings with ints in both Java and JS. This would remove all the string comparison overhead we're currently running into in our message handling (though it's not clear how significant this overhead is).

Wes, do you think either of the interns might be interested in working on this?

Flagging for tracking just to get this on the radar since this could give us some performance wins.

Comment 7

[Wesley Johnston (:wesj)]

We're running lots of profiling now. This hasn't shown up in any. It sounds nice from an theoretical performance point of view, but I don't think its worth putting time into.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 9

[:Margaret Leibovic]

With Java 7, we can use strings in switch statements, so this should be a lot easier!

Comment 10

[Vivek Balakrishnan[:vivek]]

Attachment: 732177.patch

Try server logs:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1ccf69080f83

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8536317 [details] [diff] [review]
732177.patch

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

Splinter says this is a windows patch which I'm guessing affects EOL - can you convert this to unix EOL?

::: mobile/android/base/GeckoApp.java
@@ +270,5 @@
> +        }
> +
> +        public static NativeEventType fromValue(String value) {
> +            if (value != null) {
> +                for (NativeEventType event : values()) {

While this cleans up the code, the performance is actually worse than the previous implementation because we still have to compare this String against all the event's defining Strings (equivalent perf) and then do comparisons for the enum.

We should use a Map for this method (though I wonder how performant the hashing methods are anyway... profile?).

@@ +626,5 @@
>  
>      @Override
>      public void handleMessage(final String event, final NativeJSObject message,
>                                final EventCallback callback) {
> +        switch(NativeEventType.fromValue(event)) {

nit: switch (

@@ +772,5 @@
> +            case UPDATE_INSTALL:
> +                startService(new Intent(
> +                        UpdateServiceHelper.ACTION_APPLY_UPDATE, null, this, UpdateService.class));
> +            break;
> +            default:

nit: whitespace above

Yay default cases! :D

@@ +780,5 @@
>  
>      @Override
>      public void handleMessage(String event, JSONObject message) {
>          try {
> +            switch(GeckoEventType.fromValue(event)) {

nit: switch (

@@ +809,5 @@
> +                }
> +
> +                case ACCESSIBILITY_EVENT:
> +                    GeckoAccessibility.sendAccessibilityEvent(message);
> +                    break;

nit: Add default case.

@@ +1665,5 @@
>          EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener)this,
> +            GeckoEventType.GECKO_READY.toString(),
> +            GeckoEventType.GECKO_DELAYED_STARTUP.toString(),
> +            GeckoEventType.ACCESSIBILITY_EVENT.toString(),
> +            GeckoEventType.NATIVEAPP_ISDEBUGGABLE.toString());

Let's avoid code duplication here (with unregister) - can you add a registerGeckoThreadListener method that supports Iterables, create a method that turns an array of these values, and thus can be passed to the Iterable version.

If you need to keep the old registerGeckoThreadListener method, see [1] for a dual implementation w/ varargs.

[1]: http://stackoverflow.com/a/6898691

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Btw, I'm going to need a green try push to r+ this.

Comment 13

[Vivek Balakrishnan[:vivek]]

Attachment: 732177.patch

Revised patch using Hashmap instead of Enums

Comment 14

[Vivek Balakrishnan[:vivek]]

try logs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e3f3fb9605

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8540791 [details] [diff] [review]
732177.patch

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

Richard, the idea of this approach looks like an improvement to me. How do you feel about it?

::: mobile/android/base/GeckoApp.java
@@ +583,5 @@
>      @Override
>      public void handleMessage(final String event, final NativeJSObject message,
>                                final EventCallback callback) {
> +        // Defend against NPE.
> +        if (!NATIVE_EVENT_MAP.containsKey(event)) {

We do two lookups with this - containsKey and get. Instead...

final Integer eventID = NATIVE_EVENT_MAP.get(event);
if (eventID == null) {
  throw ...; // You forgot to return, but I think throwing is better - we should never have this issue unless there is a programmer error in setting keys.
}

switch (eventID) {

@@ +588,5 @@
> +            Log.w(LOGTAG, "Unknown native event " + event + " received");
> +        }
> +
> +        switch (NATIVE_EVENT_MAP.get(event)) {
> +            case 1:

These definitely have to be named - perhaps use enums instead of ints?

NATIVE_EVENT_MAP = new HashMap<String, NativeEventName>();

@@ +651,5 @@
>  
> +            case 6: {
> +                String src = message.getString("url");
> +                setImageAs(src);
> +                break;

nit: Some breaks are inside the case scope, some are outside - be consistent.

@@ +1625,5 @@
>          //app state callbacks
>          mAppStateListeners = new LinkedList<GeckoAppShell.AppStateListener>();
>  
>          //register for events
> +        EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener)this, GECKO_EVENT_MAP.keySet());

Cool.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Vivek, did you happen to profile the change in performance?

Comment 17

[Vivek Balakrishnan[:vivek]]

As agreed over irc, I'll have this profiled by next week.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8540791 [details] [diff] [review]
732177.patch

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

Re EventDispatcher changes: cool, but let's bear in mind perf.

Re GeckoApp: I'm not convinced that these changes help, for two reasons.

Firstly, you haven't improved readability (especially not without named constants or enums!). We still have a big switch statement, and now that we use the Java 7 source level we could do that for string constant event names:

  switch (event) {
    case "Accessibility:Ready":
      ...

Secondly, this stands a good chance of regressing performance.

The hash lookup probably won't be cheap. We'll hash the string, look it up in the map, return some integer or enum, and then run that through a switch statement. javac *might* be smart enough to turn that lookup into a jump offset (probably not for the enum), but maybe not.

String comparisons, OTOH, are fast. They also boil down to == in the best case, which will be wherever we initiate a message from Java (the compiler is usually smart enough to ensure that constant strings are interned, so they'll be really-really equal). Worst-case we compare a few characters and move on to the next branch. Not faster than a numeric ==, but in the common case quite possibly faster than hashing a string and nosing through a HashMap.

I encourage you to profile, but I bet the profile doesn't show any wins, and maybe shows some losses. I also encourage you to ask ckitching about this kind of micro-optimization; reading the bytecode (or asking someone who can compile it in his head) will save some time.


My suggested approaches:

1. If you're going to be a monkey, be a gorilla: introduce a central message name definition location, use the same one from both Java and Gecko, and just use ints everywhere. That way there's definitely no string comparison going on, and no hashing either.

2. If you want to be a medium-sized monkey, at least be a statistical primate. Figure out which messages are sent most often, and move those to the top of the decision tree. Boom, instant perf win. Use a Java 7 string-switch for readability (compiles to the same bytecode), and be done with it. You can also do some fancy math, if you choose: string equality is defined in terms of pointer equality, then string length equality, then char-by-char. Use that to decide which order to use to win the quickest comparisons.

::: mobile/android/base/EventDispatcher.java
@@ +106,5 @@
>      }
>  
>      @SuppressWarnings("unchecked")
> +    public void registerGeckoThreadListener(final NativeEventListener listener, final String... events) {
> +        registerGeckoThreadListener(listener, Arrays.asList(events));

I'd rather have a small amount of duplicated code here, rather than allocating a new list during time-sensitive parts of runtime.

::: mobile/android/base/GeckoApp.java
@@ +741,5 @@
>      public void handleMessage(String event, JSONObject message) {
>          try {
> +            // Defend against NPE.
> +            if (!GECKO_EVENT_MAP.containsKey(event)) {
> +                Log.w(LOGTAG, "Unknown gecko event " + event + " received");

This is an error. I'd suggest crashing hard on pre-release builds here.

Comment 19

[Chris Kitching [:ckitching]]

Comment on attachment 8540791 [details] [diff] [review]
732177.patch

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

(In reply to Richard Newman [:rnewman] from comment #18)
> Firstly, you haven't improved readability (especially not without named
> constants or enums!). We still have a big switch statement, and now that we
> use the Java 7 source level we could do that for string constant event names:
> 
>   switch (event) {
>     case "Accessibility:Ready":
>       ...
> 
> Secondly, this stands a good chance of regressing performance.
> 
> The hash lookup probably won't be cheap. We'll hash the string, look it up
> in the map, return some integer or enum, and then run that through a switch
> statement.

This.
Using a hashmap, assuming the keys distribute well between the buckets and don't have zillions of collisions that cause the map to degrade toward linear behaviour, will lead to performance that is O(1). Ish. It'll be a very expensive constant time, though, for the reasons Richard pointed out. It'd probably be a useful optimisation if you have an extraordinarily large number of keys, but you don't.

> String comparisons, OTOH, are fast. They also boil down to == in the best
> case, which will be wherever we initiate a message from Java (the compiler
> is usually smart enough to ensure that constant strings are interned, so
> they'll be really-really equal). Worst-case we compare a few characters and
> move on to the next branch.

It is for this reason that I wish you'd stop starting every message name with "Gecko" - you're wasting five comparisons each time :P .

As for switch statements, Java 7's support for strings in switch is entirely syntactic sugar. The compiler transforms such a switch into a chain of ifs before compilation (so order of cases still matters for performance, it's still linear in branch count, and you're still calling String.equals).
In general, for integer keys, a switch statement will compile to either a tableswitch or a lookupswitch instruction. The former uses the integer key as an index into an array of jump addresses, meaning the JVM simply looks up the address to jump to using the switched-upon value as an offset and jumps there: constant time (and very possibly one instruction post-JIT). If the integer keys are very sparse (such that the jump table would be very large), then lookupswitch is used instead. This creates a list of (key, jump-address) pairs, ordered by key. The instruction performs a binary search on this list looking for the switched-upon value, and jumps there. Log-time, due to the search.

Switching on an enum is identical to switching on an integer: it just switches on the ordinal values. Since these are typically ascending integers, you get a tableswitch. Someone decompiled an example here:
http://stackoverflow.com/questions/4922932/java-performance-of-enums-vs-if-then-else

> Not faster than a numeric ==, but in the common
> case quite possibly faster than hashing a string and nosing through a
> HashMap.
> 
> I encourage you to profile, but I bet the profile doesn't show any wins, and
> maybe shows some losses.

Also, benchmarking micro-optimisations is tricky. People are often tempted to put the thing they want to test in a loop so they can run it many times and time it. Doing so, however, incurs the wrath of the JIT and will lead to you always being told your optimisation is a waste of time.
People keep doing that to test the usefulness of my patches. It's amusing. :P. It's a tricky one, though, make sure you don't fall into that trap.

> My suggested approaches:
> 
> 1. If you're going to be a monkey, be a gorilla: introduce a central message
> name definition location, use the same one from both Java and Gecko, and
> just use ints everywhere. That way there's definitely no string comparison
> going on, and no hashing either.

Do this.

> 2. If you want to be a medium-sized monkey, at least be a statistical
> primate. Figure out which messages are sent most often, and move those to
> the top of the decision tree. Boom, instant perf win. Use a Java 7
> string-switch for readability (compiles to the same bytecode), and be done
> with it.

Do this first (since it's probably much simpler than the "proper" solution of ceasing to use Strings for this).

Note that you can gain the readability by switching to "switch" instantly, for free, and with zero affect on the binary whatsoever.

::: mobile/android/base/EventDispatcher.java
@@ +92,5 @@
>      }
>  
>      private <T> void unregisterListener(final Map<String, List<T>> listenersMap,
>                                          final T listener,
> +                                        final Collection<String> events) {

Realise that iteration over a Collection is many times slower than iterating over an array.

If this is time-sensitive code, you really want to keep the array implementation (unless you have a really compelling reason not to I missed).

A few lines down we do this:
for (final String event : events) { ... }

This is the "enhanced for loop", which the compiler desugars into one of two forms.
For an array, it is changed to:

for (int i = 0; i < events.length; i++) { ... }

The overhead of this loop is just an integer increment, and the cost of getting the element out is just an array access.

The other form is what happens if you use the enhanced for loop on an Iterable (as you are now doing, because Collection is Iterable):

Iterator<String> i = events.iterator();
while (i.hasNext()) {
    final String event = i.next();
    ...
}

This loop is much more expensive: We have the additional cost of constructing the Iterator two extra (non-inlineable) function calls for each iteration.

::: mobile/android/base/GeckoApp.java
@@ +205,5 @@
>      abstract protected String getDefaultProfileName() throws NoMozillaDirectoryException;
>  
>      private static final String RESTARTER_ACTION = "org.mozilla.gecko.restart";
>      private static final String RESTARTER_CLASS = "org.mozilla.gecko.Restarter";
> +    private static final Map<String, Integer> GECKO_EVENT_MAP = new HashMap<>();

On a random note, a better way to make a constant map is to use an instance initialiser, like this:

private static final Map<String, Integer> GECKO_EVENT_MAP = Collections.unmodifiableMap(new HashMap<String, Integer>() {
    {
        put("Accessibility:Event", 1);
        put("Gecko:DelayedStartup", 2);
        ...
    }
});

The ensuing map cannot ever be modified.

Comment 20

[Chris Kitching [:ckitching]]

It occurs to me that the implementation of tableswitch probably suggests that the "default" branch on that sort of switch statement would be marginally faster than the others, as the interpreter would have to do bounds checks on the jump table first (jumping to the default branch if they fail) before dereferncing the pointer relative to the jump table.

Hmm.

Totally offtopic. Sort of fun, though.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Thanks for the fun tidbits! :) For my edification...

(In reply to Chris Kitching [:ckitching] from comment #19)
> Also, benchmarking micro-optimisations is tricky. People are often tempted
> to put the thing they want to test in a loop so they can run it many times
> and time it. Doing so, however, incurs the wrath of the JIT and will lead to
> you always being told your optimisation is a waste of time.

What do you mean by incurs the wrath of the JIT? A link will suffice.

> People keep doing that to test the usefulness of my patches. It's amusing.
> :P. It's a tricky one, though, make sure you don't fall into that trap.

What is an optimal way to test a micro-optimized patch?

> > 2. If you want to be a medium-sized monkey, at least be a statistical
> > primate. Figure out which messages are sent most often, and move those to
> > the top of the decision tree. Boom, instant perf win. Use a Java 7
> > string-switch for readability (compiles to the same bytecode), and be done
> > with it.
> 
> Do this first (since it's probably much simpler than the "proper" solution
> of ceasing to use Strings for this).

Sounds good - Vivek, do you want to move forward with this (if not, we can always reassign)? Start with changing everything to Java 7 switch statements, then you can re-order for perf (in a follow-up even). File a follow-up for Kitching's 1. too please.

Sorry to initially lead you in the wrong direction, Vivek! Though, imo, this was a pretty educational side step! :D

Comment 22

[Chris Kitching [:ckitching]]

(In reply to Michael Comella (:mcomella) from comment #21)
> What do you mean by incurs the wrath of the JIT? A link will suffice.
> 
> What is an optimal way to test a micro-optimized patch?

The JIT looks to optimise hotspots in code, typically just a couple of basic blocks at a time. Tight loops which do something a million times for benchmarking are certainly going to get its attention (as your program will be spending very nearly all its time in that loop).

The JIT is a (fairly poor in Android's case) optimising compiler. It often notices that your test function is a waste of time and optimises it out. You then find that your first three(ish) iterations are slow, and the following 999,997 take no time at all (as they're *just* the print statements).
Someone once gave me an r- after doing this. That was a fun conversation :P.

Even if you avoid the hilarious case, the JIT makes things harder to reason about. The JIT in Dalvik really likes to process just a few basic blocks at a time (you can read a little more about this here, bonus points for finding the video:
http://dl.google.com/googleio/2010/android-jit-compiler-androids-dalvik-vm.pdf ).
Its tendency to do this is a bit of a pain when you have a big complex system. It's likely the system doesn't *have* any nice hotspots for the JIT to poke with a stick, but it'll probably have thousands of warm-ish bits. These warm-ish bits won't be quite hot enough to get processed promptly, so may be executed a very large number of times in interpreted mode (and may never be processed at all). It's these warm-spots that benefit particularly well from micro-optimisations, but since their warm-ish-ness isn't preserved under benchmarking in isolation convincing reviewers you've not lost your mind can be challenging.

To make things even more exciting, the HotSpot JVM usually processes an entire method at a time. If you go run the system on a desktop JVM you'll find that as soon as the whole big function becomes hot, the JIT will eat the whole thing, once again dwarfing your micro-optimisation. You may be able to observe improvement here, but since the HotSpot JIT is a really rather good optimising compiler (in contrast to Dalvik's, which does very little optimisation), it's very likely that it already did your optimisation, along with many many more, at JIT-time.

Just because an optimisation isn't interesting post-JIT isn't a reason to not do it (unless it's hurting readability a lot or something), due to these "warm-spots" I described earlier. In the presence of a method-granularity JIT, particularly one with good optimisation like HotSpot's, such work is less pointful.

Sort-of sane approaches include:
- Do something that causes the whole shaboodle to be executed a large number of times, run this in an instrumenting (not sampling!) profiler, and check the average execution time for the small thing you improved (which will most likely be a number in microseconds, which also upsets reviewers who aren't good at multiplying by a million).
- Run it on a desktop JVM with the JIT turned off (this is correct for certain sorts of optimisations, but you now have to consider the different behaviour of Dalvik vs. the desktop JVM. Different bytecode instructions are differently expensive between the two platforms, which may skew your results).
- Run a large number of iterations first to *ensure* it's been JIT'd, and then make your measurements. This is useful only if you believe your optimisation affects the post-JIT performance. This is more problematic than you might think, however, thanks to such absurdity on some platforms as "dynamic deoptimisation". There's a rather good SO thread discussing this topic here:
http://stackoverflow.com/questions/504103/how-do-i-write-a-correct-micro-benchmark-in-java
That thread also links to this rather fun article:
https://www.ibm.com/developerworks/java/library/j-jtp02225/
Be mindful though of the way these are mostly discussing desktop JVMs (HotSpot and friends) which have radically different JIT characteristics to Dalvik. ART is another kettle of fish entirely.
- Check bytecode *length*. If you're really stuck, the solution with the least bytecode is *probably* marginally better, as the JIT finds them easier to swallow. (In general, some particularly dense desktop JVMs have been known to give up in the face of extremely large functions).
- Appeal to theory.
- Electrocute your reviewer.

It's a tricky one. I typically use the profiler approach I mentioned in the event I actually want to measure it.
Some things may be sufficiently awkward to measure that you stop caring and just appeal to theory (I do that a lot).

Comment 23

[Chris Kitching [:ckitching]]

I should also highlight the extremely excellent name for the "The JIT optimised out your benchmark" problem: Heisenbenchmark.

Comment 24

[Richard Newman [:rnewman]]

> It's a tricky one. I typically use the profiler approach I mentioned in the
> event I actually want to measure it.
> Some things may be sufficiently awkward to measure that you stop caring and
> just appeal to theory (I do that a lot).

On this tangent: it's amusing that one of the first things drummed into performance-oriented engineers is "don't trust your instincts; profile, then optimize".

When it comes down to the behavior of systems like this in the large, you end up having to measure at incredibly coarse granularity -- e.g., time to page load -- because attempting to measure subsystems affects the experiment itself.

At coarse granularity your improvements disappear in the noise, and you end up having to trust to instinct… or its more formal cousin, theory.

In this case, at least, we know that using stringswitch won't make anything worse (but it will add readability), and we're confident that removing strings from the problem entirely is very likely to yield the best end solution. Lucky!

Comment 25

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #24)
> On this tangent: it's amusing that one of the first things drummed into
> performance-oriented engineers is "don't trust your instincts; profile, then
> optimize".

You might enjoy this article:
http://www.joshbarczak.com/blog/?p=580

Comment 26

[Vivek Balakrishnan[:vivek]]

I'll try to get back to this bug at a later point.

Comment 27

[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.