841353 - 'defer' more scripts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

In many apps more scripts can be defered in order to take advantage of the html5 parser.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

I forgot some debugging statements...

Comment 2

[Etienne Segonzac (:etienne)]

Can you rebase your patch? I'd like to do a bit a testing and it's not applying properly.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v2

Less forgotten things.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 713940 [details] [diff] [review]
Patch v2

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

r=me with the changes to the fm app removed (since they break the app ;) )

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/88568d9f69edf0fcb7c59613f961b8804f1eaa88

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What's the expected perf win for this?  Which apps should be improved?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> What's the expected perf win for this?  Which apps should be improved?

The win depends on the app. If you look at datazilla you will see an improvement for gallery/music/sms between https://github.com/mozilla-b2g/gaia/commit/2e4d862b5592e1a3 and https://github.com/mozilla-b2g/gaia/commit/a09de87ea2be9cc0 which I would associate to this PR. The theory I have is that instead of blocking on the main thread for reading scripts and then parse the html, the parser parse the html on it's own thread as soon as possible and scripts are parsed on the main thread in parralel.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Followup

I add an additional space somewhere and the browser is broken in production mode now :(

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/83ea6d7a59071369376be9ef9b4b397a378dc18a

Comment 11

[Alex Keybl [:akeybl]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #10)
> https://github.com/mozilla-b2g/gaia/commit/
> 83ea6d7a59071369376be9ef9b4b397a378dc18a

Not blocking, but we will approve this change for v1-train and v1.0.1.

Comment 12

[Jason Duell]

Henri wrote the HTML 5 parser, so I'm wondering if comment 8 sound right to him...

Comment 13

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-20)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> The theory I have is that instead of blocking on the
> main thread for reading scripts and then parse the html, the parser parse
> the html on it's own thread as soon as possible and scripts are parsed on
> the main thread in parralel.

The parser thread keeps parsing on its own thread in any case. Normal <script> doesn't block the parser thread. It blocks DOM construction on the main thread. Meanwhile, the parser thread keeps parsing and generating a sequence of operations that need to be performed for DOM construction later. The sequence of operations can be used unless the script does an unbalanced document.write() (unbalanced meaning that it opens a different number of elements than it closes).

Defer runs scripts after DOM construction. The JS source is parsed at that point, so there isn't HTML parsing and JS parsing parallelism is the defer case. The main benefit compared to putting a normal <script> at the bottom of the HTML source is that the network fetch can start sooner.

Note that there's also the option to run scripts as soon as they are available, but that's the async attribute rather than the defer attribute.

Comment 14

[David Scravaglieri [:scravag]]

Ask john for for uplift

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking tef+ since that is what's needed at this stage to get uplift to v1-train *and* v1.0.1

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

Commit 83ea6d7a59071369376be9ef9b4b397a378dc18a does not apply to v1-train or v1.0.1.
This means that there are merge conflicts which need to be resolved.
If there are dependencies that are not
approved for branch landing, or have yet to land on master,
please let me know

If a manual merge is required, a good place to start might be:
  cd gaia
  git checkout v1-train
  git cherry-pick -x -m1 83ea6d7a59071369376be9ef9b4b397a378dc18a
  <RESOLVE MERGE CONFLICTS>
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 17

[Julien Wajsberg [:julienw]]

Bug 837267 is producing this conflict.

While it should be easy to merge, I'd rather uplift that low-risk bug too and I accordingly asked an approval there.

Comment 18

[Julien Wajsberg [:julienw]]

If Bug 837267 is not approved on v1.0.1 then we could easily resolve the conflict.

Comment 19

[Andrew Overholt [:overholt]]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> If Bug 837267 is not approved on v1.0.1 then we could easily resolve the
> conflict.

Okay, please do this as uplifting bug 837267 was deemed unnecessary risk during the triage meeting today (the day of 1.0.1 handoff).

Comment 20

[Julien Wajsberg [:julienw]]

see https://github.com/julienw/gaia/tree/841353-resolve-conflict for v1.0.1

for v1-train I believe bug 837267 will ultimately land so we can wait for that.

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

v1.0.1: bc34b39420a3d16b50fe70ed4d3641dc6ebfea09

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

Per comment 20, which refers to something which did land on v1-train, setting status-b2g18 to wontfix

Comment 23

[Julien Wajsberg [:julienw]]

Sorry if that wasn't clear.

I wanted to mean that when Bug 837267 will be landed to v1-train, this one could land to v1-train without conflict.

I think this happened now so you can proceed, thanks a lot John !

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Still having a merge conflict on v1-train with:

  git checkout v1-train
  git cherry-pick -x -m1 83ea6d7a59071369376be9ef9b4b397a378dc18a

#	both modified:      apps/browser/index.html

      <!-- Shared code -->
++<<<<<<< HEAD
 +    <script type="application/javascript" src="shared/js/l10n.js"></script>
 +    <script type="application/javascript" defer src="shared/js/gesture_detector.js"></script>
++=======
+     <script defer src="shared/js/l10n.js"></script>
+     <script defer src="shared/js/gesture_detector.js"></script>
+
+     <!-- Specific code -->
+     <script defer src="js/places.js"></script>
+     <script defer src="js/date_helper.js"></script>
+     <script defer src="js/modal_dialog.js"></script>
+     <script defer src="js/authentication_dialog.js"></script>
+     <script defer src="js/browser.js"></script>
+     <script defer src="js/browser_extensions.js"></script>
++>>>>>>> 83ea6d7... Merge pull request #8132 from vingtetun/841353-followup

      <!-- Resources -->

Comment 25

[Julien Wajsberg [:julienw]]

John, there are 2 commits here :

https://github.com/mozilla-b2g/gaia/commit/88568d9f69edf0fcb7c59613f961b8804f1eaa88
https://github.com/mozilla-b2g/gaia/commit/83ea6d7a59071369376be9ef9b4b397a378dc18a

I think you only tried to cherry-pick the second one.

Comment 26

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Julien Wajsberg [:julienw] from comment #25)
> John, there are 2 commits here :
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 88568d9f69edf0fcb7c59613f961b8804f1eaa88
> https://github.com/mozilla-b2g/gaia/commit/
> 83ea6d7a59071369376be9ef9b4b397a378dc18a
> 
> I think you only tried to cherry-pick the second one.

you are correct, I did only try the second one.

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

Squashed the commits:

v1-train: a3e3407f867980178a31262a6893fbf2ee22f636

Comment 28

[Julien Wajsberg [:julienw]]

Just saw that I messed up in comment 20 : I only used the second commit.

Just tried to cherry pick the first commit too (88568d9f69edf0fcb7c59613f961b8804f1eaa88) and there are conflicts. I try to sort this out.

Comment 29

[Julien Wajsberg [:julienw]]

v1.0.1: c342bb232e46bb88714ead96e2401f5815f3ea67

sorry for the mess.

Comment 30

[amiller]

does not make sense to create a regression issue.

Comment 31

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 32

[Julien Wajsberg [:julienw]]

Nothing much to verify, except that the various affected apps still work.

Maybe check that they load in less than 1 sec.

Affected apps are :

browser
calendar
camera
clock
communications (dialer, contacts, ftu, facebook import)
fm
gallery
music
sms
video