From e34146fc149069824d7aef4c8f88ac59f6200a86 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 10 Apr 2015 09:34:46 -0700 Subject: [PATCH] fix(view_factory): fix caching of views Previous implementation had bugs, and did not cache per ProtoView. --- modules/angular2/src/core/application.js | 4 +- .../src/core/compiler/view_factory.js | 39 +++++----- .../src/render/dom/view/view_factory.js | 40 +++++----- .../test/core/compiler/view_factory_spec.js | 73 ++++++++++++++++++ .../test/render/dom/view/view_factory_spec.js | 75 +++++++++++++++++++ .../examples/src/hello_world/index_static.js | 8 +- 6 files changed, 195 insertions(+), 44 deletions(-) create mode 100644 modules/angular2/test/core/compiler/view_factory_spec.js create mode 100644 modules/angular2/test/render/dom/view/view_factory_spec.js diff --git a/modules/angular2/src/core/application.js b/modules/angular2/src/core/application.js index 4036748a50..b8ba991fc4 100644 --- a/modules/angular2/src/core/application.js +++ b/modules/angular2/src/core/application.js @@ -96,7 +96,7 @@ function _injectorBindings(appComponentType): List { (capacity, eventManager, shadowDomStrategy) => new rvf.ViewFactory(capacity, eventManager, shadowDomStrategy), [rvf.VIEW_POOL_CAPACITY, EventManager, ShadowDomStrategy] ), - bind(rvf.VIEW_POOL_CAPACITY).toValue(100000), + bind(rvf.VIEW_POOL_CAPACITY).toValue(10000), ProtoViewFactory, // TODO(tbosch): We need an explicit factory here, as // we are getting errors in dart2js with mirrors... @@ -104,7 +104,7 @@ function _injectorBindings(appComponentType): List { (capacity) => new ViewFactory(capacity), [VIEW_POOL_CAPACITY] ), - bind(VIEW_POOL_CAPACITY).toValue(100000), + bind(VIEW_POOL_CAPACITY).toValue(10000), Compiler, CompilerCache, TemplateResolver, diff --git a/modules/angular2/src/core/compiler/view_factory.js b/modules/angular2/src/core/compiler/view_factory.js index b095f7f5c7..5ea0fbf74c 100644 --- a/modules/angular2/src/core/compiler/view_factory.js +++ b/modules/angular2/src/core/compiler/view_factory.js @@ -12,37 +12,38 @@ export const VIEW_POOL_CAPACITY = 'ViewFactory.viewPoolCapacity'; @Injectable() export class ViewFactory { - _poolCapacity:number; - _pooledViews:List; + _poolCapacityPerProtoView:number; + _pooledViewsPerProtoView:Map>; - constructor(@Inject(VIEW_POOL_CAPACITY) capacity) { - this._poolCapacity = capacity; - this._pooledViews = ListWrapper.create(); + constructor(@Inject(VIEW_POOL_CAPACITY) poolCapacityPerProtoView) { + this._poolCapacityPerProtoView = poolCapacityPerProtoView; + this._pooledViewsPerProtoView = MapWrapper.create(); } getView(protoView:viewModule.AppProtoView):viewModule.AppView { - // TODO(tbosch): benchmark this scanning of views and maybe - // replace it with a fancy LRU Map/List combination... - var view; - for (var i=this._pooledViews.length-1; i>=0; i--) { - var pooledView = this._pooledViews[i]; - if (pooledView.proto === protoView) { - view = ListWrapper.removeAt(this._pooledViews, i); + var pooledViews = MapWrapper.get(this._pooledViewsPerProtoView, protoView); + if (isPresent(pooledViews)) { + var result = ListWrapper.removeLast(pooledViews); + if (pooledViews.length === 0) { + MapWrapper.delete(this._pooledViewsPerProtoView, protoView); } + return result; } - if (isBlank(view)) { - view = this._createView(protoView); - } - return view; + return this._createView(protoView); } returnView(view:viewModule.AppView) { if (view.hydrated()) { throw new BaseException('Only dehydrated Views can be put back into the pool!'); } - ListWrapper.push(this._pooledViews, view); - while (this._pooledViews.length > this._poolCapacity) { - ListWrapper.removeAt(this._pooledViews, 0); + var protoView = view.proto; + var pooledViews = MapWrapper.get(this._pooledViewsPerProtoView, protoView); + if (isBlank(pooledViews)) { + pooledViews = []; + MapWrapper.set(this._pooledViewsPerProtoView, protoView, pooledViews); + } + if (pooledViews.length < this._poolCapacityPerProtoView) { + ListWrapper.push(pooledViews, view); } } diff --git a/modules/angular2/src/render/dom/view/view_factory.js b/modules/angular2/src/render/dom/view/view_factory.js index 9990232aba..26e077cc01 100644 --- a/modules/angular2/src/render/dom/view/view_factory.js +++ b/modules/angular2/src/render/dom/view/view_factory.js @@ -18,41 +18,43 @@ export const VIEW_POOL_CAPACITY = 'render.ViewFactory.viewPoolCapacity'; @Injectable() export class ViewFactory { - _poolCapacity:number; - _pooledViews:List; + _poolCapacityPerProtoView:number; + _pooledViewsPerProtoView:Map>; _eventManager:EventManager; _shadowDomStrategy:ShadowDomStrategy; - constructor(@Inject(VIEW_POOL_CAPACITY) capacity, eventManager:EventManager, shadowDomStrategy:ShadowDomStrategy) { - this._poolCapacity = capacity; - this._pooledViews = ListWrapper.create(); + constructor(@Inject(VIEW_POOL_CAPACITY) poolCapacityPerProtoView, + eventManager:EventManager, shadowDomStrategy:ShadowDomStrategy) { + this._poolCapacityPerProtoView = poolCapacityPerProtoView; + this._pooledViewsPerProtoView = MapWrapper.create(); this._eventManager = eventManager; this._shadowDomStrategy = shadowDomStrategy; } getView(protoView:pvModule.RenderProtoView):viewModule.RenderView { - // TODO(tbosch): benchmark this scanning of views and maybe - // replace it with a fancy LRU Map/List combination... - var view; - for (var i=this._pooledViews.length-1; i>=0; i--) { - var pooledView = this._pooledViews[i]; - if (pooledView.proto === protoView) { - view = ListWrapper.removeAt(this._pooledViews, i); + var pooledViews = MapWrapper.get(this._pooledViewsPerProtoView, protoView); + if (isPresent(pooledViews)) { + var result = ListWrapper.removeLast(pooledViews); + if (pooledViews.length === 0) { + MapWrapper.delete(this._pooledViewsPerProtoView, protoView); } + return result; } - if (isBlank(view)) { - view = this._createView(protoView); - } - return view; + return this._createView(protoView); } returnView(view:viewModule.RenderView) { if (view.hydrated()) { view.dehydrate(); } - ListWrapper.push(this._pooledViews, view); - while (this._pooledViews.length > this._poolCapacity) { - ListWrapper.removeAt(this._pooledViews, 0); + var protoView = view.proto; + var pooledViews = MapWrapper.get(this._pooledViewsPerProtoView, protoView); + if (isBlank(pooledViews)) { + pooledViews = []; + MapWrapper.set(this._pooledViewsPerProtoView, protoView, pooledViews); + } + if (pooledViews.length < this._poolCapacityPerProtoView) { + ListWrapper.push(pooledViews, view); } } diff --git a/modules/angular2/test/core/compiler/view_factory_spec.js b/modules/angular2/test/core/compiler/view_factory_spec.js new file mode 100644 index 0000000000..8614adf550 --- /dev/null +++ b/modules/angular2/test/core/compiler/view_factory_spec.js @@ -0,0 +1,73 @@ +import {describe, ddescribe, it, iit, xit, xdescribe, expect, beforeEach, el} from 'angular2/test_lib'; + +import {ViewFactory} from 'angular2/src/core/compiler/view_factory'; +import {AppProtoView, AppView} from 'angular2/src/core/compiler/view'; +import {dynamicChangeDetection} from 'angular2/change_detection'; + +export function main() { + function createViewFactory({capacity}):ViewFactory { + return new ViewFactory(capacity); + } + + function createPv() { + return new AppProtoView(null, + null, + dynamicChangeDetection.createProtoChangeDetector('dummy', null)); + } + + describe('RenderViewFactory', () => { + it('should create views', () => { + var pv = createPv(); + var vf = createViewFactory({ + capacity: 1 + }); + expect(vf.getView(pv) instanceof AppView).toBe(true); + }); + + describe('caching', () => { + + it('should support multiple AppProtoViews', () => { + var capacity; + var pv1 = createPv(); + var pv2 = createPv(); + var vf = createViewFactory({ capacity: 2 }); + var view1 = vf.getView(pv1); + var view2 = vf.getView(pv2); + vf.returnView(view1); + vf.returnView(view2); + + expect(vf.getView(pv1)).toBe(view1); + expect(vf.getView(pv2)).toBe(view2); + }); + + it('should reuse the newest view that has been returned', () => { + var capacity; + var pv = createPv(); + var vf = createViewFactory({ capacity: 2 }); + var view1 = vf.getView(pv); + var view2 = vf.getView(pv); + vf.returnView(view1); + vf.returnView(view2); + + expect(vf.getView(pv)).toBe(view2); + }); + + it('should not add views when the capacity has been reached', () => { + var capacity; + var pv = createPv(); + var vf = createViewFactory({ capacity: 2 }); + var view1 = vf.getView(pv); + var view2 = vf.getView(pv); + var view3 = vf.getView(pv); + vf.returnView(view1); + vf.returnView(view2); + vf.returnView(view3); + + expect(vf.getView(pv)).toBe(view2); + expect(vf.getView(pv)).toBe(view1); + }); + + }); + + }); +} diff --git a/modules/angular2/test/render/dom/view/view_factory_spec.js b/modules/angular2/test/render/dom/view/view_factory_spec.js new file mode 100644 index 0000000000..0dbe5be059 --- /dev/null +++ b/modules/angular2/test/render/dom/view/view_factory_spec.js @@ -0,0 +1,75 @@ +import {describe, ddescribe, it, iit, xit, xdescribe, expect, beforeEach, el} from 'angular2/test_lib'; + +import {ViewFactory} from 'angular2/src/render/dom/view/view_factory'; +import {RenderProtoView} from 'angular2/src/render/dom/view/proto_view'; +import {RenderView} from 'angular2/src/render/dom/view/view'; + +export function main() { + function createViewFactory({capacity}):ViewFactory { + return new ViewFactory(capacity, null, null); + } + + function createPv() { + return new RenderProtoView({ + element: el('
'), + isRootView: false, + elementBinders: [] + }); + } + + describe('RenderViewFactory', () => { + it('should create views', () => { + var pv = createPv(); + var vf = createViewFactory({ + capacity: 1 + }); + expect(vf.getView(pv) instanceof RenderView).toBe(true); + }); + + describe('caching', () => { + + it('should support multiple RenderProtoViews', () => { + var capacity; + var pv1 = createPv(); + var pv2 = createPv(); + var vf = createViewFactory({ capacity: 2 }); + var view1 = vf.getView(pv1); + var view2 = vf.getView(pv2); + vf.returnView(view1); + vf.returnView(view2); + + expect(vf.getView(pv1)).toBe(view1); + expect(vf.getView(pv2)).toBe(view2); + }); + + it('should reuse the newest view that has been returned', () => { + var capacity; + var pv = createPv(); + var vf = createViewFactory({ capacity: 2 }); + var view1 = vf.getView(pv); + var view2 = vf.getView(pv); + vf.returnView(view1); + vf.returnView(view2); + + expect(vf.getView(pv)).toBe(view2); + }); + + it('should not add views when the capacity has been reached', () => { + var capacity; + var pv = createPv(); + var vf = createViewFactory({ capacity: 2 }); + var view1 = vf.getView(pv); + var view2 = vf.getView(pv); + var view3 = vf.getView(pv); + vf.returnView(view1); + vf.returnView(view2); + vf.returnView(view3); + + expect(vf.getView(pv)).toBe(view2); + expect(vf.getView(pv)).toBe(view1); + }); + + }); + + }); +} diff --git a/modules/examples/src/hello_world/index_static.js b/modules/examples/src/hello_world/index_static.js index f9ce46662e..b3ef782b95 100644 --- a/modules/examples/src/hello_world/index_static.js +++ b/modules/examples/src/hello_world/index_static.js @@ -202,7 +202,7 @@ function setup() { }); reflector.registerType(rvf.VIEW_POOL_CAPACITY, { - "factory": () => 100000, + "factory": () => 10000, "parameters": [], "annotations": [] }); @@ -222,7 +222,7 @@ function setup() { }); reflector.registerType(VIEW_POOL_CAPACITY, { - "factory": () => 100000, + "factory": () => 10000, "parameters": [], "annotations": [] }); @@ -261,7 +261,7 @@ function setup() { }); reflector.registerType(rvf.VIEW_POOL_CAPACITY, { - "factory": () => 100000, + "factory": () => 10000, "parameters": [], "annotations": [] }); @@ -281,7 +281,7 @@ function setup() { }); reflector.registerType(VIEW_POOL_CAPACITY, { - "factory": () => 100000, + "factory": () => 10000, "parameters": [], "annotations": [] });