If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Avoid allocating GeckoEvents

RESOLVED FIXED in Firefox 30

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
Firefox 30
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8364505 [details] [diff] [review]
Patch 1/2

We allocate a new GeckoEvent for every message between Java->Gecko (even the non-Broadcast ones). Then we send it to C++ who immediately converts it to a C++ object. Then we throw away the Java event. We can avoid the overhead/GC associated with all that pretty easily with a little factory.

I saw this show up when trying to do some startup Allocation profiling (although I'm still trying to figure out the best way to do that). I fixed it and then had trouble reproducing :) Don't want to throw away the code, and it seems like good fixes regardless. Other opinions?
(Assignee)

Comment 1

4 years ago
Comment on attachment 8364505 [details] [diff] [review]
Patch 1/2

Not sure if we need to be more careful or not, but this seems to work fine.
Attachment #8364505 - Attachment is patch: true
Attachment #8364505 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 2

4 years ago
Created attachment 8364507 [details] [diff] [review]
Patch 2/2

This does the same thing on the C++ side. I'm not sure this is worth the effort, but figured I'd throw it up.
Attachment #8364507 - Flags: review?(bugmail.mozilla)
Comment on attachment 8364505 [details] [diff] [review]
Patch 1/2

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

::: mobile/android/base/GeckoEvent.java
@@ +632,5 @@
>      }
>  
> +    private static final int EVENT_FACTORY_SIZE = 5;
> +    // Maybe we're probably better to just make mType non final, and just store GeckoEvents in here...
> +    private static HashMap<Integer, ArrayBlockingQueue<GeckoEvent>> mEvents = new HashMap<Integer, ArrayBlockingQueue<GeckoEvent>>();

Drive-by: use SparseArray instead of HashMap (see bug 962968 for more context)
Comment on attachment 8364505 [details] [diff] [review]
Patch 1/2

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

Minusing for thread unsafety but looks fine otherwise.

::: mobile/android/base/GeckoEvent.java
@@ +633,5 @@
>  
> +    private static final int EVENT_FACTORY_SIZE = 5;
> +    // Maybe we're probably better to just make mType non final, and just store GeckoEvents in here...
> +    private static HashMap<Integer, ArrayBlockingQueue<GeckoEvent>> mEvents = new HashMap<Integer, ArrayBlockingQueue<GeckoEvent>>();
> +    public static GeckoEvent get(NativeGeckoEvent type) {

Move these fields and get function up to the top of the file. Also, I think this get function should be private since it's not needed outside this file.

@@ +637,5 @@
> +    public static GeckoEvent get(NativeGeckoEvent type) {
> +        if (mEvents.containsKey(type.value)) {
> +            ArrayBlockingQueue<GeckoEvent> events = mEvents.get(type.value);
> +            if (events.size() > 0) {
> +                return events.poll();

Both the get and recycle functions need to be synchronized on something since they're used from all sorts of threads. In particular this part is unsafe: you could have two threads both seeing events.size() > 0, one of them grabbing the item with the call to poll() and the other returning null. There's probably other threading problems too.
Attachment #8364505 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 8364507 [details] [diff] [review]
Patch 2/2

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

I don't think this one is worth the code. jemalloc already is pretty efficient with short-lived allocations so our existing code patterns here should be fine. Unless there's actual profiling data showing that we're spending a lot of time in the new/deletes then I wouldn't bother with this.

Alternatively you could run through a million NOP events using the original code and your new code and time it to see if there's a difference.
Attachment #8364507 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 6

4 years ago
Created attachment 8380132 [details] [diff] [review]
Patch
Attachment #8364505 - Attachment is obsolete: true
Attachment #8364507 - Attachment is obsolete: true
Attachment #8380132 - Flags: review?(bugmail.mozilla)
Comment on attachment 8380132 [details] [diff] [review]
Patch

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

This looks fine. I probably would have just used a regular ArrayList and checked the size manually rather than using an ArrayBlockingQueue, but whatever.

::: mobile/android/base/FilePickerResultHandler.java
@@ +96,5 @@
>          try {
>              // Try a query to make sure the expected columns exist
>              final ContentResolver cr = fa.getContentResolver();
>              Cursor cursor = cr.query(uri, new String[] { MediaStore.Video.Media.DATA }, null, null, null);
> +            cursor.getColumnIndexOrThrow(MediaStore.Video.Media.DATA);

Is this part of this patch? Seems like a bad qref.

::: mobile/android/base/GeckoEvent.java
@@ +30,5 @@
>  import android.view.MotionEvent;
>  
>  import java.nio.ByteBuffer;
>  
> +import java.util.concurrent.ArrayBlockingQueue;

nit: no blank line between java.* imports.

@@ +62,5 @@
> +    }
> +
> +    public void recycle() {
> +        synchronized (mEvents) {
> +        	ArrayBlockingQueue<GeckoEvent> events = mEvents.get(mType);

nit: indentation
Attachment #8380132 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/96e8a0819361
https://hg.mozilla.org/mozilla-central/rev/96e8a0819361
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.