Avoid creating extraneous merge-trees. #6453

Merged
merged 1 commit into from Nov 23, 2016

Conversation

Projects
None yet
4 participants
@rwjblue
Contributor

rwjblue commented Nov 22, 2016

This change does three main things:

  • Ensure that we always use lib/broccoli/merge-trees.js over requiring broccoli-merge-trees directly.
  • When lib/broccoli/merge-trees.js is called with a single input tree, simply return it (instead of creating a broccoli-merge-tree instance).
  • When mergeTrees is called with an empty array, use a single EMPTY_MERGE_TREES constant instance for all invocations. This prevents us from creating many useless operations to essentially support an "empty" tree. It is possible that we might want to teach Broccoli itself about empty trees, but that is left as a future excercise.

The results of these changes even in a newly created empty app are fairly significant. Here are the counts of tmp directories created before and after:

  • 952 - Before changes
  • 274 - After changes

In a real app, these numbers would be even higher (due to addons at each level of nesting).


Credit goes to @stefanpenner for the idea and initial implementation (which was stolen from #6429).

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 22, 2016

Contributor

nice!

@rwjblue may complement this nicely broccolijs/broccoli-merge-trees#43 (collapsing merge of merge)

Contributor

stefanpenner commented Nov 22, 2016

nice!

@rwjblue may complement this nicely broccolijs/broccoli-merge-trees#43 (collapsing merge of merge)

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 22, 2016

Contributor

@homu r+

Contributor

rwjblue commented Nov 22, 2016

@homu r+

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Nov 22, 2016

Contributor

📌 Commit 380c2ec has been approved by rwjblue

Contributor

homu commented Nov 22, 2016

📌 Commit 380c2ec has been approved by rwjblue

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Nov 22, 2016

Contributor

⌛️ Testing commit 380c2ec with merge 95d4533...

Contributor

homu commented Nov 22, 2016

⌛️ Testing commit 380c2ec with merge 95d4533...

homu added a commit that referenced this pull request Nov 22, 2016

Auto merge of #6453 - ember-cli:avoid-needless-merge-trees, r=rwjblue
Avoid creating extraneous merge-trees.

This change does three main things:

* Ensure that we always use `lib/broccoli/merge-trees.js` over requiring `broccoli-merge-trees` directly.
* When `lib/broccoli/merge-trees.js` is called with a single input tree, simply return it (instead of creating a `broccoli-merge-tree` instance).
* When `mergeTrees` is called with an empty array, use a single `EMPTY_MERGE_TREES` constant instance for all invocations.  This prevents us from creating many useless operations to essentially support an "empty" tree.  It is possible that we might want to teach Broccoli itself about empty trees, but that is left as a future excercise.

The results of these changes even in a newly created empty app are fairly significant. Here are the counts of `tmp` directories created before and after:

* 952 - Before changes
* 316 - After changes

In a real app, these numbers would be even higher (due to addons at each level of nesting).

---

Credit goes to @stefanpenner for the idea and initial implementation (which was stolen from #6429).
lib/broccoli/merge-trees.js
- options.description = options.annotation;
- var tree = upstreamMergeTrees(inputTree, options);
+module.exports = function mergeTrees(_inputTrees, options) {
+ var inputTrees = _inputTrees.filter(Boolean);

This comment has been minimized.

@stefanpenner

stefanpenner Nov 22, 2016

Contributor

we may also want to filter out other EmptyMergeTrees here.

@stefanpenner

stefanpenner Nov 22, 2016

Contributor

we may also want to filter out other EmptyMergeTrees here.

lib/broccoli/merge-trees.js
- options = options || {};
- options.description = options.annotation;
- var tree = upstreamMergeTrees(inputTree, options);
+module.exports = function mergeTrees(_inputTrees, options) {

This comment has been minimized.

@stefanpenner

stefanpenner Nov 22, 2016

Contributor

we should likely test this guy.

@stefanpenner

stefanpenner Nov 22, 2016

Contributor

we should likely test this guy.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 22, 2016

Contributor

@homu r-

Contributor

rwjblue commented Nov 22, 2016

@homu r-

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Nov 22, 2016

Contributor

☀️ Test successful - status

Contributor

homu commented Nov 22, 2016

☀️ Test successful - status

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 22, 2016

Contributor

@stefanpenner - Updated with tests and better filtering when input array contains the EMPTY_MERGE_TREE constant (this reduced the number of tmp dirs further down to 274).

Contributor

rwjblue commented Nov 22, 2016

@stefanpenner - Updated with tests and better filtering when input array contains the EMPTY_MERGE_TREE constant (this reduced the number of tmp dirs further down to 274).

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 23, 2016

Contributor

@homu r+

Contributor

rwjblue commented Nov 23, 2016

@homu r+

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Nov 23, 2016

Contributor

📌 Commit 332c720 has been approved by rwjblue

Contributor

homu commented Nov 23, 2016

📌 Commit 332c720 has been approved by rwjblue

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Nov 23, 2016

Contributor

⌛️ Testing commit 332c720 with merge 5e8b19b...

Contributor

homu commented Nov 23, 2016

⌛️ Testing commit 332c720 with merge 5e8b19b...

homu added a commit that referenced this pull request Nov 23, 2016

Auto merge of #6453 - ember-cli:avoid-needless-merge-trees, r=rwjblue
Avoid creating extraneous merge-trees.

This change does three main things:

* Ensure that we always use `lib/broccoli/merge-trees.js` over requiring `broccoli-merge-trees` directly.
* When `lib/broccoli/merge-trees.js` is called with a single input tree, simply return it (instead of creating a `broccoli-merge-tree` instance).
* When `mergeTrees` is called with an empty array, use a single `EMPTY_MERGE_TREES` constant instance for all invocations.  This prevents us from creating many useless operations to essentially support an "empty" tree.  It is possible that we might want to teach Broccoli itself about empty trees, but that is left as a future excercise.

The results of these changes even in a newly created empty app are fairly significant. Here are the counts of `tmp` directories created before and after:

* 952 - Before changes
* 274 - After changes

In a real app, these numbers would be even higher (due to addons at each level of nesting).

---

Credit goes to @stefanpenner for the idea and initial implementation (which was stolen from #6429).
@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Nov 23, 2016

Contributor

☀️ Test successful - status

Contributor

homu commented Nov 23, 2016

☀️ Test successful - status

@homu homu merged commit 332c720 into master Nov 23, 2016

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 93.002%
Details
homu Test successful
Details

@homu homu referenced this pull request Nov 23, 2016

Closed

WIP: Cleanup some addon discovery related code #6429

1 of 4 tasks complete

@rwjblue rwjblue deleted the avoid-needless-merge-trees branch Nov 23, 2016

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 23, 2016

Contributor

Tested this in Ghost, the tmp dirs created during ember s went from 3444 to 966.

Contributor

rwjblue commented Nov 23, 2016

Tested this in Ghost, the tmp dirs created during ember s went from 3444 to 966.

@Turbo87 Turbo87 added this to the v2.11.0 milestone Dec 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment