From ac6a4a528649d25268023c29297b99119ca5310b Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Mon, 13 Mar 2017 22:24:13 -0700 Subject: [PATCH 1/3] [BUGFIX beta] [PERF] Cache FactoryManagers * 1 FactoryManager per container/normalizeFullName * cache madeToString per factoryManager --- packages/container/lib/container.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/container/lib/container.js b/packages/container/lib/container.js index c84227b95f2..d21b9e165c7 100644 --- a/packages/container/lib/container.js +++ b/packages/container/lib/container.js @@ -38,6 +38,7 @@ export default function Container(registry, options) { this.owner = options && options.owner ? options.owner : null; this.cache = dictionary(options && options.cache ? options.cache : null); this.factoryCache = dictionary(options && options.factoryCache ? options.factoryCache : null); + this.factoryManagerCache = dictionary(options && options.factoryCache ? options.factoryCache : null); this.validationCache = dictionary(options && options.validationCache ? options.validationCache : null); this._fakeContainerToInject = buildFakeContainerWithDeprecations(this); this[CONTAINER_OVERRIDE] = undefined; @@ -281,6 +282,7 @@ if (isFeatureEnabled('ember-factory-for')) { */ Container.prototype.factoryFor = function _factoryFor(fullName, options = {}) { let normalizedName = this.registry.normalize(fullName); + assert('fullName must be a proper full name', this.registry.validateFullName(normalizedName)); if (options.source) { @@ -289,9 +291,15 @@ if (isFeatureEnabled('ember-factory-for')) { if (!normalizedName) { return; } } + let cached = this.factoryManagerCache[normalizedName]; + + if (cached) { return cached; } + let factory = this.registry.resolve(normalizedName); - if (factory === undefined) { return; } + if (factory === undefined) { + return; + } let manager = new FactoryManager(this, factory, fullName, normalizedName); @@ -299,6 +307,7 @@ if (isFeatureEnabled('ember-factory-for')) { manager = wrapManagerInDeprecationProxy(manager); }); + this.factoryManagerCache[normalizedName] = manager; return manager; }; } @@ -669,13 +678,14 @@ class FactoryManager { this.class = factory; this.fullName = fullName; this.normalizedName = normalizedName; + this.madeToString = undefined; } create(options = {}) { let injections = injectionsFor(this.container, this.normalizedName); let props = assign({}, injections, options); - props[NAME_KEY] = this.container.registry.makeToString(this.class, this.fullName); + props[NAME_KEY] = this.madeToString || (this.madeToString = this.container.registry.makeToString(this.class, this.fullName)); runInDebug(() => { let lazyInjections; From 148ee4a51b7f6f517b99c089a70bcf00d7253cb6 Mon Sep 17 00:00:00 2001 From: Chad Hietala Date: Tue, 14 Mar 2017 08:29:24 -0400 Subject: [PATCH 2/3] Adding test around factoryManager caching --- packages/container/tests/container_test.js | 42 +++++++++++++++------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/packages/container/tests/container_test.js b/packages/container/tests/container_test.js index 95d689a73d4..0223df43bd1 100644 --- a/packages/container/tests/container_test.js +++ b/packages/container/tests/container_test.js @@ -696,11 +696,11 @@ QUnit.test('#[FACTORY_FOR] class is the injected factory', (assert) => { let Component = factory(); registry.register('component:foo-bar', Component); - let factoryCreator = container[FACTORY_FOR]('component:foo-bar'); + let factoryManager = container[FACTORY_FOR]('component:foo-bar'); if (isFeatureEnabled('ember-no-double-extend')) { - assert.deepEqual(factoryCreator.class, Component, 'No double extend'); + assert.deepEqual(factoryManager.class, Component, 'No double extend'); } else { - assert.deepEqual(factoryCreator.class, lookupFactory('component:foo-bar', container), 'Double extended class'); + assert.deepEqual(factoryManager.class, lookupFactory('component:foo-bar', container), 'Double extended class'); } }); @@ -713,16 +713,32 @@ if (isFeatureEnabled('ember-factory-for')) { }, /Invalid Fullname, expected: 'type:name' got: chad-bar/); }); - QUnit.test('#factoryFor returns a factory creator', (assert) => { + QUnit.test('#factoryFor returns a factory manager', (assert) => { let registry = new Registry(); let container = registry.container(); let Component = factory(); registry.register('component:foo-bar', Component); - let factoryCreator = container.factoryFor('component:foo-bar'); - assert.ok(factoryCreator.create); - assert.ok(factoryCreator.class); + let factoryManager = container.factoryFor('component:foo-bar'); + assert.ok(factoryManager.create); + assert.ok(factoryManager.class); + }); + + QUnit.test('#factoryFor returns a cached factory manager for the same type', (assert) => { + let registry = new Registry(); + let container = registry.container(); + + let Component = factory(); + registry.register('component:foo-bar', Component); + registry.register('component:baz-bar', Component); + + let factoryManager1 = container.factoryFor('component:foo-bar'); + let factoryManager2 = container.factoryFor('component:foo-bar'); + let factoryManager3 = container.factoryFor('component:baz-bar'); + + assert.equal(factoryManager1, factoryManager2, 'cache hit'); + assert.notEqual(factoryManager1, factoryManager3, 'cache miss'); }); QUnit.test('#factoryFor class returns the factory function', (assert) => { @@ -732,8 +748,8 @@ if (isFeatureEnabled('ember-factory-for')) { let Component = factory(); registry.register('component:foo-bar', Component); - let factoryCreator = container.factoryFor('component:foo-bar'); - assert.deepEqual(factoryCreator.class, Component, 'No double extend'); + let factoryManager = container.factoryFor('component:foo-bar'); + assert.deepEqual(factoryManager.class, Component, 'No double extend'); }); QUnit.test('#factoryFor instance have a common parent', (assert) => { @@ -743,10 +759,10 @@ if (isFeatureEnabled('ember-factory-for')) { let Component = factory(); registry.register('component:foo-bar', Component); - let factoryCreator1 = container.factoryFor('component:foo-bar'); - let factoryCreator2 = container.factoryFor('component:foo-bar'); - let instance1 = factoryCreator1.create({ foo: 'foo' }); - let instance2 = factoryCreator2.create({ bar: 'bar' }); + let factoryManager1 = container.factoryFor('component:foo-bar'); + let factoryManager2 = container.factoryFor('component:foo-bar'); + let instance1 = factoryManager1.create({ foo: 'foo' }); + let instance2 = factoryManager2.create({ bar: 'bar' }); assert.deepEqual(instance1.constructor, instance2.constructor); }); From 005e2fa041ac00d26b03e37e712b23e68e8c71cd Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 14 Mar 2017 10:19:07 -0400 Subject: [PATCH 3/3] Update factoryManagerCache initial value to not share `factoryCache`. --- packages/container/lib/container.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/container/lib/container.js b/packages/container/lib/container.js index d21b9e165c7..352b22dddeb 100644 --- a/packages/container/lib/container.js +++ b/packages/container/lib/container.js @@ -38,7 +38,7 @@ export default function Container(registry, options) { this.owner = options && options.owner ? options.owner : null; this.cache = dictionary(options && options.cache ? options.cache : null); this.factoryCache = dictionary(options && options.factoryCache ? options.factoryCache : null); - this.factoryManagerCache = dictionary(options && options.factoryCache ? options.factoryCache : null); + this.factoryManagerCache = dictionary(options && options.factoryManagerCache ? options.factoryManagerCache : null); this.validationCache = dictionary(options && options.validationCache ? options.validationCache : null); this._fakeContainerToInject = buildFakeContainerWithDeprecations(this); this[CONTAINER_OVERRIDE] = undefined;