Skip to content

perf: optimize memory usage in denoised-rows query by conditionally including processedRows #1177

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

perf: optimize memory usage in denoised-rows query by conditionally including processedRows #1177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9373de
perf: optimize memory usage in denoised-rows query by conditionally i…
brandon-pereira Sep 17, 2025
10560c9
add changeset
brandon-pereira Sep 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/sixty-mangos-join.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@hyperdx/app": patch
---

Improve memory efficiency in high row cound envs
11 changes: 10 additions & 1 deletion packages/app/src/components/DBRowTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import cx from 'classnames';
import { format, formatDistance } from 'date-fns';
import { isString } from 'lodash';
import curry from 'lodash/curry';
import ms from 'ms';
import { useHotkeys } from 'react-hotkeys-hook';
import {
Bar,
Expand Down Expand Up @@ -1267,11 +1268,18 @@ function DBSqlRowTableComponent({
queryKey: [
'denoised-rows',
config,
processedRows,
denoiseResults,
// Only include processed rows if denoising is enabled
// This helps prevent the queryKey from getting extremely large
// and causing memory issues, when it's not used.
...(denoiseResults ? [processedRows] : []),
Comment on lines +1271 to +1275
Copy link
Member

Choose a reason for hiding this comment

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

I’m scratching my head as to why this is relevant. even if denoise is disabled, react-query still tries to cache the key that blows up the memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, exactly!

You'd think the enabled flag being false would turn all this off, I can file a bug in the tanstack query library when I get back from vacation (and see if one already exists) if we want to help provide feedback.

Comment on lines +1271 to +1275
Copy link
Member

Choose a reason for hiding this comment

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

perf nit: Instead of passing all the rows, one idea is to generate an ID for processedRows. For example, we can take a fixed step to sample the rows and then compute a hash from those samples

Copy link
Contributor Author

@brandon-pereira brandon-pereira Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah good idea, I spent a large portion of time messing around with their queryKeyHashFn but I couldn't get it to have a clean & fast solution on the full processedRows dataset, but if we're sampling the results and generating a hash on those then I can definitely improve the perf on that front. This should reduce memory when denoising is enabled.

Let me take a look at this next week when I get back from vacation!

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 we don't need to go too deep down optimizing this path - my preference is if scaling issues continue to come up for this feature, we re-evaluate pushing down denoising into the clickhouse query itself as opposed to incremental improvements to the current implementation.

noisyPatternIds,
patternColumn,
],
queryFn: async () => {
if (!denoiseResults) {
return [];
}
// No noisy patterns, so no need to denoise
if (noisyPatternIds.length === 0) {
return processedRows;
Expand All @@ -1295,6 +1303,7 @@ function DBSqlRowTableComponent({
}
return undefined;
},
gcTime: isLive ? ms('30s') : ms('5m'), // more aggressive gc for live data, since it can end up holding lots of data
Copy link
Member

@wrn14897 wrn14897 Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

oh this is neat. theoretically we don't need to keep the old page if live tail is enabled

enabled:
denoiseResults &&
noisyPatterns.isSuccess &&
Expand Down
Loading