805163 - Optimize and minimize the JS files

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P2)

Description

[Staś Małolepszy :stas]

Using an AMD implementation like RequireJS (which comes with an r.js minimizer) will also help us manage multiple JS file for both browser and node.

See http://requirejs.org/

Comment 1

[Staś Małolepszy :stas]

A lot of feedback at Front Trends revolved around the server side and node bindings.  Taking this bug and upping the priority.

Comment 2

[Staś Małolepszy :stas]

Mozilla's own DryIce looks like something we could use to build L20n for the browser, for node and, in the future, for Firefox as a JSM.

https://github.com/mozilla/dryice

See https://github.com/mozilla/source-map/ for an example usage.

Comment 3

[Staś Małolepszy :stas]

Attachment: Proposed patch (a lot of renames)

(Rather than reviewing the patch, it might be easier to browse the tree of the branch at https://github.com/stasm/l20n.js/tree/805163-build-system;  GH's diff is available at https://github.com/stasm/l20n.js/compare/805163-build-system.)

With this patch, L20n now uses AMD modules.

 - modules are defined with a Simplified CommonJS Wrapper,
 - on the server side, amdefine is used to provide the 'define' function,
 - on the client side, dryice modifies the source files such that modules are not anonymous and have proper dependecies,
 - furthermore, on the clientside, 'define' and 'require' are provided by https://github.com/jrburke/almond (1KB minified)
 - if not using the built version, you must use an AMD loader, e.g. require.js.

4 use cases are supported:


Client side production
----------------------

'make build' produces dist/l20n.js and dist/l20n.min.js which can be included in a script tag and are synchronous:  you can access window.L20n right away.


Server side production
----------------------

'npm install' will make 'l20n' available as a module; we need to land bug 866723 to make this work fully.


Client side development
-----------------------

There are a few ways to make this use case work.

The simplest one is when you're just working with the context and friends:

    <script src="require.js"></script>
    <script>
      require(['../js/lib/l20n'], function(lib) {
        // note that window.L20n is undefined
        var ctx = lib.L20n.getContext();
      });
    </script>    


If you want to get the bindings as well, require them directly.

    <script src="require.js"></script>
    <script>
      require(['../js/lib/l20n/bindings/html'])
    </script>

or

    <script src="require.js"></script>
    <script>
      require(['../js/lib/l20n/bindings/html'], function(bindings) {
        // the bindings act as a proxy for the L20n singleton
        var ctx = bindings.L20n.getContext();
        // and you can require other modules, too
        var Parser = require('../js/lib/l20n/parser').Parser;
      });
    </script>

or

    <script src="require.js"></script>
    <script>
      require({ baseUrl: '../js/lib/' }, ['l20n/bindings/html'], function(bindings) {
        // thanks to baseUrl, you can now require things from inside of the callback using module names
        var Parser = require('l20n/parser').Parser;
      });
    </script>


Server-side development
-----------------------

This will be very useful for running tests.  In node, you can simply require('./lib/l20n'); or any other module using relative paths.        


Summary: we end up with a modular code and a simple to use build system.  With these changes, it will be easy to have a different IO library for the client and for the server.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 743614 [details] [diff] [review]
Proposed patch (a lot of renames)

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

it looks good. I'll review performance impact and give you r+ tomorrow.

Comment 5

[Staś Małolepszy :stas]

Attachment: Parch v2 (removed one last reference to the global L20n obj)

When testing in the Gaia fork, I found one more reference to the global L20n object in the bindings which this updated version of the patch now removes.

Keep in mind that you will need the patch from bug 867055 applied to lib/l20n/context.js in order to make things work.

Depending on the manifest that you use for testing, you might also want to watch out for bug 867541.

I profiled the dist version of l20n.js built with this patch and there literally no difference in the profile to be seen compared to the minified version from the build-l20n target.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

It seems like there is a minor performance cost to this change.

reference (l10n.js+9 locales): 1083ms
reference+retranslation: 1239ms
l20n+manifest: 1106ms
l20n-manifest: 1066ms
l20n-manifest+buildsystem: 1135ms

Comment 7

[Staś Małolepszy :stas]

The define() calls just store the exports objects so this isn't much different than storing things on the global L20n object like we used to do.  The require() calls then retrieve those exports.

I'm going to guess that any overhead is created by Almond [1].  Its goal is to be a minimal implementation of the AMD API.  It supports various path mingling techniques when resolving module names.  Perhaps more importantly, it supports all different signatures of define() and require(), like:

  define(fn)
  define(deps, fn)
  define(modeulName, deps, fn)
  define(configObj, moduleName, deps, fn)

  require(moduleName)
  require(moduleName, fn)

This is handy but we could go for something simpler than almond, like dryice's mini_require [2].  We'd need some small changes to make it normalize paths correctly, but the overall footprint should be smaller.  Because we know exactly which signatures we'll be using using, we can optimize for them.

I'll prepare a branch for testing.

[1] https://github.com/jrburke/almond/blob/master/almond.js
[2] https://github.com/mozilla/dryice/blob/master/lib/dryice/mini_require.js

Comment 8

[Staś Małolepszy :stas]

Attachment: Patch with minirequire (for testing, don't land)

Here's a version of the patch which uses a modified minirequire (4KB) instead of almond (14kB).

This is just for testing.  If there are some perf gains, I suggest we have two build targets:  a more robust one, with almond, and an optimized one, with minirequire.  I can work on that later this week.

The code is also at https://github.com/stasm/l20n.js/tree/805163-build-system-minirequire.

For convenience, I included the patch from bug 867055.  Otherwise it doesn't work.

Gandalf, can you repeat the testing using this patch?

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

yeah, I'll repeat the testing. But I'm also wondering if we should just bite the bullet and take it with the cost and look for wins elsewhere.

I'll look into the cost distribution today - for example, minification shaves ~40-50ms so it could pay for the almond.

I don't want even more targets...

Comment 10

[Staś Małolepszy :stas]

Attachment 744068 [details] [diff] landed in https://github.com/l20n/l20n.js/commit/6e666f9158ec998651d68026d33a7e20a88c3553 with an r+ from gandalf on IRC.

Closing the bug.  Gandalf, let me know what the results of testing is and I'll file a new bug to use minirequire if there's a significant speed improvement.

\o/

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 744575 [details] [diff] [review]
Patch with minirequire (for testing, don't land)

Canceling the feedback request here.  I filed bug 868093 about using minirequire.