998799 - alameda.js depends toString()-ing functions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Bobby Holley (:bholley)]

Bug 990353 got backed out for Gaia test-integration bustage (see bug 990353 comment 62). I finally managed to reproduce this in a debugger, and get the bottom-most stack frame. It points here:

http://mxr.mozilla.org/gaia/source/apps/settings/js/vendor/alameda.js#1170

I couldn't easily get the rest of the stack, because the marionette rigmarole seems to hide gecko stderr somewhere I can't find it, so DumpJSStack() is useless.

Anyway, if we want to save those 4 MiB on tarako, we need to fix this pronto. Fabrice, who can own this?

Comment 1

[[:fabrice] Fabrice Desré]

James, this is your baby right?

Comment 2

[James Burke [:jrburke]]

Alameda uses toString if the modules have not been built by the optimizer to extract the dependencies and inserted as a dependency array.

This happens by default if optimize is used (used for PRODUCTION=1/GAIA_OPTIMIZE=1) in the email app, but I am unsure about:

1) how the tests are run
2) what other apps in gaia use for the build steps

If apps are not run with GAIA_OPTIMIZE=1 and they do not use that as an optimizer trigger to run uglify, then `skipDirOptimize: false` can be used in the r.js build config, and those dependencies are extracted as part of the optimizer run.

In order to prevent issues where GAIA_OPTIMIZE=1 may not be set/and apps not using the light uglify like email does, and the sources are removed by the engine, I'll just do a general pull request against apps that are using r.js to just pass skipDirOptimize: false for all the build configs using r.js.

Comment 3

[James Burke [:jrburke]]

Attachment: GitHub pull request

Makes sure to normalize define() calls even with an r.js uglify-style minification (which auto-normalizes) is not done. Should help test scenarios for Tarako, where Function toString may not be available (bug 990353) -- important for Tarako to save on some memory.

Asking for review from owners of the apps that are touched by this change.

For reviewers, this change just does one of the steps that was automatically done by r.js whenever the r.js build config had a value other than optimize: 'none', so this sort of change has already been tested if doing those minify-optimized builds. This change just does that work even if optimize is set to 'none'.

Since the travis build passes, that should be enough of an indication that this change is safe, as it just affects how dependencies are passed to the define() calls. So, if the module did this in source form:

define(function(require) {
  var a = require('a');
});

This setting uses an AST to find those require calls, then transform the define call to this form, for the modules placed in build_stage:

define(['require', 'a'], function(require) {
  var a = require('a');
});

GENERAL NOTE: this change only affects builds that use the build_stage area. So, DEBUG=1 builds that are pointing to the source area for the app are not affected by this change, and it is assumed those cases are run on platforms that have Function toString() that returns a semblance of the function source. If that is not true, that would be good to know.

Comment 4

[James Burke [:jrburke]]

Comment on attachment 8409493 [details] [review]
GitHub pull request

Adding :mcav for clock file review

Comment 5

[James Burke [:jrburke]]

Comment on attachment 8409493 [details] [review]
GitHub pull request

Adding :evelyn for settings file review

Comment 6

[James Burke [:jrburke]]

Comment on attachment 8409493 [details] [review]
GitHub pull request

Adding :djf for camera file review.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8409493 [details] [review]
GitHub pull request

(Aside: I think you can comma-delimit reviewers so you can ask for review from multiple people in a single shot.)

Comment 8

[Bobby Holley (:bholley)]

Bug 990353 is green with this PR. Will this go to Tarako automatically?

Comment 9

[James Burke [:jrburke]]

Not automatically: this will land it on master, then if 1.3+ approval is given, it can be applied to that branch. Asking for 1.3T? though to start the 1.3T approval process while waiting for final review signoffs.

For the people deciding 1.3T? -- this change is needed to improve the memory footprint on Tarako. It is a very minor build change, usually run when full optimizations are done, but for tests to pass for the toString platform change, need that work to happen for some of the test scenarios too.

In general, looking more into the tests, it does seem that some are run with DEBUG=1 which this changeset will not fix, but it is also not clear if those tests are run in a gecko build that has toString neutered. For the integration tests, it uses build_stage, so should fix the errors mentioned in the original test failure log that triggered this bug. 

However, if that is not enough and other errors are generated from a DEBUG=1 test scenario, I have prepped an alternate alameda that will do a slower xhr pathway to find deps then proceed with normal loading to allow those test cases to work. I would prefer to not need to push that version though unless it is actually needed since it is a bigger change. It is just a new version of alameda, no other app changes, but since it affects loading in general, I am more cautious about applying it.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8409493 [details] [review]
GitHub pull request

Passing my review on to Diego. I don't actually know anything about how alameda is being used by Camera.

Comment 11

[Diego Marcos [:dmarcos]]

This looks good to me

Comment 12

[Keven Kuo [:kkuo]]

Hi! James,

Check in needed. Thanks

--
Keven

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Keven Kuo [:kkuo] from comment #12)
> Hi! James,
> 
> Check in needed. Thanks

No, there's one missing review.

Comment 14

[James Zhang (Spreadtrum)]

(In reply to Fabrice Desré [:fabrice] from comment #13)
> (In reply to Keven Kuo [:kkuo] from comment #12)
> > Hi! James,
> > 
> > Check in needed. Thanks
> 
> No, there's one missing review.

How many memory can we save with this patch?

Comment 15

[James Zhang (Spreadtrum)]

OK, all review+ now, please land it, thanks.

Comment 16

[Evelyn Hung]

Settings app didn't use Alameda.js in 1.3/1.3t branch, you need to exclude Settings for uplift.

Comment 17

[[:fabrice] Fabrice Desré]

(In reply to James Zhang from comment #15)
> OK, all review+ now, please land it, thanks.

James, someone on your side is doing gaia uplifts, right? Or should we take over?
See the list of bugs waiting at https://bugzilla.mozilla.org/buglist.cgi?f1=cf_blocking_b2g&o3=notequals&v3=verified&o1=equals&resolution=FIXED&o2=notequals&query_format=advanced&f3=cf_status_b2g_1_3t&f2=cf_status_b2g_1_3t&v1=1.3T%2B&v2=fixed&list_id=9923926

Comment 18

[[:fabrice] Fabrice Desré]

Merged on master:
https://github.com/mozilla-b2g/gaia/commit/3d88098fdbf2fb12b080169a0134aeb8dc47179c

Comment 19

[James Burke [:jrburke]]

Merged to v1.3t:
https://github.com/mozilla-b2g/gaia/commit/73f1cf3f5ba6e3cd0b670bb66fafbc31b133f402

Only clock and email on that branch use alameda, so the above commit is just for those apps.

See comment #9 if issues persist: if it turns out some of the tests run with DEBUG=1 and run in a gecko without toString(), then we may need one other change that I can prep farily

Comment 20

[Bobby Holley (:bholley)]

Thanks for jumping on this and getting it done James!