Skip to content
This repository was archived by the owner on Aug 1, 2025. It is now read-only.

Add additional memoization to different UI levels - #8184

Closed
vovakulikov wants to merge 4 commits into
mainfrom
vk/improve-memoization-of-chat-ui
Closed

Add additional memoization to different UI levels#8184
vovakulikov wants to merge 4 commits into
mainfrom
vk/improve-memoization-of-chat-ui

Conversation

@vovakulikov

Copy link
Copy Markdown
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 thread vscode/webviews/chat/Transcript.tsx Outdated
Comment thread lib/shared/src/utils.ts Outdated
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
Copy Markdown
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
Copy Markdown
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

@pkukielka pkukielka left a comment

Copy link
Copy Markdown
Contributor

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: https://github.com/sourcegraph/cody/pull/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.

@pkukielka pkukielka left a comment

Copy link
Copy Markdown
Contributor

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants