Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(core): allow checkNoChanges mode to be tree-shaken in production #45913

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/src/change_detection/change_detector_ref.ts
Expand Up @@ -104,7 +104,7 @@ export abstract class ChangeDetectorRef {
* Checks the change detector and its children, and throws if any changes are detected.
*
* Use in development mode to verify that running change detection doesn't introduce
* other changes.
* other changes. Calling it in production mode is a noop.
*/
abstract checkNoChanges(): void;

Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/render3/instructions/advance.ts
Expand Up @@ -37,7 +37,8 @@ import {getLView, getSelectedIndex, getTView, isInCheckNoChangesMode, setSelecte
*/
export function ɵɵadvance(delta: number): void {
ngDevMode && assertGreaterThan(delta, 0, 'Can only advance forward');
selectIndexInternal(getTView(), getLView(), getSelectedIndex() + delta, isInCheckNoChangesMode());
selectIndexInternal(
getTView(), getLView(), getSelectedIndex() + delta, !!ngDevMode && isInCheckNoChangesMode());
}

export function selectIndexInternal(
Expand Down
14 changes: 9 additions & 5 deletions packages/core/src/render3/instructions/shared.ts
Expand Up @@ -384,7 +384,7 @@ export function refreshView<T>(
enterView(lView);
// Check no changes mode is a dev only mode used to verify that bindings have not changed
// since they were assigned. We do not want to execute lifecycle hooks in that mode.
const isInCheckNoChangesPass = isInCheckNoChangesMode();
const isInCheckNoChangesPass = ngDevMode && isInCheckNoChangesMode();
try {
resetPreOrderHookFlags(lView);

Expand Down Expand Up @@ -505,18 +505,22 @@ export function refreshView<T>(
export function renderComponentOrTemplate<T>(
tView: TView, lView: LView, templateFn: ComponentTemplate<{}>|null, context: T) {
const rendererFactory = lView[RENDERER_FACTORY];
const normalExecutionPath = !isInCheckNoChangesMode();

// Check no changes mode is a dev only mode used to verify that bindings have not changed
// since they were assigned. We do not want to invoke renderer factory functions in that mode
// to avoid any possible side-effects.
const checkNoChangesMode = !!ngDevMode && isInCheckNoChangesMode();
const creationModeIsActive = isCreationMode(lView);
try {
if (normalExecutionPath && !creationModeIsActive && rendererFactory.begin) {
if (!checkNoChangesMode && !creationModeIsActive && rendererFactory.begin) {
rendererFactory.begin();
}
if (creationModeIsActive) {
renderView(tView, lView, context);
}
refreshView(tView, lView, templateFn, context);
} finally {
if (normalExecutionPath && !creationModeIsActive && rendererFactory.end) {
if (!checkNoChangesMode && !creationModeIsActive && rendererFactory.end) {
rendererFactory.end();
}
}
Expand All @@ -531,7 +535,7 @@ function executeTemplate<T>(
if (isUpdatePhase && lView.length > HEADER_OFFSET) {
// When we're updating, inherently select 0 so we don't
// have to generate that instruction for most update blocks.
selectIndexInternal(tView, lView, HEADER_OFFSET, isInCheckNoChangesMode());
selectIndexInternal(tView, lView, HEADER_OFFSET, !!ngDevMode && isInCheckNoChangesMode());
}

const preHookType =
Expand Down
30 changes: 15 additions & 15 deletions packages/core/src/render3/state.ts
Expand Up @@ -7,7 +7,7 @@
*/

import {InjectFlags} from '../di/interface/injector';
import {assertDefined, assertEqual, assertGreaterThanOrEqual, assertLessThan, assertNotEqual} from '../util/assert';
import {assertDefined, assertEqual, assertGreaterThanOrEqual, assertLessThan, assertNotEqual, throwError} from '../util/assert';

import {assertLViewOrUndefined, assertTNodeForLView, assertTNodeForTView} from './assert';
import {DirectiveDef} from './interfaces/definition';
Expand Down Expand Up @@ -172,24 +172,23 @@ interface InstructionState {
* ```
*/
bindingsEnabled: boolean;

/**
* In this mode, any changes in bindings will throw an ExpressionChangedAfterChecked error.
*
* Necessary to support ChangeDetectorRef.checkNoChanges().
*
* checkNoChanges Runs only in devmode=true and verifies that no unintended changes exist in
* the change detector or its children.
*/
isInCheckNoChangesMode: boolean;
}

const instructionState: InstructionState = {
lFrame: createLFrame(null),
bindingsEnabled: true,
isInCheckNoChangesMode: false,
};

/**
* In this mode, any changes in bindings will throw an ExpressionChangedAfterChecked error.
*
* Necessary to support ChangeDetectorRef.checkNoChanges().
*
* The `checkNoChanges` function is invoked only in ngDevMode=true and verifies that no unintended
* changes exist in the change detector or its children.
*/
let _isInCheckNoChangesMode = false;

/**
* Returns true if the instruction state stack is empty.
*
Expand Down Expand Up @@ -353,12 +352,13 @@ export function getContextLView(): LView {
}

export function isInCheckNoChangesMode(): boolean {
// TODO(misko): remove this from the LView since it is ngDevMode=true mode only.
return instructionState.isInCheckNoChangesMode;
!ngDevMode && throwError('Must never be called in production mode');
JoostK marked this conversation as resolved.
Show resolved Hide resolved
return _isInCheckNoChangesMode;
}

export function setIsInCheckNoChangesMode(mode: boolean): void {
instructionState.isInCheckNoChangesMode = mode;
!ngDevMode && throwError('Must never be called in production mode');
_isInCheckNoChangesMode = mode;
}

// top level variables should not be exported for performance reasons (PERF_NOTES.md)
Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/render3/view_ref.ts
Expand Up @@ -281,7 +281,9 @@ export class ViewRef<T> implements viewEngine_EmbeddedViewRef<T>, viewEngine_Int
* introduce other changes.
AndrewKushnir marked this conversation as resolved.
Show resolved Hide resolved
*/
checkNoChanges(): void {
checkNoChangesInternal(this._lView[TVIEW], this._lView, this.context as unknown as {});
if (ngDevMode) {
checkNoChangesInternal(this._lView[TVIEW], this._lView, this.context as unknown as {});
}
}

attachToViewContainerRef() {
Expand Down Expand Up @@ -318,7 +320,9 @@ export class RootViewRef<T> extends ViewRef<T> {
}

override checkNoChanges(): void {
checkNoChangesInRootView(this._view);
if (ngDevMode) {
checkNoChangesInRootView(this._view);
}
}

override get context(): T {
Expand Down
12 changes: 0 additions & 12 deletions packages/core/test/bundling/animations/bundle.golden_symbols.json
Expand Up @@ -725,12 +725,6 @@
{
"name": "detachMovedView"
},
{
"name": "detectChangesInRootView"
},
{
"name": "detectChangesInternal"
},
{
"name": "diPublicInInjector"
},
Expand Down Expand Up @@ -1022,9 +1016,6 @@
{
"name": "isImportedNgModuleProviders"
},
{
"name": "isInCheckNoChangesMode"
},
{
"name": "isInlineTemplate"
},
Expand Down Expand Up @@ -1313,9 +1304,6 @@
{
"name": "setInputsFromAttrs"
},
{
"name": "setIsInCheckNoChangesMode"
},
{
"name": "setSelectedIndex"
},
Expand Down
Expand Up @@ -236,9 +236,6 @@
{
"name": "isCurrentTNodeParent"
},
{
"name": "isInCheckNoChangesMode"
},
{
"name": "isInlineTemplate"
},
Expand Down
Expand Up @@ -746,12 +746,6 @@
{
"name": "detachView"
},
{
"name": "detectChangesInRootView"
},
{
"name": "detectChangesInternal"
},
{
"name": "diPublicInInjector"
},
Expand Down Expand Up @@ -1127,9 +1121,6 @@
{
"name": "isImportedNgModuleProviders"
},
{
"name": "isInCheckNoChangesMode"
},
{
"name": "isInlineTemplate"
},
Expand Down Expand Up @@ -1463,9 +1454,6 @@
{
"name": "setInputsFromAttrs"
},
{
"name": "setIsInCheckNoChangesMode"
},
{
"name": "setSelectedIndex"
},
Expand Down
Expand Up @@ -719,12 +719,6 @@
{
"name": "detachView"
},
{
"name": "detectChangesInRootView"
},
{
"name": "detectChangesInternal"
},
{
"name": "diPublicInInjector"
},
Expand Down Expand Up @@ -1088,9 +1082,6 @@
{
"name": "isImportedNgModuleProviders"
},
{
"name": "isInCheckNoChangesMode"
},
{
"name": "isInlineTemplate"
},
Expand Down Expand Up @@ -1442,9 +1433,6 @@
{
"name": "setInputsFromAttrs"
},
{
"name": "setIsInCheckNoChangesMode"
},
{
"name": "setSelectedIndex"
},
Expand Down
Expand Up @@ -167,9 +167,6 @@
{
"name": "invertObject"
},
{
"name": "isInCheckNoChangesMode"
},
{
"name": "isLView"
},
Expand Down
12 changes: 0 additions & 12 deletions packages/core/test/bundling/router/bundle.golden_symbols.json
Expand Up @@ -1022,12 +1022,6 @@
{
"name": "detachView"
},
{
"name": "detectChangesInRootView"
},
{
"name": "detectChangesInternal"
},
{
"name": "diPublicInInjector"
},
Expand Down Expand Up @@ -1466,9 +1460,6 @@
{
"name": "isImportedNgModuleProviders"
},
{
"name": "isInCheckNoChangesMode"
},
{
"name": "isInlineTemplate"
},
Expand Down Expand Up @@ -1826,9 +1817,6 @@
{
"name": "setInputsFromAttrs"
},
{
"name": "setIsInCheckNoChangesMode"
},
{
"name": "setRouterState"
},
Expand Down
Expand Up @@ -461,12 +461,6 @@
{
"name": "detachMovedView"
},
{
"name": "detectChangesInRootView"
},
{
"name": "detectChangesInternal"
},
{
"name": "domRendererFactory3"
},
Expand Down Expand Up @@ -668,9 +662,6 @@
{
"name": "isImportedNgModuleProviders"
},
{
"name": "isInCheckNoChangesMode"
},
{
"name": "isLContainer"
},
Expand Down Expand Up @@ -848,9 +839,6 @@
{
"name": "setInjectImplementation"
},
{
"name": "setIsInCheckNoChangesMode"
},
{
"name": "setSelectedIndex"
},
Expand Down
12 changes: 0 additions & 12 deletions packages/core/test/bundling/todo/bundle.golden_symbols.json
Expand Up @@ -299,12 +299,6 @@
{
"name": "detachView"
},
{
"name": "detectChangesInRootView"
},
{
"name": "detectChangesInternal"
},
{
"name": "diPublicInInjector"
},
Expand Down Expand Up @@ -551,9 +545,6 @@
{
"name": "isDirectiveHost"
},
{
"name": "isInCheckNoChangesMode"
},
{
"name": "isInlineTemplate"
},
Expand Down Expand Up @@ -758,9 +749,6 @@
{
"name": "setInputsFromAttrs"
},
{
"name": "setIsInCheckNoChangesMode"
},
{
"name": "setSelectedIndex"
},
Expand Down