Page MenuHomePhabricator

MFA: mobile.languages.structured is bundled in webpack (code splitting warning!!)
Closed, ResolvedPublic5 Story Points

Description

Code relating to the toggle code currently lives in
mobile.languages.structured and we will move it to webpack. It is currently lazy loaded meaning we are in uncharted territory for the migration.

It is loaded here:
https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/09534b7b847ca9e60a0e1de3d67434ec2974198d/resources/skins.minerva.scripts/init.js

Acceptance criteria

  • a conversation is had about whether we want to continue to code split by feature. Alternative approaches to consider - critical and non-critical js; load all code on startup. The conversation does not need to block implementation. At minimum we need to ensure that we do not increase the critical JS size
  • code for feature is loaded in webpack (added topic to super happy dev time)
  • tests are ported to node-qunit

💩 Minerva is retained in a mergeable state throughout the migration. No need to worry about cached html but an alias module (empty module with mobile.startup dependency) will be needed until references in Minerva have been updated. JR: Wasn't applicable here.

  • Critical JS has not increased (22B increase is tolerable)
  • The bytes size of the language module has not changed considerably (e.g. we haven't loaded mobile.startup code twice)

Code:

Sign off steps

  • Progress is updated.
  • Unless I'm mistaken the port to webpack has shaved 2kb off the language overlay (post-gzip) without any changes to functionality. It would be interesting to understand how this came to be and record it for prosperity!
  • Review which features to port next and create cards. (T210209 is next.)

Event Timeline

Restricted Application added a subscriber: Gilles. · View Herald TranscriptNov 22 2018, 10:46 PM
Jdlrobson triaged this task as Normal priority.Nov 22 2018, 10:51 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.
Gilles removed a subscriber: Gilles.Nov 23 2018, 8:53 AM
Jdlrobson renamed this task from mobile.languages.structured is bundled in webpack to MFA: mobile.languages.structured is bundled in webpack.Nov 23 2018, 11:49 PM
Jdlrobson renamed this task from MFA: mobile.languages.structured is bundled in webpack to MFA: mobile.languages.structured is bundled in webpack (code splitting warning!!).Nov 26 2018, 10:32 PM
ovasileva set the point value for this task to 5.Nov 27 2018, 5:28 PM
Jdlrobson updated the task description. (Show Details)Nov 27 2018, 5:29 PM
Jdlrobson raised the priority of this task from Normal to High.Dec 14 2018, 1:28 AM
Jdlrobson updated the task description. (Show Details)Dec 18 2018, 12:45 AM

We talked about this today:

  • Webpack runtime bundle doesn't seem to be for splitting sharing code
  • Webpack does seem to support de-duplicating JS via SplitChunksPlugin
  • Many ways to do this seem to exist:
    • could bypass RL and access JS directly
    • We could use webpack async loader
    • ... but beware referencing urls because cache problems (and that will get the attention of the performance team)

We're all lacking in experience in this area so we will probably need to talk again about this when better informed, but the minimum we need to do is make sure
that we don't ship code e.g. Overlay twice (inside mobile.startup and mobile.languages) and maintain the same level of bytes. Have added a bullet point to capture that.

nray added a comment.Dec 18 2018, 1:14 AM

We talked about this today:

  • Webpack runtime bundle doesn't seem to be for splitting sharing code
  • Webpack does seem to support de-duplicating JS via SplitChunksPlugin
  • Many ways to do this seem to exist:
    • could bypass RL and access JS directly
    • We could use webpack async loader
    • ... but beware referencing urls because cache problems (and that will get the attention of the performance team)

We're all lacking in experience in this area so we will probably need to talk again about this when better informed, but the minimum we need to do is make sure
that we don't ship code e.g. Overlay twice (inside mobile.startup and mobile.languages) and maintain the same level of bytes. Have added a bullet point to capture that.

thanks for adding this @Jdlrobson !

Change 481095 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] WIP: Bundle mobile.languages.structured with webpack

https://gerrit.wikimedia.org/r/481095

Change 481211 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Move rtlLanguages in separate file

https://gerrit.wikimedia.org/r/481211

Change 482090 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Make languages's util.js mirror style of mobile.startup/util.js

https://gerrit.wikimedia.org/r/482090

Jdlrobson raised the priority of this task from High to Needs Triage.Jan 3 2019, 10:38 PM

Change 481095 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Bundle mobile.languages.structured with webpack

https://gerrit.wikimedia.org/r/481095

Change 482247 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Document new bundle sizes of mobile.startup components

https://gerrit.wikimedia.org/r/482247

Jdlrobson updated the task description. (Show Details)Jan 4 2019, 12:41 AM

Change 482090 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Make languages's util.js mirror style of mobile.startup/util.js

https://gerrit.wikimedia.org/r/482090

Jdlrobson updated the task description. (Show Details)Jan 4 2019, 1:00 AM
Jdlrobson triaged this task as High priority.Jan 4 2019, 1:00 AM

The bytes size of the language module has not changed considerably (e.g. we haven't loaded mobile.startup code twice)

Interestingly, a request to /w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj is 2kb less after your change, while the mobile.startup module doesn't seem to gain any size (a good thing!!).
I'm not sure what's causing this drop (maybe just uglify?!) but definitely worth exploring some more in sign off. I've added a sign off step.

I've submitted a couple of follow ups, only https://gerrit.wikimedia.org/r/482090 should be considered necessary to move this task to sign off!

Change 481211 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move rtlLanguages in separate file

https://gerrit.wikimedia.org/r/481211

Jdlrobson removed nray as the assignee of this task.Jan 4 2019, 1:15 AM
Jdlrobson added a subscriber: nray.

Change 482247 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Document new bundle sizes of mobile.startup components

https://gerrit.wikimedia.org/r/482247

Change 482348 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove unnecessary test dependency

https://gerrit.wikimedia.org/r/482348

Change 482348 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Clarify test dependency

https://gerrit.wikimedia.org/r/482348

Unless I'm mistaken the port to webpack has shaved 2kb off the language overlay (post-gzip) without any changes to functionality. It would be interesting to understand how this came to be and record it for prosperity!

I cannot repro. mobile.common.js is now part of mobile.startup so the comparison of before and after must be the sum of mobile.startup and its dependencies, mediawiki.template.hogan, and mobile.languages.structured:

curl -i -H 'Accept-Encoding: gzip' "$URL"|
wc -c

Before (41c903db3)
http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva&version=0x5qnuq
67100

http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj
3041

67100 + 3041 = 70141

---

After (c5a8dfd76)
http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva&version=177e0eb
67122

http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj
2777

67122 + 2777 = 69899

For users visiting the language overlay, it's a 242B improvement. For all other users, it's a 22B detriment.

Niedzielski updated the task description. (Show Details)
Niedzielski closed this task as Resolved.Jan 7 2019, 8:30 PM
Niedzielski updated the task description. (Show Details)
Jdlrobson reopened this task as Open.EditedJan 7 2019, 9:06 PM

@Niedzielski can you try the following? I'm still seeing a big discrepancy in bytes. My version numbers don't seem to map to yours either..

git checkout c5a8dfd769ede9b3484a168a57058583ce1d092f
curl -i -H 'Accept-Encoding: gzip' 'http://localhost:8888/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=0hym8m0' > 1.js

7703

git checkout 41c903db34c2c234b5934196949231323865087a
curl -i H 'Accept-Encoding: gzip' 'http://localhost:8888/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj' > 2.js

9746

Hm... version=1t802mj in the after case was a mistake. I've rerun with the following results:

# before
git checkout 41c903db34c2c234b5934196949231323865087a

# decompressed startup
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva&version=0x5qnuq' --compressed |wc -c
233374

# compressed startup
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva&version=0x5qnuq' |wc -c
67100

# decompressed languages
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj' --compressed |wc -c
10125

# compressed languages
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj' |wc -c
3041

# after
git checkout c5a8dfd769ede9b3484a168a57058583ce1d092f

# ---

# decompressed startup
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva&version=177e0eb' --compressed|wc -c
233455

# compressed startup
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva&version=177e0eb' |wc -c
67122

# decompressed languages
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=0hym8m0' --compressed |wc -c
8082

# compressed languages
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=0hym8m0' |wc -c
2785

curl --version
curl 7.52.1 (x86_64-pc-linux-gnu) libcurl/7.52.1 OpenSSL/1.0.2q zlib/1.2.8 libidn2/0.16 libpsl/0.17.0 (+libidn2/0.16) libssh2/1.7.0 nghttp2/1.18.1 librtmp/2.3
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL

I'm not sure what the version parameter does so I've omitted it and run the above again:

# before
git checkout 41c903db34c2c234b5934196949231323865087a

# decompressed startup
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva' --compressed |wc -c
233366

# compressed startup
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva' |wc -c
67092

# decompressed languages
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva' --compressed |wc -c
10117

# compressed languages
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva' |wc -c
3033

# after
git checkout c5a8dfd769ede9b3484a168a57058583ce1d092f

# ---

# decompressed startup
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva' --compressed|wc -c
233447

# compressed startup
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=jquery%7Cmediawiki.template.hogan%7Cmobile.startup&skin=minerva' |wc -c
67114

# decompressed languages
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva' --compressed |wc -c
8074

# compressed languages
curl -siH 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva' |wc -c
2777

The results are nearly the same but quite different from yours.

curl -i H 'Accept-Encoding: gzip' 'http://localhost:8888/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj' > 2.js

Please double check you added a hyphen to the -H param!

I'm not sure what the version parameter does so I've omitted it and run the above again:

The version is a hash generated from the contents of the module for caching (it's how RL only has the 5 min cache update time).. so clearly we're generating slightly different modules. I think there might be a problem with my gzip.
@nray can you run the same tests so we get a 3rd data point here to confirm that's the case?

@Jdlrobson, please double check your -H parameter. I get very different results since one is checking gzipped size and one isn't:

boxwiki███ curl -i H 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj'|wc -c
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: H
curl: (3) Illegal port number
100  9746    0  9746    0     0   3786      0 --:--:--  0:00:02 --:--:--  7124
10107
boxwiki███ curl -i -H 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj'|wc -c
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2662  100  2662    0     0   3328      0 --:--:-- --:--:-- --:--:--  3327
3041
nray added a comment.Jan 8 2019, 12:43 AM

git checkout c5a8dfd769ede9b3484a168a57058583ce1d092f
curl -i -H 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=0hym8m0' | wc -c
2785 bytes

git checkout 41c903db34c2c234b5934196949231323865087a
curl -i -H 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=1t802mj' | wc -c
3041 bytes

3041 - 2785 = A reduction of 256 bytes after changes

Also, I confirmed that the browser is requesting the same version query params shown above

Jdlrobson closed this task as Resolved.Jan 8 2019, 12:49 AM

Thanks for checking. Must be an issue my side. Will let you know if I work it out! Looking closely it seems CURL is using Python and I've been having a few Python issues recently...