Last Comment Bug 906088 - move preferences get/observe from java into native-ish code
: move preferences get/observe from java into native-ish code
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 26
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
Depends on: 908919 909380
Blocks: 908675 908676
  Show dependency treegraph
 
Reported: 2013-08-16 09:32 PDT by Nathan Froyd [:froydnj]
Modified: 2013-09-13 13:24 PDT (History)
6 users (show)
nfroyd: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - pass String[] to PrefsHelper.getPrefs wherever possible (4.02 KB, patch)
2013-08-16 09:33 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review+
Details | Diff | Splinter Review
part 1b - add ArrayList getPrefs method to PrefsHelper (2.86 KB, patch)
2013-08-16 09:34 PDT, Nathan Froyd [:froydnj]
bnicholson: review+
Details | Diff | Splinter Review
part 2 - factor out reading an nsString from a Java string in AndroidJavaWrappers.cpp (3.20 KB, patch)
2013-08-16 09:34 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review+
Details | Diff | Splinter Review
part 3 - add Preferences events to AndroidGeckoEvent (8.00 KB, patch)
2013-08-16 09:35 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review+
Details | Diff | Splinter Review
part 4 - refactor browser.js's getPreferences function into a JSON frontend and a "raw" backend (2.89 KB, patch)
2013-08-16 09:36 PDT, Nathan Froyd [:froydnj]
mark.finkle: review+
Details | Diff | Splinter Review
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON (12.43 KB, patch)
2013-08-16 09:39 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review+
mark.finkle: review+
Details | Diff | Splinter Review
part 6 - update tests to use the new API (22.94 KB, patch)
2013-08-22 12:06 PDT, Nathan Froyd [:froydnj]
bugmail: feedback+
Details | Diff | Splinter Review
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON (12.75 KB, patch)
2013-08-22 14:24 PDT, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 6 - update tests to use the new API (22.88 KB, patch)
2013-08-22 14:25 PDT, Nathan Froyd [:froydnj]
bugmail: review+
Details | Diff | Splinter Review
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON (17.07 KB, patch)
2013-09-13 04:08 PDT, Nathan Froyd [:froydnj]
mark.finkle: review+
Details | Diff | Splinter Review
part 6 - update tests to use the new API (24.25 KB, patch)
2013-09-13 04:10 PDT, Nathan Froyd [:froydnj]
bugmail: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2013-08-16 09:32:53 PDT

    
Comment 1 Nathan Froyd [:froydnj] 2013-08-16 09:33:36 PDT
Created attachment 791325 [details] [diff] [review]
part 1 - pass String[] to PrefsHelper.getPrefs wherever possible

We want JSONArray to go away, since we're going to be transferring strings via JNI.
Comment 2 Nathan Froyd [:froydnj] 2013-08-16 09:34:20 PDT
Created attachment 791327 [details] [diff] [review]
part 1b - add ArrayList getPrefs method to PrefsHelper

Related to the last patch, this is the lone place where we transfer an ArrayList.
Just move the code into PrefsHelper in case anybody ever wants to do something similar.
Comment 3 Nathan Froyd [:froydnj] 2013-08-16 09:34:58 PDT
Created attachment 791329 [details] [diff] [review]
part 2 - factor out reading an nsString from a Java string in AndroidJavaWrappers.cpp

We duplicate code in several places, and we're going to need this same code for reading
a String[].
Comment 4 Nathan Froyd [:froydnj] 2013-08-16 09:35:35 PDT
Created attachment 791330 [details] [diff] [review]
part 3 - add Preferences events to AndroidGeckoEvent

This is just the C++ side of things; it's simpler to have this code split out as
a separate patch to review.
Comment 5 Nathan Froyd [:froydnj] 2013-08-16 09:36:57 PDT
Created attachment 791331 [details] [diff] [review]
part 4 - refactor browser.js's getPreferences function into a JSON frontend and a "raw" backend

This is a small patch, but I think it makes part 5 a little easier to understand.
In part five, we're going to be introducing XPCOM methods that browser.js needs to
implement.  That patch is going to require something very much like handlePreferencesRequest,
so just make it visible here and then part 5 should be much easier to understand.
Comment 6 Nathan Froyd [:froydnj] 2013-08-16 09:39:02 PDT
Created attachment 791332 [details] [diff] [review]
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON

This is the big patch that hooks up handling of the preferences AndroidGeckoEvent type
in nsAppShell and routes requests to JS through our normal mechanism, rather than
through the observer service.

Overhead for JSON serialization drops away in traces due to this patch and I daresay
it might even be slightly more efficient.  Haven't figured out a good way of measuring
that, yet, since it requires monitoring several languages at once...
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2013-08-17 21:02:01 PDT
Comment on attachment 791325 [details] [diff] [review]
part 1 - pass String[] to PrefsHelper.getPrefs wherever possible

>+        final String[] prefs = { PREF_SCROLLING_FRICTION_FAST,
>+                                 PREF_SCROLLING_FRICTION_SLOW,
>+                                 PREF_SCROLLING_MAX_EVENT_ACCELERATION,
>+                                 PREF_SCROLLING_OVERSCROLL_DECEL_RATE,
>+                                 PREF_SCROLLING_OVERSCROLL_SNAP_LIMIT,
>+                                 PREF_SCROLLING_MIN_SCROLLABLE_DISTANCE };

I'm not sure if there is a convention or not, but since we *love* our 80 chars in Java code, could we use this format:

          final String[] prefs = {
              PREF_SCROLLING_FRICTION_FAST,
              PREF_SCROLLING_FRICTION_SLOW,
              PREF_SCROLLING_MAX_EVENT_ACCELERATION,
              PREF_SCROLLING_OVERSCROLL_DECEL_RATE,
              PREF_SCROLLING_OVERSCROLL_SNAP_LIMIT,
              PREF_SCROLLING_MIN_SCROLLABLE_DISTANCE
          };
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2013-08-17 21:08:21 PDT
Comment on attachment 791331 [details] [diff] [review]
part 4 - refactor browser.js's getPreferences function into a JSON frontend and a "raw" backend


>   getPreferences: function getPreferences(aPrefsRequest, aListen) {
>+    this.handlePreferencesRequest(aPrefsRequest.requestId,
>+                                  aPrefsRequest.preferences,
>+                                  aListen);

No need to wrap these. We don't limit to 80 in this file.

>+  },
>+
>+  handlePreferencesRequest: function handlePreferencesRequest(aRequestId,
>+                                                              aPrefNames,
>+                                                              aListen) {

Same here
Comment 9 Kartikaya Gupta (email:kats@mozilla.com) 2013-08-18 03:46:32 PDT
Don't forget to also update the tests in mobile/android/base/tests. Some of that code also reads prefs via Preferences:Get.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2013-08-19 07:44:35 PDT
Comment on attachment 791332 [details] [diff] [review]
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON


>diff --git a/widget/android/nsIAndroidBridge.idl b/widget/android/nsIAndroidBridge.idl

> interface nsIAndroidBrowserApp : nsISupports {
>   nsIBrowserTab getBrowserTab(in int32_t tabId);
>+  void getPreferences(in int32_t requestId,
>+                      [array, size_is(count)] in wstring prefNames,
>+                      in unsigned long count);
>+  void observePreferences(in int32_t requestId,
>+                          [array, size_is(count)] in wstring prefNames,
>+                          in unsigned long count);

The only thing about this patch that I'd want to change would be this part. Could we create a new nsIBrowserPreferences { get(...), observe(...) } interface? We'd add a getBrowserPreferences to nsIBrowserApp.

You could keep the impl of these methods in BrowserApp (in browser.js) for now. Meaning:

  getBrowserPreferences: function() {
    return this;
  }

When we decide to move the preference code out of BrowserApp, we can update this imp.
Comment 11 Nathan Froyd [:froydnj] 2013-08-20 08:08:01 PDT
Comment on attachment 791327 [details] [diff] [review]
part 1b - add ArrayList getPrefs method to PrefsHelper

Apparently I didn't set reviewers properly on this one.
Comment 12 Brad Lassey [:blassey] (use needinfo?) 2013-08-20 14:52:26 PDT
Comment on attachment 791332 [details] [diff] [review]
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON

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

::: mobile/android/base/PrefsHelper.java
@@ +58,2 @@
>              GeckoAppShell.sendEventToGecko(event);
>          } catch (Exception e) {

I wonder what exceptions this is here to to catch
Comment 13 Brian Nicholson (:bnicholson) 2013-08-20 19:12:12 PDT
Comment on attachment 791327 [details] [diff] [review]
part 1b - add ArrayList getPrefs method to PrefsHelper

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

I'm not sure I understand this patch. Before, we had 'new JSONArray(prefs)' to do the ArrayList->JSONArray conversion. Now you manually iterate over the ArrayList to populate the JSONArray -- why the change? 'new JSONArray(prefs)' seems like a more concise way to do the same thing.

I also don't see the need for a second getPrefs() here since we do this in only one place as you said, but I don't have a strong opinion either way.
Comment 14 Nathan Froyd [:froydnj] 2013-08-21 04:59:45 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #13)
> I'm not sure I understand this patch. Before, we had 'new JSONArray(prefs)'
> to do the ArrayList->JSONArray conversion. Now you manually iterate over the
> ArrayList to populate the JSONArray -- why the change? 'new
> JSONArray(prefs)' seems like a more concise way to do the same thing.

Ah, I'm not quite sure why I did it that way either.  I can change it.  In either event, the JSONArray bits go away in patch 5.

> I also don't see the need for a second getPrefs() here since we do this in
> only one place as you said, but I don't have a strong opinion either way.

I think it is marginally nicer to have clients be able to pass in the data in their preferred format and have PreferencesHelper deal with converting it to a String[].
Comment 15 Brian Nicholson (:bnicholson) 2013-08-21 14:16:22 PDT
Comment on attachment 791327 [details] [diff] [review]
part 1b - add ArrayList getPrefs method to PrefsHelper

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

(In reply to Nathan Froyd (:froydnj) from comment #14)
> Ah, I'm not quite sure why I did it that way either.  I can change it.  In
> either event, the JSONArray bits go away in patch 5.

I didn't realize this code was dropped in patch 5, so it's not really an issue. LGTM.
Comment 16 Nathan Froyd [:froydnj] 2013-08-22 12:06:56 PDT
Created attachment 794182 [details] [diff] [review]
part 6 - update tests to use the new API

This patch changes all the tests to use the new, specialized preferences events.  It's
pretty straightforward, but it's causing testMasterPassword (which is already intermittent)
to perma-orange on try:

https://tbpl.mozilla.org/?tree=Try&rev=36db0d36daa2 (with some known rc1 failures)
https://tbpl.mozilla.org/?tree=Try&rev=1319f8947c1c (with changes to fix rc1 failures)

Do these changes look sane enough?  The console messages in the log suggest that I should
go looking in the XPCOM bits to make sure we're passing preferences to JS correctly, but
it's also possible the testMasterPassword bits are just poorly written.
Comment 17 Kartikaya Gupta (email:kats@mozilla.com) 2013-08-22 12:48:19 PDT
Comment on attachment 794182 [details] [diff] [review]
part 6 - update tests to use the new API

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

These changes look sane enough, although I would prefer using a single "random number" for the id rather than a handful.

I looked at the second try link you have there and the testMasterPassword failure appears to be related to this in the console output:

12:29:54     INFO -  08-22 12:29:25.421 E/GeckoConsole( 3133): MasterPassword.setPassword: TypeError: aPrefNames is undefined

It should be not too hard to track that down, I think. The exception is logged at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/MasterPassword.js#48 so it's coming from somewhere in that block. Not sure if that try run had the latest version of your patches or not.
Comment 18 Nathan Froyd [:froydnj] 2013-08-22 13:07:22 PDT
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> Comment on attachment 794182 [details] [diff] [review]
> part 6 - update tests to use the new API
> 
> Review of attachment 794182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These changes look sane enough, although I would prefer using a single
> "random number" for the id rather than a handful.
> 
> I looked at the second try link you have there and the testMasterPassword
> failure appears to be related to this in the console output:
> 
> 12:29:54     INFO -  08-22 12:29:25.421 E/GeckoConsole( 3133):
> MasterPassword.setPassword: TypeError: aPrefNames is undefined
> 
> It should be not too hard to track that down, I think. The exception is
> logged at
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> MasterPassword.js#48 so it's coming from somewhere in that block. Not sure
> if that try run had the latest version of your patches or not.

It didn't, but the second try run (which did have the latest version of the patches) is showing the same error.  I didn't modify the setPreferences (Preferences:Set) path, though, so I'm slightly skeptical that my changes had anything to do with this.  And there's no reference to aPrefNames in MasterPassword.js (though there is in my browser.js changes)...
Comment 19 Nathan Froyd [:froydnj] 2013-08-22 14:24:08 PDT
Created attachment 794262 [details] [diff] [review]
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON

Small fix to notifyPrefObservers to call getPreferences with the new argument style.
Carrying over r+.
Comment 20 Nathan Froyd [:froydnj] 2013-08-22 14:25:35 PDT
Created attachment 794263 [details] [diff] [review]
part 6 - update tests to use the new API

Fixed testPrefsObserver to use the correct requestId for unregistering observers.
Changed all tests to use th same hexadecimal constant, per Kats's feedback.  Asking
for actual review now.  Local robocop testing shows the failing tests on try now pass
locally, so I'm assuming I have fixed the issues...
Comment 21 Kartikaya Gupta (email:kats@mozilla.com) 2013-08-22 20:18:40 PDT
Comment on attachment 794263 [details] [diff] [review]
part 6 - update tests to use the new API

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

r=me. I'd like to see the comment below addressed but won't block this just for that. Also I would still suggest doing a try push to make sure the tests pass; our robocop tests have a tendency to work just fine locally and then blow up on tbpl.

::: mobile/android/base/tests/testAddonManager.java.in
@@ +59,5 @@
>              mActions.sendGeckoEvent("Preferences:Set", jsonPref.toString());
>  
>              // Wait for confirmation of the pref change before proceeding with the test.
> +            final String[] prefNames = { "extensions.getAddons.browseAddons" };
> +            final int ourRequestId = 0x7357;

I'm still a little unhappy with this constant. Can we move it to be a package static final int in BaseTest.java, with a comment that explains it's just some random number that shouldn't overlap with any actual request IDs from the app? We can use that const value in all the tests rather than having this number and duplicated local/class variables. If there was a good way to push this all the way into FennecNativeActions.java or RobocopAPI.java I would prefer that but I can't think of anything.
Comment 23 Nathan Froyd [:froydnj] 2013-08-23 08:13:36 PDT
I'll also file followup bugs for mfinkle's comment 10 and kats's comment 21 and address those early next week.
Comment 24 Nathan Froyd [:froydnj] 2013-08-23 08:17:49 PDT
(In reply to Nathan Froyd (:froydnj) from comment #22)
> Try run is here: https://tbpl.mozilla.org/?tree=Try&rev=a2b0edb5c306
> 
> It's mostly green, with some occasional testHistory orange.  IMHO, that
> orange looks like the revenge of bug 907624 and not problems with my patch. 
> I'll let the sheriffs decide...

Should also say that my try push was with a slightly older try, so that may have been the problem too...
Comment 26 Kartikaya Gupta (email:kats@mozilla.com) 2013-08-26 07:11:33 PDT
FYI, either this bug or bug 905759 (or the combination) appears to have reduced memory usage (resident memory 30s after startup) by 3.68 MiB. Nice!

This is the exact pushlog range over which the improvement happened:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fa56d4c9e630&tochange=610f5f410e3b
Comment 27 Kartikaya Gupta (email:kats@mozilla.com) 2013-08-26 07:11:53 PDT
(The above is based on the areweslimyet.com/mobile data)
Comment 28 Nathan Froyd [:froydnj] 2013-08-26 07:14:12 PDT
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26)
> FYI, either this bug or bug 905759 (or the combination) appears to have
> reduced memory usage (resident memory 30s after startup) by 3.68 MiB. Nice!

=/  I was thinking about backing this bug out today after investigating bug 908919.  I'm not sure whether the Talos test is no good or whether this bug did actually slow things down.
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-08-26 17:25:32 PDT
This was backed out due to bug 908919.
https://hg.mozilla.org/mozilla-central/rev/8df4a9443d38
Comment 30 Nathan Froyd [:froydnj] 2013-09-04 14:25:12 PDT
So, I did a little benchmarking on Try.  An example push from m-i, selected because it was recent and had green Android 4.0 ts:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4dceda951fba

ts has a score of 3928.79.  Now, Try with the sequence of patches from this bug:

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

ts has a score of 4366.95.  A large increase, and the reason we backed this out.  However, a small reordering of switch cases:

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

ts has a score of 3949.95 -- nearly the same as inbound.

I really don't understand this.  I thought this was something to do with code locality, hence the reason I moved the cases around.  But AFAICS, the switch is implemented as a jump table, so the ordering of the cases shouldn't really matter.

2.2 scores are all over the map: Inbound at 4249.58, these patches at 4757.16, and small tweak at 3782 (!).

I feel like the changes are trivial enough that they can be checked in unreviewed.  But I'd really like to understand *why* they causes these changes.  Any suggestions?
Comment 32 Nathan Froyd [:froydnj] 2013-09-11 11:59:27 PDT
Backed out for busting robocop tests with menus, among other things:

https://hg.mozilla.org/integration/mozilla-inbound/rev/67646b378722
Comment 33 Nathan Froyd [:froydnj] 2013-09-13 04:08:23 PDT
Created attachment 804393 [details] [diff] [review]
part 5 - send preference requests to JS through JNI and XPConnect rather than JSON

Turns out Preferences:RemoveObservers needs exactly the same datatype for its
requestId as the original observe request.  Might as well just move it over to
XPConnect, too, though it's not used anywhere but the tests.  Asking for re-review,
but the changes are straightforward.
Comment 34 Nathan Froyd [:froydnj] 2013-09-13 04:10:09 PDT
Created attachment 804394 [details] [diff] [review]
part 6 - update tests to use the new API

Updating the patch to plumb the new removeObservers all the way through.

Try shows green: https://tbpl.mozilla.org/?tree=Try&rev=c34bad9b3956
Comment 35 Lucas Rocha (:lucasr) 2013-09-13 05:39:54 PDT
I suspect this patch caused a regression in "time to throbber stop". Numbers went back to normal after these patches got backed out.

More details here: https://bugzilla.mozilla.org/show_bug.cgi?id=909380#c1
Comment 36 Nathan Froyd [:froydnj] 2013-09-13 06:07:50 PDT
(In reply to Lucas Rocha (:lucasr) from comment #35)
> I suspect this patch caused a regression in "time to throbber stop". Numbers
> went back to normal after these patches got backed out.
> 
> More details here: https://bugzilla.mozilla.org/show_bug.cgi?id=909380#c1

That seems likely, since it regressed Talos Ts.  However, the new crop of patches shows no regressions on talos relative to m-c or m-i.  So I feel pretty confident about landing these.
Comment 37 Kartikaya Gupta (email:kats@mozilla.com) 2013-09-13 08:09:37 PDT
Comment on attachment 804394 [details] [diff] [review]
part 6 - update tests to use the new API

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

r=me with indentation fixed

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +45,5 @@
>      private Method mUnregisterEventListener;
>      private Method mBroadcastEvent;
> +    private Method mPreferencesGetEvent;
> +    private Method mPreferencesObserveEvent;
> +	private Method mPreferencesRemoveObserversEvent;

indentation

@@ +291,5 @@
> +
> +	public void sendPreferencesRemoveObserversEvent(int requestId) {
> +		try {
> +			mPreferencesRemoveObserversEvent.invoke(mRobocopApi, requestId);
> +		} catch (IllegalAccessException e) {

woah crazy indentation

Note You need to log in before you can comment on or make changes to this bug.