Skip to content
This repository has been archived by the owner on Jul 13, 2020. It is now read-only.

Re-export performance #386

Closed
guybedford opened this issue Jun 2, 2015 · 11 comments
Closed

Re-export performance #386

guybedford opened this issue Jun 2, 2015 · 11 comments

Comments

@guybedford
Copy link
Member

Re-exports in the System.register module format call the setter functions for each export successively such that the setter functions get run all the way to the top of the dependency tree of exponential order in the depth of the re-exporting.

Aurelia currently has re-exporting depths of at least 4, which means that 10 deep exports becomes of the order of 10^4 setter runs, which is causing performance issues for them.

The proposal to fix this is to provide a bulk-export variation of the _export function in the System.register format that takes any object containing key value pairs (optionally an entire module object for export *) as arguments instead of key, value.

This way:

export * from 'p';

Can be written as:

System.register(['p'], function(_export) {
  return {
    setters: [function(m) {
      _export(m);
    }]
    execute: function() {}
  };
});

Then in the implementation, this single export function means this triggers a single bulk export event up the tree depth making the setter count O(1^d) instead of O(n^d) for the number of re-exports.

Normal rexport statements can also use the same logic:

export {p, q, r as s} from 'p';

can be written:

System.register(['p'], function(_export) {
  return {
    setters: [function(m) {
      _export({
        p: m.p,
        q: m.q,
        s: m.r
      });
    }]
    execute: function() {}
  };
});

I'll aim to create an implementation for this in Traceur after this coming major release and include it in a coming patch. I may have overlooked something having not yet tested this theory out completely, so will report back when I've got full confirmation that the above optimization is working and we can include it in the other transpilers.

/cc @EisenbergEffect @sebmck @vladima

@guybedford guybedford added the next label Jun 2, 2015
@EisenbergEffect
Copy link

Thanks Guy! I really appreciate everyone's cooperation. Though this affects Aurelia pretty significantly, I think it's understood that this has pretty broad affects. Probably most people haven't realized it yet.

@vladima
Copy link
Contributor

vladima commented Jun 2, 2015

@guybedford sure, I can add it to TypeScript once approach is confirmed to be valid and implementation details are availabl

@vladima
Copy link
Contributor

vladima commented Jun 2, 2015

@guybedford I know that it is probably to early to ask about it but can I give an approximate time (month?) of release for the patch that will include bulk export? This will help us prioritizing tasks on the TypeScript side

@guybedford
Copy link
Member Author

@vladima thanks for your help with getting these features going. It's difficult to give a release date - there are a lot of things to align here. With some luck, it should be out before Friday. Note that the named bundle support is far more important than this issue to complete the loop on the TypeScript support.

@guybedford
Copy link
Member Author

This is now tested and working in Traceur on google/traceur-compiler#1957. I'll create the test case for Babel now as well. @vladima feel free to go ahead with the TypeScript implementation.

@guybedford
Copy link
Member Author

Babel tests PR created at babel/babel#1722.

@vladima
Copy link
Contributor

vladima commented Jun 12, 2015

@guybedford just to elaborate, can you confirm that this is correct:

//es6
export * from 'a'
// setter in transpiled code
function (_a) {  
    export_(_a)
}

// es6
export {a,b as b1} from 'f';
// setter in transpiled code
function (_f) {
    var exports = {
        a: _f.a, 
        b1: _f.b
    };
    export_(exports);
}

@guybedford
Copy link
Member Author

@vladima to be spec-compliant export * needs some filtering in place:

//es6
export * from 'a'
// setter in transpiled code
function (_a) {
  var _rexports = {};
  Object.keys(_a).forEach(function(p) {
    if (p != 'default' && !moduleExports.contains(p))
      _exports[p] = _a[p];
  });
  export_(_rexports);
}

The moduleExports part is optional and something to come back to later - basically it allows us to filter out exports which have taken spec priority when export names collide.

The other case is exactly right.

@guybedford
Copy link
Member Author

This feature has been released - it is just pending implementations now.

@vladima
Copy link
Contributor

vladima commented Jul 21, 2015

@guybedford TypeScript now supports it (microsoft/TypeScript@d057f56)

@guybedford
Copy link
Member Author

@vladima excellent, that gives us implementations in all of them now! Babel has this behind as an optional transformer though (optimisation.modules.system) which should probably be documented somewhere.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants