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
2020-02-04 21:41:49 -08:00 · 2020-02-04 21:41:49 -08:00 · b9b512f729
parent 27cd01f606
commit b9b512f729
5 changed files with 73 additions and 37 deletions
--- 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
      }
--- 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,28 +374,7 @@ 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<T = any>(level: number): T {
--- 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"
  },
--- 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"
  },
--- 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"
  },