Merge lp:~sidnei/lazr-js/better-module-config-compression into lp:lazr-js

[Sidnei da Silva]

- If a string literal is used more than once in requires/use/after,
  declare a constant inside a closure so that minification can
  compress the module config better.

[Graham Binns (gmb)]

I'm requesting Maris's review on this because I don't have the necessary knowledge to review it.

review: Abstain

[Māris Fogels (mars)]

Hi Sidnei,

This change's functionality looks good. I asked for a few more source code comments in key areas, and had one suggestion about the testing. With the addition source comments, I think you can land it.

I think the code for counting and generating the string literal optimizations is very close to needing a refactoring. If the optimizer was a collection of methods or functions then it would be a lot easier to test their behaviour in isolation. The testing appears to be based on the inputs and outputs of a large piece of sample data because all of the functionality is in a linear script right now, and this makes the tests very difficult to read (and therefore, difficult for someone else to debug). I do not think that this testing approach can be taken much farther beyond what you have now.

Maris

=== modified file 'src-py/lazr/js/meta.py'
--- src-py/lazr/js/meta.py 2010-01-27 21:20:17 +0000
+++ src-py/lazr/js/meta.py 2010-03-16 04:42:14 +0000

@@ -101,17 +105,48 @@
                 entry["ext"] = self.ext
                 if self.include_skin and entry.get("skinnable"):
                     self.generate_skin_modules(entry, metadata, root)
- else:
- entry['skinnable'] = False
             metadata.extend(meta)

         modules = {}
+ all_literals = []
         for meta in metadata:
             name = meta.pop("name")
             modules[name] = meta
-
- open(out, "w").write("var %s = %s;" %
- (var_name, simplejson.dumps(modules)))
+ all_literals.append(name)
+ all_literals.extend(meta.get("use", ()))
+ all_literals.extend(meta.get("requires", ()))
+ all_literals.extend(meta.get("after", ()))
+
+ # Only optimize string literals if they are used more than
+ # twice, since otherwise the optimization is pointless.
+ literals = [k for k, g in itertools.groupby(sorted(all_literals))
+ if len(list(g)) > 1]
+ literals.extend(["css", "js"])

So I assume the loop is an easy way to count the literal occurances. Why do
you manually add the css and js literals? Please add a comment explaining
this section.

+
+ # For each string literal we are interested in, generate a
+ # variable declaration for the string literal, to improve
+ # minification.
+ variables_decl = "var %s" % ",\n ".join(
+ ["%s = \"%s\"" % (NAME_RE.sub("_", literal).upper(), literal)
+ for literal in sorted(literals)])

So this creates a string like "var FOO='foo',BAR='bar'..."?
It looks like you are correcting the literal names? Please add a comment
demonstrating what this code outputs.

+
+ def literal_sub(match):
+ if match.group(2) in literals:
+ return (match.group(1) +
+ NAME_RE.sub("_", match.group(2)).upper() +
+ match.group(3))
+ return match.group(0)

I am not sure what this does. Why are you replacing underscores here? Was that
not done in the last code block? Please add a comment explaining this section.

+
+ modules_decl = LITERAL_RE.sub(l...

review: Approve

[lp:~sidnei/lazr-js/better-module-config-compression]

175. By Sidnei da Silva on 2010-03-16

Merged newer-buildout into better-module-config-compression.

176. By Sidnei da Silva on 2010-03-16

- Add some more comments as recommended by Maris, simplify part of the code.

=== modified file 'src-py/lazr/js/meta.py'
--- src-py/lazr/js/meta.py	2010-01-27 21:20:17 +0000
+++ src-py/lazr/js/meta.py	2010-03-16 19:10:44 +0000
@@ -2,6 +2,7 @@
 import re
 import sys
 import glob
+import itertools
 import optparse
 
 import simplejson
@@ -15,6 +16,9 @@
     "({[^{}a-zA-Z]*(use|requires|optional|after|"
     "supersedes|omit|skinnable)[\s\'\":]*[^{}]*})\s*\);", re.M | re.S)
 
+LITERAL_RE = re.compile("([\[ ]+)\"([\w\.\+-]+)\"([^:])")
+NAME_RE = re.compile("[\.\+-]")
+
 
 def extract_metadata(src):
     """Extract metadata about an YUI module, given it's source."""
@@ -101,17 +105,67 @@
                 entry["ext"] = self.ext
                 if self.include_skin and entry.get("skinnable"):
                     self.generate_skin_modules(entry, metadata, root)
-                else:
-                    entry['skinnable'] = False
             metadata.extend(meta)
 
         modules = {}
+        all_literals = []
         for meta in metadata:
             name = meta.pop("name")
             modules[name] = meta
-
-        open(out, "w").write("var %s = %s;" %
-                             (var_name, simplejson.dumps(modules)))
+            all_literals.append(name)
+            all_literals.extend(meta.get("use", ()))
+            all_literals.extend(meta.get("requires", ()))
+            all_literals.extend(meta.get("after", ()))
+            all_literals.append(meta["type"])
+
+        # Only optimize string literals if they are used more than
+        # once, since otherwise the optimization is pointless. This
+        # loop here is basically filtering out those that only occur
+        # once.
+        literals = [k for k, g in itertools.groupby(sorted(all_literals))
+                    if len(list(g)) > 1]
+
+        # For each string literal we are interested in, generate a
+        # variable declaration for the string literal, to improve
+        # minification.
+        #
+        # The variable name is generated by replacing [".", "+", "-"]
+        # with an underscore and then make that the variable name,
+        # uppercase.
+        #
+        # We'll save a mapping of literal -> variable name here for
+        # reuse below on the re.sub() helper function.
+        literals_map = dict([(literal, NAME_RE.sub("_", literal).upper())
+                             for literal in literals])
+        variables_decl = "var %s" % ",\n  ".join(
+            ["%s = \"%s\"" % (variable, literal)
+             for literal, variable in sorted(literals_map.iteritems())])
+
+        # This re.sub() helper function operates on the JSON-ified
+        # representation of the modules, by looking for string
+        # literals that occur over the JSON structure but *not* as
+        # attribute names.
+        #
+        # If a string literal is found that matches the list of
+        # literals we have declared as variables above, then replace
+        # the it by the equivalent variable, otherwise return the
+        # original string.
+        def literal_sub(match):
+            literal = match.group(2)
+            if literal in literals_map:
+                return match.group(1) + literals_map[literal] + match.group(3)
+            return match.group(0)
+
+        modules_decl = LITERAL_RE.sub(literal_sub, simplejson.dumps(modules))
+
+        module_config = open(out, "w")
+        try:
+            module_config.write("""var %s = (function(){
+  %s;
+  return %s;
+})();""" % (var_name, variables_decl, modules_decl))
+        finally:
+            module_config.close()
 
     def generate_skin_modules(self, entry, metadata, root):
         # Generate a skin module definition, since YUI assumes that

=== modified file 'src-py/lazr/js/tests/test_meta.py'
--- src-py/lazr/js/tests/test_meta.py	2010-01-27 14:01:09 +0000
+++ src-py/lazr/js/tests/test_meta.py	2010-03-16 19:10:44 +0000
@@ -207,7 +207,7 @@
         """
         root = self.makeDir()
         anim = self.makeDir(path=os.path.join(root, "anim"))
-        sample = self.makeFile("""\
+        self.makeFile("""\
         YUI.add('lazr.anim', function(Y){
            Y.log('Hello World');
         }, "0.1", {"use": ["dom"]});
@@ -218,20 +218,15 @@
                               output=output, exclude_regex="")
         builder.do_build()
 
-        module = {}
-        config = {"lazr.anim":
-                  {"path": "anim/anim-min.js",
-                   "use": ["dom"],
-                   "ext": True,
-                   "type": "js",
-                   "skinnable": False
-                   }
-                  }
-
         got = open(output, "r").read()
         prefix = got[:18]
+        modules = got.splitlines()[-2].strip()[7:-1]
         self.assertEquals(prefix, "var LAZR_CONFIG = ")
-        self.assertEquals(simplejson.loads(got[18:-1]), config)
+        self.assertTrue('"path": "anim/anim-min.js"' in modules)
+        self.assertFalse('"skinnable": false' in modules)
+        self.assertTrue('"type": "js"' in modules)
+        self.assertTrue('"use": ["dom"]' in modules)
+        self.assertTrue('"ext": true' in modules)
 
     def test_extract_skinnable(self):
         """
@@ -242,14 +237,14 @@
         """
         root = self.makeDir()
         anim = self.makeDir(path=os.path.join(root, "anim"))
-        sample = self.makeFile("""\
+        self.makeFile("""\
         YUI.add('lazr.anim', function(Y){
            Y.log('Hello World');
         }, "0.1", {"use": ["dom"], "requires": ["widget"], "skinnable": true});
         """, basename="anim.js", dirname=anim)
 
         skin = self.makeDir(path=os.path.join(anim, "assets", "skins", "sam"))
-        css = self.makeFile("""\
+        self.makeFile("""\
         /* anim-skin.css */
         """, basename="anim-skin.css", dirname=skin)
 
@@ -259,29 +254,24 @@
                               prefix="lazr")
         builder.do_build()
 
-        config = {
-            "skin-sam-lazr.anim":
-            {"path": "lazr/anim/assets/skins/sam/anim-skin.css",
-             "type": "css",
-             "ext": True,
-             "after": ["cssreset", "cssfonts", "cssgrids",
-                       "cssreset-context", "cssfonts-context",
-                       "cssgrids-context", "skin-sam-widget"],
-             },
-            "lazr.anim":
-            {"path": "lazr/anim/anim-min.js",
-             "use": ["dom"],
-             "requires": ["widget"],
-             "type": "js",
-             "ext": True,
-             "skinnable": True,
-             }
-            }
-
         got = open(output, "r").read()
         prefix = got[:18]
+        modules = got.splitlines()[-2].strip()[7:-1]
         self.assertEquals(prefix, "var LAZR_CONFIG = ")
-        self.assertEquals(simplejson.loads(got[18:-1]), config)
+        self.assertTrue(
+            '"path": "lazr/anim/assets/skins/sam/anim-skin.css"' in modules)
+        self.assertTrue(
+            '"path": "lazr/anim/anim-min.js"' in modules)
+        self.assertTrue('"skinnable": true' in modules)
+        self.assertTrue('"type": "js"' in modules)
+        self.assertTrue('"type": "css"' in modules)
+        self.assertTrue('"use": ["dom"]' in modules)
+        self.assertTrue('"requires": ["widget"]' in modules)
+        self.assertTrue(
+            ('"after": ["cssreset", "cssfonts", "cssgrids", '
+             '"cssreset-context", "cssfonts-context", "cssgrids-context", '
+             '"skin-sam-widget"]') in modules)
+        self.assertTrue('"ext": true' in modules)
 
     def test_extract_skinnable_with_lazr_conventions(self):
         """
@@ -294,19 +284,19 @@
         """
         root = self.makeDir()
         anim = self.makeDir(path=os.path.join(root, "anim"))
-        sample = self.makeFile("""\
+        self.makeFile("""\
         YUI.add('lazr.anim', function(Y){
            Y.log('Hello World');
         }, "0.1", {"use": ["dom"], "requires": ["widget"], "skinnable": true});
         """, basename="anim.js", dirname=anim)
 
         assets = self.makeDir(path=os.path.join(anim, "assets"))
-        css_core = self.makeFile("""\
+        self.makeFile("""\
         /* purty-anim-core.css */
         """, basename="purty-anim-core.css", dirname=assets)
 
         skin = self.makeDir(path=os.path.join(assets, "skins", "sam"))
-        css_skin = self.makeFile("""\
+        self.makeFile("""\
         /* purty-anim-skin.css */
         """, basename="purty-anim-skin.css", dirname=skin)
 
@@ -316,39 +306,51 @@
                               prefix="lazr")
         builder.do_build()
 
-        config = {
-            "skin-sam-lazr.anim+core":
-            {"path": "lazr/anim/assets/purty-anim-core.css",
-             "type": "css",
-             "ext": True,
-             "after": ["cssreset", "cssfonts", "cssgrids",
-                       "cssreset-context", "cssfonts-context",
-                       "cssgrids-context", "skin-sam-widget"],
-             },
-            "skin-sam-lazr.anim":
-            {"path": "lazr/anim/assets/skins/sam/purty-anim-skin.css",
-             "type": "css",
-             "ext": True,
-             "after": ["cssreset", "cssfonts", "cssgrids",
-                       "cssreset-context", "cssfonts-context",
-                       "cssgrids-context", "skin-sam-widget",
-                       "skin-sam-lazr.anim+core"],
-             "requires": ["skin-sam-lazr.anim+core"],
-             },
-            "lazr.anim":
-            {"path": "lazr/anim/anim-min.js",
-             "use": ["dom"],
-             "requires": ["widget"],
-             "type": "js",
-             "ext": True,
-             "skinnable": True,
-             }
-            }
-
         got = open(output, "r").read()
         prefix = got[:18]
+        lines = got.splitlines()
+        modules = lines[-2].strip()[7:-1]
+        variables = {}
+        for line in lines[1:-2]:
+            variable, literal = line.split("=")
+            variable = variable.strip(" ").split(" ")[-1]
+            variables[variable] = literal.strip(" ").split("\"")[1]
         self.assertEquals(prefix, "var LAZR_CONFIG = ")
-        self.assertEquals(simplejson.loads(got[18:-1]), config)
+
+        # Variables are only defined for optimizing minification if
+        # they are used more than once, see how "dom" and "js" are not
+        # included in this list, even though they are used by the
+        # module.  All the other names are used more than once due to
+        # the two CSS files that are part of the skin for this module.
+        self.assertEquals(variables, {
+            "CSS": "css",
+            "CSSFONTS": "cssfonts",
+            "CSSFONTS_CONTEXT": "cssfonts-context",
+            "CSSGRIDS": "cssgrids",
+            "CSSGRIDS_CONTEXT": "cssgrids-context",
+            "CSSRESET": "cssreset",
+            "CSSRESET_CONTEXT": "cssreset-context",
+            "SKIN_SAM_LAZR_ANIM_CORE": "skin-sam-lazr.anim+core",
+            "SKIN_SAM_WIDGET": "skin-sam-widget"})
+
+        self.assertTrue(
+            '"path": "lazr/anim/assets/purty-anim-core.css"' in modules)
+        self.assertTrue(
+            '"path": "lazr/anim/assets/skins/sam/purty-anim-skin.css"'
+            in modules)
+        self.assertTrue(
+            '"path": "lazr/anim/anim-min.js"' in modules)
+        self.assertTrue('"skinnable": true' in modules)
+        self.assertTrue('"type": "js"' in modules)
+        self.assertTrue('"type": CSS' in modules)
+        self.assertTrue('"use": ["dom"]' in modules)
+        self.assertTrue('"requires": ["widget"]' in modules)
+        self.assertTrue('"requires": [SKIN_SAM_LAZR_ANIM_CORE]' in modules)
+        self.assertTrue(
+            ('"after": [CSSRESET, CSSFONTS, CSSGRIDS, '
+            'CSSRESET_CONTEXT, CSSFONTS_CONTEXT, CSSGRIDS_CONTEXT, '
+            'SKIN_SAM_WIDGET, SKIN_SAM_LAZR_ANIM_CORE]') in modules)
+        self.assertTrue('"ext": true' in modules)
 
 
 def test_suite():