-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Latency improved for activity keyer and tree composer #5465
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
base: main
Are you sure you want to change the base?
Latency improved for activity keyer and tree composer #5465
Conversation
@microsoft-github-policy-service agree company="Microsoft" |
@@ -143,5 +144,6 @@ export { | |||
useUIState, | |||
useUserID, | |||
useUsername, | |||
useVoiceSelector | |||
useVoiceSelector, | |||
useReduceActivities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort.
|
||
activityToKeyMapRef.current.set(activity, key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious why this would work.
In the new code, activityToKeyMapRef
is always frozen and setting to it should throw (in strict mode) or no-op (in non-strict mode).
|
||
activities.forEach(activity => { | ||
// Process new activities (if any) | ||
if (activities.length > lastProcessedIndexRef.current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The activities
array can change at any given position. Says, we have 3 activities [a1, b1, c1]
.
It could be updated and become [a2, b1, c1]
and the lastProcessedIndexRef
would missed that change.
Can we use useReduceActivities
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with useReduceActivities
but then I was not aware that activities can change at any given position in that case updated activity will come in prevActivities
in hook's argument and we need to process it again to fill our ref maps, that would nothing but iteration through all activities like before - is my understanding correct ?
const activityMap = activityMapRef.current; | ||
|
||
// Update or add the activity in the persistent Map | ||
activityMap.set(activityKey, activity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the activity
is removed from the rawActivities
, it will still keep in the activityMapRef
forever and this will cause memory leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's separate this PR into 3 parts:
- Remove
useUpsertedActivities
- Move
ActivityTreeComposer
to useuseReduceActivities
- Make
ActivityKeyerComposer
better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to consider across all these changes: the result returning from useActivities()
can change at any random position, including:
- Appending an activity at the end of the array
- Replacing an existing activity
- Removing an existing activity
- Inserting an activity at any position
The algorithm need to be more robust against this. Also make sure when activity is being replaced/removed, they will not be lingering in the system.
Please run tests locally.
Changelog Entry
ActivityKeyerComposer
andActivityTreeComposer
ActivitListenerComposer
anduseUpsertedActivities
hook as it is not needed anymore.Description
This is to improve latency of composer as we get larger number of activities when streaming is enabled and having multiple iteration in composer makes rendering slow
Visuals
Specific Changes
useReducerActivities
inActivityTreeComposer
to avoid recompute everything when new activity comes inActivityKeyerComposer
rather than re-computing and iterating through all activities when new activity comes inCHANGELOG.md
Review Checklist
z-index
)package.json
andpackage-lock.json
reviewed