Skip to content

ROX-28253: Add GetByQueryFn #14636

Closed
mtodor wants to merge 2 commits intomasterfrom
mtodor/ROX-28253-add-get-by-query-fn
Closed

ROX-28253: Add GetByQueryFn #14636
mtodor wants to merge 2 commits intomasterfrom
mtodor/ROX-28253-add-get-by-query-fn

Conversation

@mtodor
Copy link
Contributor

@mtodor mtodor commented Mar 17, 2025

Description

This PR is adding the GetByQueryFn function to storage. This way of handling rows returned by DB helps with:

  • faster processing because callbacks will be called as rows are received from DB over the network layer
  • reduced memory usage because intermediary objects have a shorter lifetime, and GC can clear them faster

During the evaluation of improvements, I have done the following testing:

  1. I have used default benchmark tests, but they didn't show any significant improvement (~4%) in memory allocations.
  2. More specific testing of impact on GC and ability to free memory

For 2. I did the following:

  • Parallel query execution The test spawns multiple goroutines to simulate memory-intensive workloads. This workload runs queries in a loop.
  • GC triggering: A separate goroutine forces garbage collection every second to observe GC effects.
  • Memory tracking: Another goroutine monitors and records the peak memory usage using runtime.ReadMemStats().

This test shows the following results (5 executions on each version):
OLD

    bench_test.go:151: Maximum memory usage: 598.23 MB
--- PASS: TestMemoryUsage (51.60s)

    bench_test.go:151: Maximum memory usage: 595.63 MB
--- PASS: TestMemoryUsage (52.03s)

    bench_test.go:151: Maximum memory usage: 578.43 MB
--- PASS: TestMemoryUsage (52.46s)

    bench_test.go:151: Maximum memory usage: 584.55 MB
--- PASS: TestMemoryUsage (53.32s)

    bench_test.go:151: Maximum memory usage: 587.40 MB
--- PASS: TestMemoryUsage (50.92s)

NEW

    bench_test.go:151: Maximum memory usage: 406.72 MB
--- PASS: TestMemoryUsage (51.33s)

    bench_test.go:151: Maximum memory usage: 307.58 MB
--- PASS: TestMemoryUsage (51.02s)

    bench_test.go:151: Maximum memory usage: 390.79 MB
--- PASS: TestMemoryUsage (50.57s)

    bench_test.go:151: Maximum memory usage: 329.87 MB
--- PASS: TestMemoryUsage (50.78s)

    bench_test.go:151: Maximum memory usage: 311.44 MB
--- PASS: TestMemoryUsage (51.74s)

Memory levels are way better with callbacks. The memory level was 70% of the initial code.

User-facing documentation

  • CHANGELOG is updated OR update is not needed
  • documentation PR is created and is linked above OR is not needed

Testing and quality

  • the change is production ready: the change is GA or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • no additional tests

How I validated my change

CI pipeline and local bench testing

@openshift-ci
Copy link

openshift-ci bot commented Mar 17, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mtodor mtodor force-pushed the mtodor/ROX-28253-add-get-by-query-fn branch from c253002 to 23126b6 Compare March 17, 2025 14:05
@rhacs-bot
Copy link
Contributor

rhacs-bot commented Mar 17, 2025

Images are ready for the commit at 83a63ac.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-363-g83a63ac716.

@janisz janisz force-pushed the walk_by_query_do_not_batch branch 2 times, most recently from 38b0c0e to a65a27c Compare March 24, 2025 11:02
@mtodor mtodor force-pushed the mtodor/ROX-28253-add-get-by-query-fn branch from 23126b6 to 799cec1 Compare April 3, 2025 15:10
@mtodor mtodor changed the base branch from walk_by_query_do_not_batch to master April 3, 2025 15:11
@@ -84,6 +84,7 @@ type Store interface {
Get(ctx context.Context, {{template "paramList" $pks}}) (*storeType, bool, error)
{{- if .SearchCategory }}
GetByQuery(ctx context.Context, query *v1.Query) ([]*storeType, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
GetByQuery(ctx context.Context, query *v1.Query) ([]*storeType, error)
// Deprecated: Use GetByQueryFn instead
GetByQuery(ctx context.Context, query *v1.Query) ([]*storeType, error)

@mtodor mtodor force-pushed the mtodor/ROX-28253-add-get-by-query-fn branch from 799cec1 to 83a63ac Compare April 3, 2025 15:45
@mtodor
Copy link
Contributor Author

mtodor commented May 6, 2025

PR is obsolete, and implementation is done within other PRs. Tnx @janisz!

@mtodor mtodor closed this May 6, 2025
@mtodor mtodor deleted the mtodor/ROX-28253-add-get-by-query-fn branch September 16, 2025 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants