Skip to content

Commit

Permalink
perf(core): speed up retrieval of DestroyRef in EventEmitter (#54748
Browse files Browse the repository at this point in the history
)

Speeds up the retrieval of `DestroyRef` in `EventEmitter` because
`try/catch` is expensive if there is no injection context.

We saw a script time regression in Cloud.

The goldens had to be updated because `getInjectImplementation` is now
referenced. `inject` also references the underlying field, but directly.
This is super minimal overhead of a function exposing the internal
field.

PR Close #54748
  • Loading branch information
devversion authored and atscott committed Mar 11, 2024
1 parent fcfb42a commit bb35414
Show file tree
Hide file tree
Showing 13 changed files with 47 additions and 8 deletions.
11 changes: 9 additions & 2 deletions packages/core/src/di/contextual.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@

import {RuntimeError, RuntimeErrorCode} from '../errors';
import {InjectorProfilerContext, setInjectorProfilerContext} from '../render3/debug/injector_profiler';

import {getInjectImplementation, setInjectImplementation} from './inject_switch';
import type {Injector} from './injector';
import {getCurrentInjector, setCurrentInjector} from './injector_compatibility';
import {getInjectImplementation, setInjectImplementation} from './inject_switch';
import {R3Injector} from './r3_injector';

/**
Expand Down Expand Up @@ -47,6 +48,12 @@ export function runInInjectionContext<ReturnT>(injector: Injector, fn: () => Ret
}
}

/**
* Whether the current stack frame is inside an injection context.
*/
export function isInInjectionContext(): boolean {
return getInjectImplementation() !== undefined || getCurrentInjector() != null;
}
/**
* Asserts that the current stack frame is within an [injection
* context](guide/dependency-injection-context) and has access to `inject`.
Expand All @@ -58,7 +65,7 @@ export function runInInjectionContext<ReturnT>(injector: Injector, fn: () => Ret
export function assertInInjectionContext(debugFn: Function): void {
// Taking a `Function` instead of a string name here prevents the unminified name of the function
// from being retained in the bundle regardless of minification.
if (!getInjectImplementation() && !getCurrentInjector()) {
if (!isInInjectionContext()) {
throw new RuntimeError(
RuntimeErrorCode.MISSING_INJECTION_CONTEXT,
ngDevMode &&
Expand Down
11 changes: 5 additions & 6 deletions packages/core/src/event_emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {setActiveConsumer} from '@angular/core/primitives/signals';
import {PartialObserver, Subject, Subscription} from 'rxjs';

import {OutputRef} from './authoring/output/output_ref';
import {isInInjectionContext} from './di/contextual';
import {inject} from './di/injector_compatibility';
import {DestroyRef} from './linker/destroy_ref';

Expand Down Expand Up @@ -107,18 +108,16 @@ export interface EventEmitter<T> extends Subject<T>, OutputRef<T> {

class EventEmitter_ extends Subject<any> implements OutputRef<any> {
__isAsync: boolean; // tslint:disable-line
destroyRef: DestroyRef|undefined;
destroyRef: DestroyRef|undefined = undefined;

constructor(isAsync: boolean = false) {
super();
this.__isAsync = isAsync;

// Attempt to retrieve a `DestroyRef` optionally.
// For backwards compatibility reasons, this cannot be required.
try {
this.destroyRef = inject(DestroyRef);
} catch {
this.destroyRef = undefined;
// For backwards compatibility reasons, this cannot be required
if (isInInjectionContext()) {
this.destroyRef = inject(DestroyRef, {optional: true}) ?? undefined;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,9 @@
{
"name": "getFirstLContainer"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -980,6 +980,9 @@
{
"name": "getFirstLContainer"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,9 @@
{
"name": "getFirstLContainer"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/bundling/defer/bundle.golden_symbols.json
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,9 @@
{
"name": "getFirstNativeNode"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,9 @@
{
"name": "getFirstNativeNode"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,9 @@
{
"name": "getFirstNativeNode"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,9 @@
{
"name": "getFirstLContainer"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,9 @@
{
"name": "getFirstLContainer"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/bundling/router/bundle.golden_symbols.json
Original file line number Diff line number Diff line change
Expand Up @@ -1283,6 +1283,9 @@
{
"name": "getInherited"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,9 @@
{
"name": "getFirstLContainer"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/bundling/todo/bundle.golden_symbols.json
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,9 @@
{
"name": "getFirstNativeNode"
},
{
"name": "getInjectImplementation"
},
{
"name": "getInjectableDef"
},
Expand Down

0 comments on commit bb35414

Please sign in to comment.