987283 - usage app cold_load_launch regressed 300ms Mar 22

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Cost Control, defect, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

The usage app cold_load_launch time regressed by 300+ ms on March 22, 2014.  See here:

  https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=cold_load_time&app_list=usage&app=usage&gaia_rev=66da954e077f9ef3&gecko_rev=e23e8b1d8f84594f&plot=median

Regression range:

  https://github.com/mozilla-b2g/gaia/compare/35090996f7b9...66da954e077
  No gecko changes

Which pretty much isolates this to bug 960500.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Marina, can you investigate the launch time regression in costcontrol here?

Comment 2

[Jason Smith [:jsmith]]

We probably should backout bug 960500 - 300ms is a significant performance regression & bug 960500 isn't a release blocker, so there's a net loss here in the implementation of the usage app right now.

Comment 3

[Marina Rodríguez [:mai]]

Hi, 
the changes of the Bug 960500 don't affect a normal start up of the application. This patch only produces this delay when a error is found if the simcard is not detected (Currently without simcard the app doesn't work), therefore the delay only affects to show the error message when the user can not work with the app. 

This delay is produced by the use of the AirplaneMode module (it's necessary reading the settings to detect it). IMO, the time loss is compensated for the posibility of don't have to close the app when the airplane mode is disabled. This is because the app, when detects the first icccard, continue its starting automatically. This saves more time than if the user has to closed the app and relaunch it (the behaviour before the patch)

Anyway, currently I'm working on the Bug 977077 -[Cost Control] New module Sim Manager, on this patch I'm trying to reduce the cold_load_launch time produced since we added the DSDS support.

Regards

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Hmm, that does sound reasonable, but I wonder if we have accidentally introduced some delay for all paths here.  The cold_load_launch basically tests the time to load static content, so it probably does not include async work such as responding to the ICC error message.

In particular, it looks like the airplane mode helper immediately grabs a settings lock when its first loaded, so it may be quite expensive.

Marina, can you try lazy loading the airplane mode helper instead of loading it directly in the index.html?

Something like:

  LazyLoader.load(['/shared/js/airplane_mode_helper.js'], function() {
     var fakeState = null;
     if (AirplaneModeHelper.getStatus() === 'enabled') {
       fakeState = 'airplaneMode';
       var iccManager = window.navigator.mozIccManager;
       iccManager.addEventListener('iccdetected',
         function _oniccdetected() {
           iccManager.removeEventListener('iccdetected', _oniccdetected);
           waitForSIMReady(callback);
         });
     }
     console.warn('Error when trying to get the ICC ID status =' + fakeState);
     showNonReadyScreen(fakeState);
  });

And then change this in index.html:

     <script type="text/javascript" defer src="shared/js/airplane_mode_helper.js"></script>

To:

     <script defer src="/shared/js/lazy_loader.js"></script>

     <!-- Include lazy loaded files from shared/js in the app package.
     <link href="/shared/js/airplane_mode_helper.js">
     -->

Thanks!

Comment 5

[Salvador de la Puente González [:salva]]

I think it's a good idea.

Comment 6

[Marina Rodríguez [:mai]]

Hi Ben,
thanks for your feedback, I'm going to prepare a patch following your idea,

Regards.

Comment 7

[Marina Rodríguez [:mai]]

Attachment: patch 1.0

Salva, could you review the patch?

Comment 8

[Salvador de la Puente González [:salva]]

Comment on attachment 8396949 [details] [review]
patch 1.0

Despite the solution is correct, there is a more simpler solution we want to take. Anyway, this regression has shown a flaw in the AirplaneModeHelper and Marina will explain it later.

Marina, ask for my r? once the simpler solution is implemented and proved correct.

Comment 9

[Marina Rodríguez [:mai]]

Comment on attachment 8396949 [details] [review]
patch 1.0

Hi Salva, the PR is updated and tested. Could you review the patch?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8396949 [details] [review]
patch 1.0

This does seem simpler, but do we really want to load the airplane helper in all cases?  It seems its only used when we get the NoSim error.

If I'm reading this right, this patch will still introduce a 300ms delay or so for initializing the app even when we don't get the NoSim error.

Unfortunately, the pattern of moving js files from index.html to a single large LazyLoader.load() call in at initialization makes datazilla look better, but still slows things down for the user.  Its a bit of an anti-pattern I've seen in a couple apps now.  To really get the benefit of lazy loading we need to defer until the specific use case requiring the feature is hit.

Comment 11

[Marina Rodríguez [:mai]]

 Hi Ben,
I can not implement your approach because when I'm trying lazy loading the airplaneMode Helper, I discover that the method getStatus returns an invalid value. (The expected values are 'enabled' and 'disabled').

This behavior is produce by a race condition between the module initialization (asyncrhonous) and the method getStatus (synchronous). Currently, the airplanemodeHelper module does not notify when its initialization is finished.  

The only way to deal with this behaviour is checking if the returned value for this method is different than the expected values and, if this occurs, putting a 'changestate' event listener to wait for the correct status value, but this can produce more problems.

For these reasons, I've lazy loading the airplanemode, it is not the best way to optimized this load, but at the moment i think that is the best way.

I've open a new bug for ask a change in the AirplaneModeHelper module behavior. (Bug 988445 -[AirplaneModeHelper] getStatus method does not return the correct state)

Regards

Comment 12

[Salvador de la Puente González [:salva]]

Hi Ben, you're right and the original solution implemented it the way you want. Then we find the behaviour described in bug 988445 and we opted for implement this solution with less risk but suboptimal with the hope of opening the other bug. Once solved, we can open a follow up to resolve this issue. 

You were too fast, Ben, :)

WDYT?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Hmm, can we do something like this?

  function handleNewStatus(status) {
    // do stuff...
  }

  var status = AirplaneModeHelper.getStatus();
  if (status !== 'enabled' && status != 'disabled') {
    AirplaneModeHelper.addEventListener('statechange', function onStateChange() {
      AirplaneModeHelper.removeEventListener('statechange', onStateChange);
      status = AirplaneModeHelper.getStatus();
      handleNewStatus(status);
    });
    return;
  }

  handleNewStatus(status);

Its not pretty, but I think that should work.

Comment 14

[Salvador de la Puente González [:salva]]

Comment on attachment 8396949 [details] [review]
patch 1.0

Yes, of course, it was that way before. Ok. So lets do the workaround but we should fix the helper as well. Anyway, during a short period of time we will be delaying the apparition of the error screen.

Thank you Ben.

Marina, can we return to the former solution?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

I see now that you reference this solution in bug 988445.  I guess I'd rather use the solution in comment 13 until bug 988445 is fixed rather than the current PR.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Thanks!

Comment 17

[Marina Rodríguez [:mai]]

Comment on attachment 8396949 [details] [review]
patch 1.0

i've updated the pr with other approach, could you review the path?

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8396949 [details] [review]
patch 1.0

Look good!  Thanks for the investigating this.

Comment 19

[Salvador de la Puente González [:salva]]

Comment on attachment 8396949 [details] [review]
patch 1.0

Please, consider my comment and you have the r+. Wait for Travis to end the build.

Comment 20

[Marina Rodríguez [:mai]]

Master: 62d068141cc06b50e8d0540efe91759a80723464