Skip to content

Commit fa6d9c2

Browse files
crisbetokirjs
authored andcommitted
refactor(core): track match index of directives (#60075)
If we want to target an input write to a directive, we have to know the index at which its instance is stored. Technically we can already find this by looking through `TView.data`, but that'll require a linear lookup for each write which can get slow. These changes introduce the new `TNode.directiveToIndex` map which allows us to quickly find the index of a directive based on its definition, as well as any host directives that its might've brought in. PR Close #60075
1 parent d5f467e commit fa6d9c2

File tree

15 files changed

+140
-43
lines changed

15 files changed

+140
-43
lines changed

packages/core/src/render3/interfaces/node.ts

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,12 @@
55
* Use of this source code is governed by an MIT-style license that can be
66
* found in the LICENSE file at https://angular.dev/license
77
*/
8+
import {Type} from '../../interface/type';
89
import {KeyValueArray} from '../../util/array_utils';
910
import {TStylingRange} from '../interfaces/styling';
1011
import {AttributeMarker} from './attribute_marker';
1112

1213
import {TIcu} from './i18n';
13-
import {InputFlags} from './input_flags';
1414
import {CssSelector} from './projection';
1515
import {RNode} from './renderer_dom';
1616
import type {LView, TView} from './view';
@@ -441,6 +441,11 @@ export interface TNode {
441441
*/
442442
hostDirectiveOutputs: HostDirectiveOutputs | null;
443443

444+
/**
445+
* Mapping between directive classes applied to the node and their indexes.
446+
*/
447+
directiveToIndex: DirectiveIndexMap | null;
448+
444449
/**
445450
* The TView attached to this node.
446451
*
@@ -857,6 +862,30 @@ export type HostDirectiveInputs = Record<string, (number | string)[]>;
857862
*/
858863
export type HostDirectiveOutputs = Record<string, (number | string)[]>;
859864

865+
/**
866+
* Represents a map between a class reference and the index at which its directive is available on
867+
* a specific TNode. The value can be either:
868+
* 1. A number means that there's only one selector-matched directive on the node and it
869+
* doesn't have any host directives.
870+
* 2. An array means that there's a selector-matched directive and it has host directives.
871+
* The array is structured as follows:
872+
* - 0: Index of the selector-matched directive.
873+
* - 1: Start index of the range within which the host directives are defined.
874+
* - 2: End of the host directive range.
875+
*
876+
* Example:
877+
* ```
878+
* Map {
879+
* [NoHostDirectives]: 5,
880+
* [HasHostDirectives]: [10, 6, 8],
881+
* }
882+
* ```
883+
*/
884+
export type DirectiveIndexMap = Map<
885+
Type<unknown>,
886+
number | [directiveIndex: number, hostDirectivesStart: number, hostDirectivesEnd: number]
887+
>;
888+
860889
/**
861890
* Type representing a set of TNodes that can have local refs (`#foo`) placed on them.
862891
*/

packages/core/src/render3/tnode_manipulation.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,7 @@ export function createTNode(
296296
hostDirectiveInputs: null,
297297
outputs: null,
298298
hostDirectiveOutputs: null,
299+
directiveToIndex: null,
299300
tView: null,
300301
next: null,
301302
prev: null,

packages/core/src/render3/view/directives.ts

Lines changed: 97 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {RuntimeError, RuntimeErrorCode} from '../../errors';
1010
import {Writable} from '../../interface/type';
1111
import {DoCheck, OnChanges, OnInit} from '../../interface/lifecycle_hooks';
1212
import {
13+
assertDefined,
1314
assertGreaterThan,
1415
assertGreaterThanOrEqual,
1516
assertNotEqual,
@@ -39,7 +40,7 @@ import {
3940
type TElementNode,
4041
type TNode,
4142
} from '../interfaces/node';
42-
import {isComponentDef, isComponentHost} from '../interfaces/type_checks';
43+
import {isComponentDef} from '../interfaces/type_checks';
4344
import {HEADER_OFFSET, HostBindingOpCodes, type LView, type TView} from '../interfaces/view';
4445
import {isInlineTemplate} from '../node_selector_matcher';
4546
import {NO_CHANGE} from '../tokens';
@@ -51,6 +52,13 @@ export type DirectiveMatcherStrategy = (
5152
tNode: TElementNode | TContainerNode | TElementContainerNode,
5253
) => DirectiveDef<unknown>[] | null;
5354

55+
/**
56+
* Map that tracks a selector-matched directive to the range within which its host directives
57+
* are declared. Host directives for a specific directive are always contiguous within the runtime.
58+
* Note that both the start and end are inclusive and they're both **after** `tNode.directiveStart`.
59+
*/
60+
type HostDirectiveRanges = Map<DirectiveDef<unknown>, [start: number, end: number]>;
61+
5462
/**
5563
* Resolve the matched directives on a node.
5664
*/
@@ -61,21 +69,28 @@ export function resolveDirectives(
6169
localRefs: string[] | null,
6270
directiveMatcher: DirectiveMatcherStrategy,
6371
): void {
64-
// Please make sure to have explicit type for `exportsMap`. Inferred type triggers bug in
65-
// tsickle.
72+
// Please make sure to have explicit type for `exportsMap`. Inferred type triggers bug in tsickle.
6673
ngDevMode && assertFirstCreatePass(tView);
6774

6875
const exportsMap: Record<string, number> | null = localRefs === null ? null : {'': -1};
6976
const matchedDirectiveDefs = directiveMatcher(tView, tNode);
7077

7178
if (matchedDirectiveDefs !== null) {
72-
const [directiveDefs, hostDirectiveDefs] = resolveHostDirectives(
79+
const [directiveDefs, hostDirectiveDefs, hostDirectiveRanges] = resolveHostDirectives(
7380
tView,
7481
tNode,
7582
matchedDirectiveDefs,
7683
);
7784

78-
initializeDirectives(tView, lView, tNode, directiveDefs, exportsMap, hostDirectiveDefs);
85+
initializeDirectives(
86+
tView,
87+
lView,
88+
tNode,
89+
directiveDefs,
90+
exportsMap,
91+
hostDirectiveDefs,
92+
hostDirectiveRanges,
93+
);
7994
}
8095
if (exportsMap !== null && localRefs !== null) {
8196
cacheMatchingLocalNames(tNode, localRefs, exportsMap);
@@ -104,38 +119,51 @@ function cacheMatchingLocalNames(
104119
}
105120
}
106121

107-
export function resolveHostDirectives(
122+
function resolveHostDirectives(
108123
tView: TView,
109124
tNode: TNode,
110125
matches: DirectiveDef<unknown>[],
111-
): [matches: DirectiveDef<unknown>[], hostDirectiveDefs: HostDirectiveDefs | null] {
126+
): [
127+
matches: DirectiveDef<unknown>[],
128+
hostDirectiveDefs: HostDirectiveDefs | null,
129+
hostDirectiveRanges: HostDirectiveRanges | null,
130+
] {
112131
const allDirectiveDefs: DirectiveDef<unknown>[] = [];
132+
const hasComponent = matches.length > 0 && isComponentDef(matches[0]);
113133
let hostDirectiveDefs: HostDirectiveDefs | null = null;
114-
115-
for (const def of matches) {
134+
let hostDirectiveRanges: HostDirectiveRanges | null = null;
135+
136+
// Components are inserted at the front of the matches array so that their lifecycle
137+
// hooks run before any directive lifecycle hooks. This appears to be for ViewEngine
138+
// compatibility. This logic doesn't make sense with host directives, because it
139+
// would allow the host directives to undo any overrides the host may have made.
140+
// To handle this case, the host directives of components are inserted at the beginning
141+
// of the array, followed by the component. As such, the insertion order is as follows:
142+
// 1. Host directives belonging to the selector-matched component.
143+
// 2. Selector-matched component.
144+
// 3. Host directives belonging to selector-matched directives.
145+
// 4. Selector-matched dir
146+
if (hasComponent) {
147+
const def = matches[0];
116148
if (def.findHostDirectiveDefs !== null) {
117-
// TODO(pk): probably could return matches instead of taking in an array to fill in?
149+
hostDirectiveRanges ??= new Map();
118150
hostDirectiveDefs ??= new Map();
119-
// Components are inserted at the front of the matches array so that their lifecycle
120-
// hooks run before any directive lifecycle hooks. This appears to be for ViewEngine
121-
// compatibility. This logic doesn't make sense with host directives, because it
122-
// would allow the host directives to undo any overrides the host may have made.
123-
// To handle this case, the host directives of components are inserted at the beginning
124-
// of the array, followed by the component. As such, the insertion order is as follows:
125-
// 1. Host directives belonging to the selector-matched component.
126-
// 2. Selector-matched component.
127-
// 3. Host directives belonging to selector-matched directives.
128-
// 4. Selector-matched directives.
129-
def.findHostDirectiveDefs(def, allDirectiveDefs, hostDirectiveDefs);
151+
resolveHostDirectivesForDef(def, allDirectiveDefs, hostDirectiveRanges, hostDirectiveDefs);
130152
}
153+
markAsComponentHost(tView, tNode, allDirectiveDefs.push(def) - 1);
154+
}
131155

132-
if (isComponentDef(def)) {
133-
allDirectiveDefs.push(def);
134-
markAsComponentHost(tView, tNode, allDirectiveDefs.length - 1);
156+
// If there's a component, we already processed it above so we can skip it here.
157+
for (let i = hasComponent ? 1 : 0; i < matches.length; i++) {
158+
const def = matches[i];
159+
if (def.findHostDirectiveDefs !== null) {
160+
hostDirectiveRanges ??= new Map();
161+
hostDirectiveDefs ??= new Map();
162+
resolveHostDirectivesForDef(def, allDirectiveDefs, hostDirectiveRanges, hostDirectiveDefs);
135163
}
136164
}
137165

138-
if (isComponentHost(tNode)) {
166+
if (hasComponent) {
139167
allDirectiveDefs.push(...matches.slice(1));
140168
} else {
141169
allDirectiveDefs.push(...matches);
@@ -145,7 +173,23 @@ export function resolveHostDirectives(
145173
assertNoDuplicateDirectives(allDirectiveDefs);
146174
}
147175

148-
return [allDirectiveDefs, hostDirectiveDefs];
176+
return [allDirectiveDefs, hostDirectiveDefs, hostDirectiveRanges];
177+
}
178+
179+
function resolveHostDirectivesForDef(
180+
def: DirectiveDef<unknown>,
181+
allDirectiveDefs: DirectiveDef<unknown>[],
182+
hostDirectiveRanges: HostDirectiveRanges,
183+
hostDirectiveDefs: HostDirectiveDefs,
184+
) {
185+
ngDevMode && assertDefined(def.findHostDirectiveDefs, 'Expected host directive resolve function');
186+
const start = allDirectiveDefs.length;
187+
// TODO(pk): probably could return matches instead of taking in an array to fill in?
188+
def.findHostDirectiveDefs!(def, allDirectiveDefs, hostDirectiveDefs);
189+
190+
// Note that these indexes are within the offset by `directiveStart`. We can't do the
191+
// offsetting here, because `directiveStart` hasn't been initialized on the TNode yet.
192+
hostDirectiveRanges.set(def, [start, allDirectiveDefs.length - 1]);
149193
}
150194

151195
/**
@@ -168,38 +212,46 @@ function initializeDirectives(
168212
directives: DirectiveDef<unknown>[],
169213
exportsMap: {[key: string]: number} | null,
170214
hostDirectiveDefs: HostDirectiveDefs | null,
215+
hostDirectiveRanges: HostDirectiveRanges | null,
171216
) {
172217
ngDevMode && assertFirstCreatePass(tView);
173218

219+
const directivesLength = directives.length;
220+
174221
// Publishes the directive types to DI so they can be injected. Needs to
175222
// happen in a separate pass before the TNode flags have been initialized.
176-
for (let i = 0; i < directives.length; i++) {
223+
for (let i = 0; i < directivesLength; i++) {
177224
diPublicInInjector(getOrCreateNodeInjectorForNode(tNode, lView), tView, directives[i].type);
178225
}
179226

180-
initTNodeFlags(tNode, tView.data.length, directives.length);
227+
initTNodeFlags(tNode, tView.data.length, directivesLength);
181228

182229
// When the same token is provided by several directives on the same node, some rules apply in
183230
// the viewEngine:
184231
// - viewProviders have priority over providers
185232
// - the last directive in NgModule.declarations has priority over the previous one
186233
// So to match these rules, the order in which providers are added in the arrays is very
187234
// important.
188-
for (let i = 0; i < directives.length; i++) {
235+
for (let i = 0; i < directivesLength; i++) {
189236
const def = directives[i];
190237
if (def.providersResolver) def.providersResolver(def);
191238
}
192239
let preOrderHooksFound = false;
193240
let preOrderCheckHooksFound = false;
194-
let directiveIdx = allocExpando(tView, lView, directives.length, null);
241+
let directiveIdx = allocExpando(tView, lView, directivesLength, null);
195242
ngDevMode &&
196243
assertSame(
197244
directiveIdx,
198245
tNode.directiveStart,
199246
'TNode.directiveStart should point to just allocated space',
200247
);
201248

202-
for (let i = 0; i < directives.length; i++) {
249+
// If there's at least one directive, we'll have to track it so initialize the map.
250+
if (directivesLength > 0) {
251+
tNode.directiveToIndex = new Map();
252+
}
253+
254+
for (let i = 0; i < directivesLength; i++) {
203255
const def = directives[i];
204256
// Merge the attrs in the order of matches. This assumes that the first directive is the
205257
// component itself, so that the component has the least priority.
@@ -208,6 +260,20 @@ function initializeDirectives(
208260
configureViewWithDirective(tView, tNode, lView, directiveIdx, def);
209261
saveNameToExportMap(directiveIdx, def, exportsMap);
210262

263+
// If a directive has host directives, we need to track both its index and the range within
264+
// the host directives are declared. Host directives are not tracked, but should be resolved
265+
// by looking up the host and getting its indexes from there.
266+
if (hostDirectiveRanges !== null && hostDirectiveRanges.has(def)) {
267+
const [start, end] = hostDirectiveRanges.get(def)!;
268+
tNode.directiveToIndex!.set(def.type, [
269+
directiveIdx,
270+
start + tNode.directiveStart,
271+
end + tNode.directiveStart,
272+
]);
273+
} else if (hostDirectiveDefs === null || !hostDirectiveDefs.has(def)) {
274+
tNode.directiveToIndex!.set(def.type, directiveIdx);
275+
}
276+
211277
if (def.contentQueries !== null) tNode.flags |= TNodeFlags.hasContentQuery;
212278
if (def.hostBindings !== null || def.hostAttrs !== null || def.hostVars !== 0)
213279
tNode.flags |= TNodeFlags.hasHostBindings;

packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,6 @@
395395
"makeTimingAst",
396396
"map",
397397
"markAncestorsForTraversal",
398-
"markAsComponentHost",
399398
"markViewDirty",
400399
"markViewForRefresh",
401400
"markedFeatures",
@@ -441,6 +440,7 @@
441440
"requiresRefreshOrTraversal",
442441
"resetPreOrderHookFlags",
443442
"resolveForwardRef",
443+
"resolveHostDirectivesForDef",
444444
"resolveTiming",
445445
"resolveTimingValue",
446446
"roundOffset",

packages/core/test/bundling/animations/bundle.golden_symbols.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,6 @@
418418
"makeTimingAst",
419419
"map",
420420
"markAncestorsForTraversal",
421-
"markAsComponentHost",
422421
"markViewDirty",
423422
"markViewForRefresh",
424423
"markedFeatures",
@@ -467,6 +466,7 @@
467466
"requiresRefreshOrTraversal",
468467
"resetPreOrderHookFlags",
469468
"resolveForwardRef",
469+
"resolveHostDirectivesForDef",
470470
"resolveTiming",
471471
"resolveTimingValue",
472472
"roundOffset",

packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,6 @@
337337
"makeRecord",
338338
"map",
339339
"markAncestorsForTraversal",
340-
"markAsComponentHost",
341340
"markViewDirty",
342341
"markViewForRefresh",
343342
"markedFeatures",
@@ -378,6 +377,7 @@
378377
"requiresRefreshOrTraversal",
379378
"resetPreOrderHookFlags",
380379
"resolveForwardRef",
380+
"resolveHostDirectivesForDef",
381381
"runEffectsInView",
382382
"runInInjectionContext",
383383
"saveNameToExportMap",

packages/core/test/bundling/defer/bundle.golden_symbols.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -796,7 +796,6 @@
796796
"makeRecord",
797797
"map",
798798
"markAncestorsForTraversal",
799-
"markAsComponentHost",
800799
"markViewDirty",
801800
"markViewForRefresh",
802801
"markedFeatures",
@@ -841,6 +840,7 @@
841840
"resetPreOrderHookFlags",
842841
"resolveDirectives",
843842
"resolveForwardRef",
843+
"resolveHostDirectivesForDef",
844844
"retrieveHydrationInfo",
845845
"runEffectsInView",
846846
"runInInjectionContext",

packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,6 @@
501501
"makeValidatorsArray",
502502
"map",
503503
"markAncestorsForTraversal",
504-
"markAsComponentHost",
505504
"markDuplicates",
506505
"markViewDirty",
507506
"markViewForRefresh",
@@ -570,6 +569,7 @@
570569
"resetPreOrderHookFlags",
571570
"resolveDirectives",
572571
"resolveForwardRef",
572+
"resolveHostDirectivesForDef",
573573
"resolveProvider",
574574
"runEffectsInView",
575575
"runInInjectionContext",

packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,6 @@
488488
"makeValidatorsArray",
489489
"map",
490490
"markAncestorsForTraversal",
491-
"markAsComponentHost",
492491
"markDuplicates",
493492
"markViewDirty",
494493
"markViewForRefresh",
@@ -561,6 +560,7 @@
561560
"resetPreOrderHookFlags",
562561
"resolveDirectives",
563562
"resolveForwardRef",
563+
"resolveHostDirectivesForDef",
564564
"resolveProvider",
565565
"resolvedPromise",
566566
"resolvedPromise2",

0 commit comments

Comments
 (0)