991167 - Support UI thread event listeners in EventDispatcher

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

For a lot of events, they get re-posted to the UI thread as soon as the event listener receives them. We should add the ability to register "UI thread event listeners" to remove redundant code and also to improve thread safety. Brian also had a similar idea in bug 981852.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add BundleEventListener class (v1)

Add a BundleEventListener that's simiar to NativeEventListener or GeckoEventListener, but uses a Bundle for the event message.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add UI and background thread event listener support (v1)

Add ability to register a BundleEventListener for the UI thread or background thread. Registered events are dispatched to the target thread automatically.

Comment 3

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

Comment on attachment 8621261 [details] [diff] [review]
Add UI and background thread event listener support (v1)

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

Let's take a step back here. Now that we have more than 2 listener arrays, maybe iterating through them to find listeners isn't the best idea.

Did you consider alternative approaches? I was thinking we could have a Map<String, Map<String, List<extends Listener>>> (see below) where we map the event names to our existing Listener maps. There is a slight overhead in hashing, but it's probably better than iterating through the whole thing.

Also as a general note, casting in Java is generally frowned upon because you're losing type safety and it takes a performance hit. I think we can do a lot of these methods without casting by having inheriting from a generic Listener interface that lets listeners construct their own types, e.g.

public interface Listener {
  public void getNewInstance();
}

public interface NativeEventListener extends Listener { // or is it implements for interfaces?
  public void handleMessage(...
}

Enums also do a lot here.

::: mobile/android/base/EventDispatcher.java
@@ +92,5 @@
> +                                             final String[] events) {
> +        if (AppConstants.RELEASE_BUILD) {
> +            // for performance reasons, we only check for
> +            // already-registered listeners in non-release builds.
> +            return;

I'm sure we find a lot more of these bugs on release builds because of the larger testing pool - it's a bit of a shame to lose that, but I see we already do similar checking in some of the code.

@@ +94,5 @@
> +            // for performance reasons, we only check for
> +            // already-registered listeners in non-release builds.
> +            return;
> +        }
> +        for (final Object listenersMap : new Object[] { mGeckoThreadNativeListeners,

Can you just declare this as Map<String, ?>[] to avoid the cast below?

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Michael Comella (:mcomella) from comment #3)
> Comment on attachment 8621261 [details] [diff] [review]
> Add UI and background thread event listener support (v1)
> 
> Review of attachment 8621261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's take a step back here. Now that we have more than 2 listener arrays,
> maybe iterating through them to find listeners isn't the best idea.

So we have more than 2 listener _maps_ that we look up to find a listener array for an event, but I think that's what you meant.

> Did you consider alternative approaches? I was thinking we could have a
> Map<String, Map<String, List<extends Listener>>> (see below) where we map
> the event names to our existing Listener maps. There is a slight overhead in
> hashing, but it's probably better than iterating through the whole thing.

By "iterating through the whole thing" did you mean looking up each of the 4 listener maps? I'm hesitant to use another map because HashMaps are rather memory-inefficient (it allocates an additional object for each entry plus the hashtable itself).

I don't think going through each of the 4 maps will have too much of an impact perf-wise unless perf numbers indicate otherwise. And we're unlikely to be adding more maps in the future -- I can't think of another thread where we'd want event listeners to run on. So IMO, going through each of the maps is okay for now.

> Also as a general note, casting in Java is generally frowned upon because
> you're losing type safety and it takes a performance hit. I think we can do
> a lot of these methods without casting by having inheriting from a generic
> Listener interface that lets listeners construct their own types, e.g.
> 
> public interface Listener {
>   public void getNewInstance();
> }
> 
> public interface NativeEventListener extends Listener { // or is it
> implements for interfaces?
>   public void handleMessage(...
> }
> 
> Enums also do a lot here.

Can you clarify more? I'm not sure what getNewInstance would do exactly.

> ::: mobile/android/base/EventDispatcher.java
> @@ +92,5 @@
> > +                                             final String[] events) {
> > +        if (AppConstants.RELEASE_BUILD) {
> > +            // for performance reasons, we only check for
> > +            // already-registered listeners in non-release builds.
> > +            return;
> 
> I'm sure we find a lot more of these bugs on release builds because of the
> larger testing pool - it's a bit of a shame to lose that, but I see we
> already do similar checking in some of the code.

Right.

> @@ +94,5 @@
> > +            // for performance reasons, we only check for
> > +            // already-registered listeners in non-release builds.
> > +            return;
> > +        }
> > +        for (final Object listenersMap : new Object[] { mGeckoThreadNativeListeners,
> 
> Can you just declare this as Map<String, ?>[] to avoid the cast below?

Java doesn't allow arrays of generic types :(  You could use an array list or something, but I like the simplicity of using an inline array despite requiring a cast below.

Comment 5

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

(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> I don't think going through each of the 4 maps will have too much of an
> impact perf-wise unless perf numbers indicate otherwise. And we're unlikely
> to be adding more maps in the future -- I can't think of another thread
> where we'd want event listeners to run on. So IMO, going through each of the
> maps is okay for now.

Fair enough – if we find there are perf issues, we can always benchmark.

> > Also as a general note, casting in Java is generally frowned upon because
> > you're losing type safety and it takes a performance hit. I think we can do
> > a lot of these methods without casting by having inheriting from a generic
> > Listener interface that lets listeners construct their own types, e.g.
> > 
> > public interface Listener {
> >   public void getNewInstance();
> > }
> > 
> > public interface NativeEventListener extends Listener { // or is it
> > implements for interfaces?
> >   public void handleMessage(...
> > }
> > 
> > Enums also do a lot here.
> 
> Can you clarify more? I'm not sure what getNewInstance would do exactly.

Sorry, I should have specified that this is not directly related to this patch. getNewInstance (or better yet, getNewBackingArrayInstance – I really wasn't awake when I did this review!) this could replace the listType and subsequent listType.newInstance call in registerListener.

Not sure what I meant by the enum comment.

> > Can you just declare this as Map<String, ?>[] to avoid the cast below?
> 
> Java doesn't allow arrays of generic types :(  You could use an array list
> or something, but I like the simplicity of using an inline array despite
> requiring a cast below.

I think the cast works against simplicity – I'd rather see an ArrayList. If we were trying to keep it simple, we can make it a member variable that we only allocate on non-release builds.

Comment 6

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

(In reply to Michael Comella (:mcomella) from comment #5)
> I think the cast works against simplicity – I'd rather see an ArrayList. If
> we were trying to keep it simple, we can make it a member variable that we
> only allocate on non-release builds.

Or actually, allocate lazily (or we can have a method that returns the ArrayList w/ no member variable) – having a conditional allocation sounds like a recipe for NullPointerExceptions.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add UI and background thread event listener support (v2)

(In reply to Michael Comella (:mcomella) from comment #5)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> > > Can you just declare this as Map<String, ?>[] to avoid the cast below?
> > 
> > Java doesn't allow arrays of generic types :(  You could use an array list
> > or something, but I like the simplicity of using an inline array despite
> > requiring a cast below.
> 
> I think the cast works against simplicity – I'd rather see an ArrayList. If
> we were trying to keep it simple, we can make it a member variable that we
> only allocate on non-release builds.

Arrays.asList apparently works, so the cast is no longer needed.

Comment 8

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

Comment on attachment 8625941 [details] [diff] [review]
Add UI and background thread event listener support (v2)

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

Sorry for the delay. :d It won't happen again - thanks for being patient!

Some cleanup possible, but functionally seems correct: r+.

::: mobile/android/base/EventDispatcher.java
@@ +46,5 @@
>       */
>      private static final int GECKO_NATIVE_EVENTS_COUNT = 0; // Default for HashMap
>      private static final int GECKO_JSON_EVENTS_COUNT = 256; // Empirically measured
> +    private static final int UI_EVENTS_COUNT = 0; // Default for HashMap
> +    private static final int BACKGROUND_EVENTS_COUNT = 0; // Default for HashMap

nit: UI_EVENTS_COUNT -> DEFAULT_UI_EVENTS_COUNT

Same w/ BACKGROUND_EVENTS_COUNT & the Gecko events.

I'm also fine hard-coding

@@ +53,5 @@
>          new HashMap<String, List<NativeEventListener>>(GECKO_NATIVE_EVENTS_COUNT);
>      private final Map<String, List<GeckoEventListener>> mGeckoThreadJSONListeners =
>          new HashMap<String, List<GeckoEventListener>>(GECKO_JSON_EVENTS_COUNT);
> +    private final Map<String, List<BundleEventListener>> mUiThreadListeners =
> +        new HashMap<String, List<BundleEventListener>>(UI_EVENTS_COUNT);

Why do we initialize these values as 0? It is my interpretation that these values are here so you can set an initial value that is greater than or equal to the number of items you intend to store so you don't need to resize the underlying data structures (which is potentially expensive).

Ideally, we should try to estimate (or statically analyze) how many listeners there will be and use that value, plus some padding.

fwiw, it seems the normal default value is 16 [1] and an initial value of 0 is actually changed to the nearest power of 2 [2] so we're actually starting with 1.

[1]: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/HashMap.java#132
[2]: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/HashMap.java#197

@@ +102,5 @@
> +            if (listenersMap == allowedMap) {
> +                continue;
> +            }
> +            synchronized (listenersMap) {
> +                for (final String event: events) {

nit: event : events

@@ +155,5 @@
> +    public void registerUiThreadListener(final BundleEventListener listener,
> +                                         final String... events) {
> +        checkNotRegisteredElsewhere(mUiThreadListeners, events);
> +
> +        registerListener((Class)ArrayList.class,

nit: `(Class) ArrayList.class`

Same below for background.

Is ArrayList.class actually not of type Class? Curious!

@@ +224,5 @@
> +            // and return early. This assumption is checked when registering listeners.
> +            return;
> +        }
> +
> +        if (dispatchToThread(type, message, callback,

Can you add a comment why we prioritize this over JSON listeners?

I feel like we'd have more JSON than UI/background, at least at the moment, so this configuration would be less efficient.

@@ +265,5 @@
> +
> +                // There were native listeners, and they're gone.
> +                // Dispatch an error rather than looking for more listeners.
> +                if (callback != null) {
> +                    callback.sendError("No listeners for request");

This behavior seems inconsistent where if there are no registered listeners ever, the callback is not called. I noticed the callback is eventually called in dispatchEvent but that's not super intuitive because two separate methods take care of the callback at different times (and dispatchEvent makes assumptions that it can just call the callback, unlike dispatchToThread).

Perhaps there's a way to better organize the code for this?

@@ +275,5 @@
> +            try {
> +                messageAsBundle = message.toBundle();
> +            } catch (final NativeJSObject.InvalidPropertyException e) {
> +                Log.e(LOGTAG, "Exception occurred while handling " + type, e);
> +                return true;

Shouldn't we try to call the callback here?

@@ +280,5 @@
> +            }
> +
> +            for (final BundleEventListener listener : listeners) {
> +                thread.post(new Runnable() {
> +                    @Override public void run() {

nit: 
@Override
public void run()...

@@ +285,5 @@
> +                        listener.handleMessage(type, messageAsBundle, callback);
> +                    }
> +                });
> +            }
> +            return true;

Do we need to call callback.sendSuccess here? If not and it's because listener.handleMessage does, I'd think a comment that the status is sent via the listener would be good.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add UI and background thread event listener suppor (v2.1)

(In reply to Michael Comella (:mcomella) from comment #8)
> Comment on attachment 8625941 [details] [diff] [review]
> Add UI and background thread event listener support (v2)
> 
> Review of attachment 8625941 [details] [diff] [review]:
> -----------------------------------------------------------------

Addressed most of the comments.

> @@ +53,5 @@
> >          new HashMap<String, List<NativeEventListener>>(GECKO_NATIVE_EVENTS_COUNT);
> >      private final Map<String, List<GeckoEventListener>> mGeckoThreadJSONListeners =
> >          new HashMap<String, List<GeckoEventListener>>(GECKO_JSON_EVENTS_COUNT);
> > +    private final Map<String, List<BundleEventListener>> mUiThreadListeners =
> > +        new HashMap<String, List<BundleEventListener>>(UI_EVENTS_COUNT);
> 
> Why do we initialize these values as 0? It is my interpretation that these
> values are here so you can set an initial value that is greater than or
> equal to the number of items you intend to store so you don't need to resize
> the underlying data structures (which is potentially expensive).
> 
> Ideally, we should try to estimate (or statically analyze) how many
> listeners there will be and use that value, plus some padding.
> 
> fwiw, it seems the normal default value is 16 [1] and an initial value of 0
> is actually changed to the nearest power of 2 [2] so we're actually starting
> with 1.

On Android, an initial value of 0 means an empty table (with no capacity), but I agree we should find reasonable values in a follow-up.

> nit: `(Class) ArrayList.class`
> 
> Same below for background.
> 
> Is ArrayList.class actually not of type Class? Curious!

It's a problem with type erasure. I changed registerListener a bit so that the @SuppressWarnings and cast to Class are not required here anymore.

> @@ +265,5 @@
> > +
> > +                // There were native listeners, and they're gone.
> > +                // Dispatch an error rather than looking for more listeners.
> > +                if (callback != null) {
> > +                    callback.sendError("No listeners for request");
> 
> This behavior seems inconsistent where if there are no registered listeners
> ever, the callback is not called. I noticed the callback is eventually
> called in dispatchEvent but that's not super intuitive because two separate
> methods take care of the callback at different times (and dispatchEvent
> makes assumptions that it can just call the callback, unlike
> dispatchToThread).
> 
> Perhaps there's a way to better organize the code for this?

Maybe in a follow-up? The existing code in dispatchEvent(NativeJSObject) has similar behavior.

> @@ +275,5 @@
> > +            try {
> > +                messageAsBundle = message.toBundle();
> > +            } catch (final NativeJSObject.InvalidPropertyException e) {
> > +                Log.e(LOGTAG, "Exception occurred while handling " + type, e);
> > +                return true;
> 
> Shouldn't we try to call the callback here?

Maybe in a follow-up? The existing code in dispatchEvent don't call the callback.

Comment 10

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

(In reply to Jim Chen [:jchen] [:darchons] from comment #9)
> On Android, an initial value of 0 means an empty table (with no capacity),
> but I agree we should find reasonable values in a follow-up.

As I mentioned in comment 8, the value of 0 is actually changed to 1 so the perf is probably pretty dumpy. Using the default value might be safer.

> Maybe in a follow-up? The existing code in dispatchEvent(NativeJSObject) has
> similar behavior.

> Maybe in a follow-up? The existing code in dispatchEvent don't call the
> callback.

wfm.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #9)
> > On Android, an initial value of 0 means an empty table (with no capacity),
> > but I agree we should find reasonable values in a follow-up.
> 
> As I mentioned in comment 8, the value of 0 is actually changed to 1 so the
> perf is probably pretty dumpy. Using the default value might be safer.

The Android HashMap implementation [1] has a slightly different behavior (yay Android) where setting initialCapacity to 0 means the same thing as using default constructor. The default is having an empty table with no capacity.

[1] http://androidxref.com/5.1.1_r6/xref/libcore/luni/src/main/java/java/util/HashMap.java#143

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53a0390d59db
https://hg.mozilla.org/integration/mozilla-inbound/rev/f39b4a4435f5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53a0390d59db
https://hg.mozilla.org/mozilla-central/rev/f39b4a4435f5