Skip to content

Add additional memoization to different UI levels #8184

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

Closed
wants to merge 4 commits into from

Conversation

vovakulikov
Copy link
Contributor

This PR adds memoisation to different UI components to avoid unnecessary react renders (potentially it should improve memory allocation during LLM responses on the chat client)

Test plan

  • manual inspection of the chat logic

@vovakulikov vovakulikov requested review from fkling and pkukielka July 21, 2025 14:55
@vovakulikov vovakulikov self-assigned this Jul 21, 2025
Comment on lines 192 to 208
export function memoize<T extends (...args: any[]) => any>(
func: T
): (...args: Parameters<T>) => ReturnType<T> {
let lastArguments: any[] | null = null
let lastCalculatedValue: ReturnType<T> | null = null

return (...args: Parameters<T>): ReturnType<T> => {
if (isEqual(lastArguments, args)) {
return lastCalculatedValue as ReturnType<T>
}

lastArguments = args
lastCalculatedValue = func(args)

return lastCalculatedValue as ReturnType<T>
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lodash has a memoize function as well... does that not work for our purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does have memoize function, but this function gathers all arguments in key-value cache store, which will produce another place which GC can free up, so I remember only the last result

Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I tested this the same way as I tested my branch: #8183
and I get exactly the same (improved vs main) results

For me this does not fix the memory leak but makes an app much quicker.

Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants