Bug 1314861: Some trivial optimizations that add up something significant. r?ochameau draft
authorKris Maglione <maglione.k@gmail.com>
Fri, 07 Apr 2017 18:05:18 -0700
changeset 558813 22e1611c9ba2e557344be3d32c783b192c7251e8
parent 558812 5f1754e1d3d7407451c191cd4e15243dd0be655a
child 558814 126d931d4e5787a7e4038b2b15bcca66f3bb7d9f
push id52953
push usermaglione.k@gmail.com
push dateSat, 08 Apr 2017 01:32:07 +0000
reviewersochameau
bugs1314861
milestone55.0a1
Bug 1314861: Some trivial optimizations that add up something significant. r?ochameau MozReview-Commit-ID: FmiH8daYxK8
addon-sdk/source/lib/toolkit/loader.js
addon-sdk/source/test/test-native-loader.js
--- a/addon-sdk/source/lib/toolkit/loader.js
+++ b/addon-sdk/source/lib/toolkit/loader.js
@@ -48,18 +48,20 @@ defineLazyGetter(this, "XulApp", () => {
                                      "sdk/system/xul-app.jsm");
   return Cu.import(xulappURI, {});
 });
 
 // Define some shortcuts.
 const bind = Function.call.bind(Function.bind);
 const getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;
 const prototypeOf = Object.getPrototypeOf;
-const getOwnIdentifiers = x => [...Object.getOwnPropertyNames(x),
-                                ...Object.getOwnPropertySymbols(x)];
+function* getOwnIdentifiers(x) {
+  yield* Object.getOwnPropertyNames(x);
+  yield* Object.getOwnPropertySymbols(x);
+}
 
 const NODE_MODULES = new Set([
   "assert",
   "buffer_ieee754",
   "buffer",
   "child_process",
   "cluster",
   "console",
@@ -120,19 +122,18 @@ function freeze(object) {
       freeze(object);
   }
   return object;
 }
 
 // Returns map of given `object`-s own property descriptors.
 const descriptor = iced(function descriptor(object) {
   let value = {};
-  getOwnIdentifiers(object).forEach(function(name) {
+  for (let name of getOwnIdentifiers(object))
     value[name] = getOwnPropertyDescriptor(object, name)
-  });
   return value;
 });
 Loader.descriptor = descriptor;
 
 // Freeze important built-ins so they can't be used by untrusted code as a
 // message passing channel.
 freeze(Object);
 freeze(Object.prototype);
@@ -155,21 +156,21 @@ function iced(f) {
 }
 
 // Defines own properties of given `properties` object on the given
 // target object overriding any existing property with a conflicting name.
 // Returns `target` object. Note we only export this function because it's
 // useful during loader bootstrap when other util modules can't be used &
 // thats only case where this export should be used.
 const override = iced(function override(target, source) {
-  let properties = descriptor(target)
-  let extension = descriptor(source || {})
-  getOwnIdentifiers(extension).forEach(function(name) {
-    properties[name] = extension[name];
-  });
+  let properties = descriptor(target);
+
+  for (let name of getOwnIdentifiers(source || {}))
+    properties[name] = getOwnPropertyDescriptor(source, name);
+
   return Object.defineProperties({}, properties);
 });
 Loader.override = override;
 
 function sourceURI(uri) { return String(uri).split(" -> ").pop(); }
 Loader.sourceURI = iced(sourceURI);
 
 function isntLoaderFrame(frame) { return frame.fileName !== module.uri }
@@ -550,52 +551,44 @@ const load = iced(function load(loader, 
       throw err;
     }
   }
 
   // Only freeze the exports object if we created it ourselves. Modules
   // which completely replace the exports object and still want it
   // frozen need to freeze it themselves.
   if (module.exports === originalExports)
-    freeze(module.exports);
+    Object.freeze(module.exports);
 
   return module;
 });
 Loader.load = load;
 
 // Utility function to normalize module `uri`s so they have `.js` extension.
 function normalizeExt(uri) {
   return isJSURI(uri) ? uri :
          isJSONURI(uri) ? uri :
          isJSMURI(uri) ? uri :
          uri + '.js';
 }
 
-// Strips `rootURI` from `string` -- used to remove absolute resourceURI
-// from a relative path
-function stripBase(rootURI, string) {
-  return string.replace(rootURI, './');
-}
-
 // Utility function to join paths. In common case `base` is a
 // `requirer.uri` but in some cases it may be `baseURI`. In order to
 // avoid complexity we require `baseURI` with a trailing `/`.
 const resolve = iced(function resolve(id, base) {
   if (!isRelative(id))
     return id;
 
   let baseDir = dirname(base);
-  if (!baseDir)
-    return normalize(id);
 
   let resolved = join(baseDir, id);
 
   // Joining and normalizing removes the './' from relative files.
   // We need to ensure the resolution still has the root
-  if (isRelative(base))
+  if (base.startsWith('./'))
     resolved = './' + resolved;
 
   return resolved;
 });
 Loader.resolve = resolve;
 
 // Attempts to load `path` and then `path.js`
 // Returns `path` with valid file, or `undefined` otherwise
@@ -631,17 +624,17 @@ function resolveAsDirectory(path) {
 }
 
 function resolveRelative(rootURI, modulesDir, id) {
   let fullId = join(rootURI, modulesDir, id);
 
   let resolvedPath = (resolveAsFile(fullId) ||
                       resolveAsDirectory(fullId));
   if (resolvedPath) {
-    return stripBase(rootURI, resolvedPath);
+    return './' + resolvedPath.slice(rootURI.length);
   }
 
   return null;
 }
 
 // From `resolve` module
 // https://github.com/substack/node-resolve/blob/master/lib/node-modules-paths.js
 function* getNodeModulePaths(rootURI, start) {
@@ -872,17 +865,17 @@ const Require = iced(function Require(lo
     // We also freeze module to prevent it from further changes
     // at runtime.
     if (!(uri in modules)) {
       // Many of the loader's functionalities are dependent
       // on modules[uri] being set before loading, so we set it and
       // remove it if we have any errors.
       module = modules[uri] = Module(requirement, uri);
       try {
-        freeze(load(loader, module));
+        Object.freeze(load(loader, module));
       }
       catch (e) {
         // Clear out modules cache so we can throw on a second invalid require
         delete modules[uri];
         // Also clear out the Sandbox that was created
         delete loader.sandboxes[uri];
         throw e;
       }
@@ -899,22 +892,16 @@ const Require = iced(function Require(lo
       throw Error('you must provide a module name when calling require() from '
                   + requirer.id, requirer.uri);
 
     let requirement, uri;
 
     // TODO should get native Firefox modules before doing node-style lookups
     // to save on loading time
     if (isNative) {
-      // If a requireMap is available from `generateMap`, use that to
-      // immediately resolve the node-style mapping.
-      // TODO: write more tests for this use case
-      if (requireMap && requireMap[requirer.id])
-        requirement = requireMap[requirer.id][id];
-
       let { overrides } = manifest.jetpack;
       for (let key in overrides) {
         // ignore any overrides using relative keys
         if (/^[.\/]/.test(key)) {
           continue;
         }
 
         // If the override is for x -> y,
@@ -927,18 +914,16 @@ const Require = iced(function Require(lo
       }
 
       // For native modules, we want to check if it's a module specified
       // in 'modules', like `chrome`, or `@loader` -- if it exists,
       // just set the uri to skip resolution
       if (!requirement && modules[id])
         uri = requirement = id;
 
-      // If no requireMap was provided, or resolution not found in
-      // the requireMap, and not a npm dependency, attempt a runtime lookup
       if (!requirement && !NODE_MODULES.has(id)) {
         // If `isNative` defined, this is using the new, native-style
         // loader, not cuddlefish, so lets resolve using node's algorithm
         // and get back a path that needs to be resolved via paths mapping
         // in `resolveURI`
         requirement = loaderResolve(id, requirer.id, {
           manifest: manifest,
           rootURI: rootURI
@@ -1201,32 +1186,29 @@ function Loader(options) {
         set: function(module) { main = main || module; }
       }
     }
   };
 
   if (isNative) {
     returnObj.isNative = { enumerable: false, value: true };
     returnObj.manifest = { enumerable: false, value: manifest };
-    returnObj.requireMap = { enumerable: false, value: requireMap };
     returnObj.rootURI = { enumerable: false, value: addTrailingSlash(rootURI) };
   }
 
   return freeze(Object.create(null, returnObj));
 };
 Loader.Loader = Loader;
 
 var isSystemURI = uri => /^resource:\/\/(gre|devtools|testing-common)\//.test(uri);
 
 var isJSONURI = uri => uri.endsWith('.json');
 var isJSMURI = uri => uri.endsWith('.jsm');
 var isJSURI = uri => uri.endsWith('.js');
-var isAbsoluteURI = uri => uri.startsWith("resource://") ||
-                           uri.startsWith("chrome://") ||
-                           uri.startsWith("file://");
+var isAbsoluteURI = uri => /^(resource|chrome|file|jar):/.test(uri);
 var isRelative = id => id.startsWith(".");
 
 // Default `main` entry to './index.js' and ensure is relative,
 // since node allows 'lib/index.js' without relative `./`
 function getManifestMain(manifest) {
   let main = manifest.main || './index.js';
   return isRelative(main) ? main : './' + main;
 }
--- a/addon-sdk/source/test/test-native-loader.js
+++ b/addon-sdk/source/test/test-native-loader.js
@@ -105,45 +105,16 @@ for (let variant of variants) {
 
   /*
   // TODO not working in current env
   exports[`test bundle (${variant.description`] = function (assert, done) {
     loadAddon('/native-addons/native-addon-test/')
   };
   */
 
-  exports[`test native Loader with mappings (${variant.description})`] = function (assert, done) {
-    all([
-      getJSON('/fixtures/native-addon-test/expectedmap.json'),
-      getJSON('/fixtures/native-addon-test/package.json')
-    ]).then(([expectedMap, manifest]) => {
-
-      // Override dummy module and point it to `test-math` to see if the
-      // require is pulling from the mapping
-      expectedMap['./index.js']['./dir/dummy'] = './dir/a.js';
-
-      let rootURI = variant.getRootURI('native-addon-test');
-      let loader = Loader({
-        paths: makePaths(rootURI),
-        rootURI: rootURI,
-        manifest: manifest,
-        requireMap: expectedMap,
-        isNative: true
-      });
-
-      let program = main(loader);
-      assert.equal(program.dummyModule, 'dir/a',
-        'The lookup uses the information given in the mapping');
-
-      testLoader(program, assert);
-      unload(loader);
-      done();
-    }).then(null, (reason) => console.error(reason));
-  };
-
   exports[`test native Loader overrides (${variant.description})`] = function*(assert) {
     const expectedKeys = Object.keys(require("sdk/io/fs")).join(", ");
     const manifest = yield getJSON('/fixtures/native-overrides-test/package.json');
     const rootURI = variant.getRootURI('native-overrides-test');
 
     let loader = Loader({
       paths: makePaths(rootURI),
       rootURI: rootURI,