From b9b512f7299e9ac8d0cdc0fe35e23750083d5af6 Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Tue, 4 Feb 2020 21:41:49 -0800 Subject: [PATCH] fix(ivy): `LFrame` needs to release memory on `leaveView()` (#35156) Root cause is that for perf reasons we cache `LFrame` so that we don't have to allocate it all the time. To be extra fast we clear the `LFrame` on `enterView()` rather that on `leaveView()`. The implication of this strategy is that the deepest `LFrame` will retain objects until the `LFrame` allocation depth matches the deepest object. The fix is to simply clear the `LFrame` on `leaveView()` rather then on `enterView()` Fix #35148 PR Close #35156 --- integration/_payload-limits.json | 12 +-- packages/core/src/render3/state.ts | 89 ++++++++++++------- .../cyclic_import/bundle.golden_symbols.json | 3 + .../hello_world/bundle.golden_symbols.json | 3 + .../bundling/todo/bundle.golden_symbols.json | 3 + 5 files changed, 73 insertions(+), 37 deletions(-) diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index 0af85caccc..a801d1bf30 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -3,7 +3,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 141000, + "main-es2015": 141569, "polyfills-es2015": 36657 } } @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 16312, + "main-es2015": 16514, "polyfills-es2015": 36657 } } @@ -21,7 +21,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 147082, + "main-es2015": 147647, "polyfills-es2015": 36657 } } @@ -30,7 +30,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 136250, + "main-es2015": 136777, "polyfills-es2015": 37334 } } @@ -39,7 +39,7 @@ "master": { "uncompressed": { "runtime-es2015": 2289, - "main-es2015": 246583, + "main-es2015": 247198, "polyfills-es2015": 36657, "5-es2015": 751 } @@ -49,7 +49,7 @@ "master": { "uncompressed": { "runtime-es2015": 2289, - "main-es2015": 225584, + "main-es2015": 226144, "polyfills-es2015": 36657, "5-es2015": 779 } diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 5bde42a6b2..8c1bfa479d 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -7,7 +7,7 @@ */ import {StyleSanitizeFn} from '../sanitization/style_sanitizer'; -import {assertDefined} from '../util/assert'; +import {assertDefined, assertEqual} from '../util/assert'; import {assertLViewOrUndefined} from './assert'; import {TNode} from './interfaces/node'; import {CONTEXT, DECLARATION_VIEW, LView, OpaqueViewState, TVIEW, TView} from './interfaces/view'; @@ -374,29 +374,8 @@ export function enterDI(newView: LView, tNode: TNode) { instructionState.lFrame = newLFrame; newLFrame.previousOrParentTNode = tNode !; newLFrame.lView = newView; - if (ngDevMode) { - // resetting for safety in dev mode only. - newLFrame.isParent = DEV_MODE_VALUE; - newLFrame.selectedIndex = DEV_MODE_VALUE; - newLFrame.contextLView = DEV_MODE_VALUE; - newLFrame.elementDepthCount = DEV_MODE_VALUE; - newLFrame.currentNamespace = DEV_MODE_VALUE; - newLFrame.currentSanitizer = DEV_MODE_VALUE; - newLFrame.bindingRootIndex = DEV_MODE_VALUE; - newLFrame.currentQueryIndex = DEV_MODE_VALUE; - } } -const DEV_MODE_VALUE: any = - 'Value indicating that DI is trying to read value which it should not need to know about.'; - -/** - * This is a light weight version of the `leaveView` which is needed by the DI system. - * - * Because the implementation is same it is only an alias - */ -export const leaveDI = leaveView; - /** * Swap the current lView with a new lView. * @@ -412,21 +391,25 @@ export const leaveDI = leaveView; export function enterView(newView: LView, tNode: TNode | null): void { ngDevMode && assertLViewOrUndefined(newView); const newLFrame = allocLFrame(); + if (ngDevMode) { + assertEqual(newLFrame.isParent, true, 'Expected clean LFrame'); + assertEqual(newLFrame.lView, null, 'Expected clean LFrame'); + assertEqual(newLFrame.tView, null, 'Expected clean LFrame'); + assertEqual(newLFrame.selectedIndex, 0, 'Expected clean LFrame'); + assertEqual(newLFrame.elementDepthCount, 0, 'Expected clean LFrame'); + assertEqual(newLFrame.currentDirectiveIndex, -1, 'Expected clean LFrame'); + assertEqual(newLFrame.currentNamespace, null, 'Expected clean LFrame'); + assertEqual(newLFrame.currentSanitizer, null, 'Expected clean LFrame'); + assertEqual(newLFrame.bindingRootIndex, -1, 'Expected clean LFrame'); + assertEqual(newLFrame.currentQueryIndex, 0, 'Expected clean LFrame'); + } const tView = newView[TVIEW]; instructionState.lFrame = newLFrame; newLFrame.previousOrParentTNode = tNode !; - newLFrame.isParent = true; newLFrame.lView = newView; newLFrame.tView = tView; - newLFrame.selectedIndex = 0; newLFrame.contextLView = newView !; - newLFrame.elementDepthCount = 0; - newLFrame.currentDirectiveIndex = -1; - newLFrame.currentNamespace = null; - newLFrame.currentSanitizer = null; - newLFrame.bindingRootIndex = -1; newLFrame.bindingIndex = tView.bindingStartIndex; - newLFrame.currentQueryIndex = 0; } /** @@ -461,8 +444,52 @@ function createLFrame(parent: LFrame | null): LFrame { return lFrame; } +/** + * A lightweight version of leave which is used with DI. + * + * This function only resets `previousOrParentTNode` and `LView` as those are the only properties + * used with DI (`enterDI()`). + * + * NOTE: This function is reexported as `leaveDI`. However `leaveDI` has return type of `void` where + * as `leaveViewLight` has `LFrame`. This is so that `leaveViewLight` can be used in `leaveView`. + */ +function leaveViewLight(): LFrame { + const oldLFrame = instructionState.lFrame; + instructionState.lFrame = oldLFrame.parent; + oldLFrame.previousOrParentTNode = null !; + oldLFrame.lView = null !; + return oldLFrame; +} + +/** + * This is a lightweight version of the `leaveView` which is needed by the DI system. + * + * NOTE: this function is an alias so that we can change the type of the function to have `void` + * return type. + */ +export const leaveDI: () => void = leaveViewLight; + +/** + * Leave the current `LView` + * + * This pops the `LFrame` with the associated `LView` from the stack. + * + * IMPORTANT: We must zero out the `LFrame` values here otherwise they will be retained. This is + * because for performance reasons we don't release `LFrame` but rather keep it for next use. + */ export function leaveView() { - instructionState.lFrame = instructionState.lFrame.parent; + const oldLFrame = leaveViewLight(); + oldLFrame.isParent = true; + oldLFrame.tView = null !; + oldLFrame.selectedIndex = 0; + oldLFrame.contextLView = null !; + oldLFrame.elementDepthCount = 0; + oldLFrame.currentDirectiveIndex = -1; + oldLFrame.currentNamespace = null; + oldLFrame.currentSanitizer = null; + oldLFrame.bindingRootIndex = -1; + oldLFrame.bindingIndex = -1; + oldLFrame.currentQueryIndex = 0; } export function nextContextImpl(level: number): T { diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index b2c2a743f7..facce6d3a7 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -485,6 +485,9 @@ { "name": "leaveView" }, + { + "name": "leaveViewLight" + }, { "name": "locateHostElement" }, diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 3198731638..91b04a2349 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -380,6 +380,9 @@ { "name": "leaveView" }, + { + "name": "leaveViewLight" + }, { "name": "locateHostElement" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 9b6679651a..7297319d19 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -911,6 +911,9 @@ { "name": "leaveView" }, + { + "name": "leaveViewLight" + }, { "name": "listenerInternal" },