Skip to content

Commit 45490e5

Browse files
authored
fix: ActivityKeyerComposer perf (#5790)
* WIP * Changelog * Fix tests * Update packages/api/src/providers/ActivityKeyer/ActivityKeyerComposer.tsx * Fix lint
1 parent 8aef13a commit 45490e5

2 files changed

Lines changed: 121 additions & 15 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,7 @@ Breaking changes in this release:
337337
- Improved adaptive cards rendering in copilot variant, in PR [#5682](https://github.com/microsoft/BotFramework-WebChat/pull/5682), by [@OEvgeny](https://github.com/OEvgeny)
338338
- Bumped to [`botframework-directlinejs@0.15.8`](https://www.npmjs.com/package/botframework-directlinejs/v/0.15.8) to include support for the new `streaming` property, by [@pranavjoshi001](https://github.com/pranavjoshi001), in PR [#5686](https://github.com/microsoft/BotFramework-WebChat/pull/5686)
339339
- Removed unused deps `simple-git`, by [@compulim](https://github.com/compulim), in PR [#5786](https://github.com/microsoft/BotFramework-WebChat/pull/5786)
340+
- Improved `ActivityKeyerComposer` performance for append scenarios by adding an incremental fast path that only processes newly-appended activities, in PR [#5790](https://github.com/microsoft/BotFramework-WebChat/pull/5790), by [@OEvgeny](https://github.com/OEvgeny)
340341

341342
### Deprecated
342343

packages/api/src/providers/ActivityKeyer/ActivityKeyerComposer.tsx

Lines changed: 120 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,105 @@ const ActivityKeyerComposer = ({ children }: Readonly<{ children?: ReactNode | u
3939
}
4040

4141
const [activities] = useActivities();
42-
const activityIdToKeyMapRef = useRef<Readonly<ActivityIdToKeyMap>>(Object.freeze(new Map()));
43-
const activityToKeyMapRef = useRef<Readonly<ActivityToKeyMap>>(Object.freeze(new Map()));
44-
const clientActivityIdToKeyMapRef = useRef<Readonly<ClientActivityIdToKeyMap>>(Object.freeze(new Map()));
45-
const keyToActivitiesMapRef = useRef<Readonly<KeyToActivitiesMap>>(Object.freeze(new Map()));
4642

47-
// TODO: [P1] `useMemoWithPrevious` to check and cache the resulting array if it hasn't changed.
43+
// TODO: [P0] We should remove the mapping in favor of `localId`,
44+
// the id should represent the latest available activity in the stream
45+
//
46+
// Maps are intentionally mutable so the incremental fast path can append to them in-place.
47+
const activityIdToKeyMapRef = useRef<ActivityIdToKeyMap>(new Map());
48+
const activityToKeyMapRef = useRef<ActivityToKeyMap>(new Map());
49+
const clientActivityIdToKeyMapRef = useRef<ClientActivityIdToKeyMap>(new Map());
50+
const keyToActivitiesMapRef = useRef<KeyToActivitiesMap>(new Map());
51+
const prevActivitiesRef = useRef<readonly WebChatActivity[]>(Object.freeze([]));
52+
const prevActivityKeysStateRef = useRef<readonly [readonly string[]]>(
53+
Object.freeze([Object.freeze([])]) as readonly [readonly string[]]
54+
);
55+
56+
// Incremental keying: the fast path only processes newly-appended activities (O(delta) per render)
57+
// instead of re-iterating all activities (O(n) per render, O(n²) total for n streaming pushes).
4858
const activityKeysState = useMemo<readonly [readonly string[]]>(() => {
59+
const prevActivities = prevActivitiesRef.current;
60+
61+
// Detect how many leading activities are identical (same reference) to the previous render.
62+
let commonPrefixLength = 0;
63+
const maxPrefix = Math.min(prevActivities.length, activities.length);
64+
65+
// eslint-disable-next-line security/detect-object-injection
66+
while (commonPrefixLength < maxPrefix && prevActivities[commonPrefixLength] === activities[commonPrefixLength]) {
67+
commonPrefixLength++;
68+
}
69+
70+
const isAppendOnly = commonPrefixLength === prevActivities.length;
71+
72+
if (isAppendOnly) {
73+
// Fast path: only new activities were appended — process them incrementally.
74+
if (commonPrefixLength === activities.length) {
75+
// Array reference changed but content is identical.
76+
prevActivitiesRef.current = activities;
77+
78+
return prevActivityKeysStateRef.current;
79+
}
80+
81+
const { current: activityIdToKeyMap } = activityIdToKeyMapRef;
82+
const { current: activityToKeyMap } = activityToKeyMapRef;
83+
const { current: clientActivityIdToKeyMap } = clientActivityIdToKeyMapRef;
84+
const { current: keyToActivitiesMap } = keyToActivitiesMapRef;
85+
86+
const newKeys: string[] = [];
87+
88+
for (let i = commonPrefixLength; i < activities.length; i++) {
89+
// eslint-disable-next-line security/detect-object-injection
90+
const activity = activities[i];
91+
const activityId = getActivityId(activity);
92+
const clientActivityId = getClientActivityId(activity);
93+
const typingActivityId = getActivityLivestreamingMetadata(activity)?.sessionId;
94+
95+
// Since we mutate maps in-place, a single lookup covers both "previous" and
96+
// "current-iteration" entries — equivalent to the slow path's dual-map check.
97+
const key =
98+
(clientActivityId && clientActivityIdToKeyMap.get(clientActivityId)) ||
99+
(typingActivityId && activityIdToKeyMap.get(typingActivityId)) ||
100+
(activityId && activityIdToKeyMap.get(activityId)) ||
101+
activityToKeyMap.get(activity) ||
102+
uniqueId();
103+
104+
activityId && activityIdToKeyMap.set(activityId, key);
105+
typingActivityId && activityIdToKeyMap.set(typingActivityId, key);
106+
clientActivityId && clientActivityIdToKeyMap.set(clientActivityId, key);
107+
activityToKeyMap.set(activity, key);
108+
109+
const activitiesForKey = keyToActivitiesMap.get(key);
110+
111+
keyToActivitiesMap.set(
112+
key,
113+
activitiesForKey ? Object.freeze([...activitiesForKey, activity]) : Object.freeze([activity])
114+
);
115+
116+
!activitiesForKey && newKeys.push(key);
117+
}
118+
119+
prevActivitiesRef.current = activities;
120+
121+
if (newKeys.length) {
122+
const nextKeys = Object.freeze([...prevActivityKeysStateRef.current[0], ...newKeys]);
123+
const result = Object.freeze([nextKeys]) as readonly [readonly string[]];
124+
125+
prevActivityKeysStateRef.current = result;
126+
127+
return result;
128+
}
129+
130+
// New activities were added to existing keys — no new keys, but the keyToActivitiesMap
131+
// was mutated. Return a new tuple reference so context consumers re-render and see the
132+
// updated activities-per-key via getActivitiesByKey.
133+
const result = Object.freeze([prevActivityKeysStateRef.current[0]]) as readonly [readonly string[]];
134+
135+
prevActivityKeysStateRef.current = result;
136+
137+
return result;
138+
}
139+
140+
// Slow path: activities were removed or reordered — full recalculation.
49141
const { current: activityIdToKeyMap } = activityIdToKeyMapRef;
50142
const { current: activityToKeyMap } = activityToKeyMapRef;
51143
const { current: clientActivityIdToKeyMap } = clientActivityIdToKeyMapRef;
@@ -76,20 +168,33 @@ const ActivityKeyerComposer = ({ children }: Readonly<{ children?: ReactNode | u
76168
nextActivityToKeyMap.set(activity, key);
77169
nextActivityKeys.add(key);
78170

79-
const activities = nextKeyToActivitiesMap.has(key) ? [...nextKeyToActivitiesMap.get(key)] : [];
171+
const activitiesForKey = nextKeyToActivitiesMap.has(key) ? [...nextKeyToActivitiesMap.get(key)] : [];
80172

81-
activities.push(activity);
82-
nextKeyToActivitiesMap.set(key, Object.freeze(activities));
173+
activitiesForKey.push(activity);
174+
nextKeyToActivitiesMap.set(key, Object.freeze(activitiesForKey));
83175
});
84176

85-
activityIdToKeyMapRef.current = Object.freeze(nextActivityIdToKeyMap);
86-
activityToKeyMapRef.current = Object.freeze(nextActivityToKeyMap);
87-
clientActivityIdToKeyMapRef.current = Object.freeze(nextClientActivityIdToKeyMap);
88-
keyToActivitiesMapRef.current = Object.freeze(nextKeyToActivitiesMap);
177+
activityIdToKeyMapRef.current = nextActivityIdToKeyMap;
178+
activityToKeyMapRef.current = nextActivityToKeyMap;
179+
clientActivityIdToKeyMapRef.current = nextClientActivityIdToKeyMap;
180+
keyToActivitiesMapRef.current = nextKeyToActivitiesMap;
181+
prevActivitiesRef.current = activities;
182+
183+
const nextKeys = Object.freeze([...nextActivityKeys.values()]);
184+
const result = Object.freeze([nextKeys]) as readonly [readonly string[]];
185+
186+
prevActivityKeysStateRef.current = result;
89187

90-
// `nextActivityKeys` could potentially same as `prevActivityKeys` despite reference differences, we should memoize it.
91-
return Object.freeze([Object.freeze([...nextActivityKeys.values()])]) as readonly [readonly string[]];
92-
}, [activities, activityIdToKeyMapRef, activityToKeyMapRef, clientActivityIdToKeyMapRef, keyToActivitiesMapRef]);
188+
return result;
189+
}, [
190+
activities,
191+
activityIdToKeyMapRef,
192+
activityToKeyMapRef,
193+
clientActivityIdToKeyMapRef,
194+
keyToActivitiesMapRef,
195+
prevActivitiesRef,
196+
prevActivityKeysStateRef
197+
]);
93198

94199
const getActivitiesByKey: (key?: string | undefined) => readonly WebChatActivity[] | undefined = useCallback(
95200
(key?: string | undefined): readonly WebChatActivity[] | undefined => key && keyToActivitiesMapRef.current.get(key),

0 commit comments

Comments
 (0)