Closed Bug 963208 Opened 6 years ago Closed 6 years ago

Avoid allocating GeckoEvents

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: wesj, Assigned: wesj)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch 1/2 (obsolete) — Splinter Review
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?
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)
Attached patch Patch 2/2 (obsolete) — Splinter Review
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-
Attached patch PatchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/96e8a0819361
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.