test: basic benchmarks for workflow archive DB operations #13767
Conversation
This adds basic benchmarks for listing and counting archived workflows so we can evaluate potential optimizations, e.g. argoproj#13601 ``` $ make BenchmarkWorkflowArchive GIT_COMMIT=f218fc540367840d013a99642590d3509560de51 GIT_BRANCH=feat-postgresql-jsonb GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false TARGET_PLATFORM=linux/amd64 RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true go test --tags api,cli,cron,executor,examples,corefunctional,functional,plugins ./test/e2e -run='BenchmarkWorkflowArchive' -benchmem -bench . WARN[0000] Non-transient error: <nil> WARN[0000] Non-transient error: <nil> goos: linux goarch: amd64 pkg: github.com/argoproj/argo-workflows/v3/test/e2e cpu: 12th Gen Intel(R) Core(TM) i5-12400 BenchmarkWorkflowArchive/ListWorkflows-12 6 167109091 ns/op 527468 B/op 8614 allocs/op --- BENCH: BenchmarkWorkflowArchive/ListWorkflows-12 workflow_archive_test.go:27: Found 100 workflows workflow_archive_test.go:27: Found 100 workflows workflow_archive_test.go:27: Found 100 workflows workflow_archive_test.go:27: Found 100 workflows workflow_archive_test.go:27: Found 100 workflows workflow_archive_test.go:27: Found 100 workflows workflow_archive_test.go:27: Found 100 workflows BenchmarkWorkflowArchive/CountWorkflows-12 31 36799882 ns/op 9022 B/op 212 allocs/op --- BENCH: BenchmarkWorkflowArchive/CountWorkflows-12 workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows workflow_archive_test.go:37: Found 100756 workflows ... [output truncated] PASS ok github.com/argoproj/argo-workflows/v3/test/e2e 3.392s ``` Signed-off-by: Mason Malone <[email protected]>
MasonM
force-pushed
the
test-db-benchmarks
branch
from
5502599
to
5711f07
Compare
Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
|
@MasonM This is on my TODO to test out and approve. Not really related to this PR: It would be nice if the benchmark results were a displayed on PRs as a comment so reviewers can more easily review performance impacts of the PR, it would be some effort to properly get this into CI I'm guessing. |
With PostgreSQL, the `argo_archived_workflows.workflow` column has been of type `json` ever since 8a1e611, which was released as v2.5.0. Therefore, the `::json` casts do nothing, and prevent users from improving performance by migrating to JSONB using the following query: ```sql alter table argo_archived_workflows alter column workflow set data type jsonb using workflow::jsonb ``` Without the changes in this PR, running the above will massively slow down the queries, because casting JSONB to JSON is expensive. With the changes in this PR, it improves performance by ~80%, which I determined by running the benchmarks added in argoproj#13767 against a DB with 100,000 randomly generated workflows generated by argoproj#13715: ``` $ benchstat postgres_before_10000_workflows.txt postgres_after_10000_workflows.txt goos: linux goarch: amd64 pkg: github.com/argoproj/argo-workflows/v3/test/e2e cpu: 12th Gen Intel(R) Core(TM) i5-12400 │ postgres_before_10000_workflows.txt │ postgres_after_10000_workflows.txt │ │ sec/op │ sec/op vs base │ WorkflowArchive/ListWorkflows-12 185.81m ± ∞ ¹ 25.06m ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/ListWorkflows_with_label_selector-12 186.35m ± ∞ ¹ 25.99m ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/CountWorkflows-12 42.39m ± ∞ ¹ 11.78m ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 113.6m 19.72m -82.64% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 ``` The only downside to migrating to JSONB is it can take a long time if you've got a ton of workflows (~72s on my local DB with 100,000 workflows). I'll enter a separate PR for the migration, but I'm entering this change separately so it can hopefully go out in 3.6.0. Signed-off-by: Mason Malone <[email protected]>
With PostgreSQL, the `argo_archived_workflows.workflow` column has been of type `json` ever since 8a1e611, which was released as v2.5.0. Therefore, the `::json` casts do nothing, and prevent users from improving performance by migrating to JSONB using the following query: ```sql alter table argo_archived_workflows alter column workflow set data type jsonb using workflow::jsonb ``` Without the changes in this PR, running the above will massively slow down the queries, because casting JSONB to JSON is expensive. With the changes in this PR, it improves performance by ~80%, which I determined by running the benchmarks added in argoproj#13767 against a DB with 100,000 randomly generated workflows generated by argoproj#13715: ``` $ benchstat postgres_before_10000_workflows.txt postgres_after_10000_workflows.txt goos: linux goarch: amd64 pkg: github.com/argoproj/argo-workflows/v3/test/e2e cpu: 12th Gen Intel(R) Core(TM) i5-12400 │ postgres_before_10000_workflows.txt │ postgres_after_10000_workflows.txt │ │ sec/op │ sec/op vs base │ WorkflowArchive/ListWorkflows-12 185.81m ± ∞ ¹ 25.06m ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/ListWorkflows_with_label_selector-12 186.35m ± ∞ ¹ 25.99m ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/CountWorkflows-12 42.39m ± ∞ ¹ 11.78m ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 113.6m 19.72m -82.64% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 ``` The only downside to migrating to JSONB is it can take a long time if you've got a ton of workflows (~72s on my local DB with 100,000 workflows). I'll enter a separate PR for the migration, but I'm entering this change separately so it can hopefully go out in 3.6.0. Signed-off-by: Mason Malone <[email protected]>
…j#13601 As explained in argoproj#13601 (comment), I believe argoproj#12912 introduced a performance regression when listing workflows for PostgreSQL users. Reverting that PR could re-introduce the memory issues mentioned in the PR description, so instead this mitigates the impact by converting the `workflow` column to be of type `jsonb`. Initially `workflow` was of type `text`, and was migrated to `json` in argoproj#2152. I'm not sure why `jsonb` wasn't chosen, but [based on this comment in the linked issue](argoproj#2133 (comment)), I think it was simply an oversight. Here's the relevant docs (https://www.postgresql.org/docs/current/datatype-json.html): > The json and jsonb data types accept almost identical sets of values as input. The major practical difference is one of efficiency. The json data type stores an exact copy of the input text, which processing functions must reparse on each execution; while jsonb data is stored in a decomposed binary format that makes it slightly slower to input due to added conversion overhead, but significantly faster to process, since no reparsing is needed. jsonb also supports indexing, which can be a significant advantage. > > Because the json type stores an exact copy of the input text, it will preserve semantically-insignificant white space between tokens, as well as the order of keys within JSON objects. Also, if a JSON object within the value contains the same key more than once, all the key/value pairs are kept. (The processing functions consider the last value as the operative one.) By contrast, jsonb does not preserve white space, does not preserve the order of object keys, and does not keep duplicate object keys. If duplicate keys are specified in the input, only the last value is kept. > > In general, most applications should prefer to store JSON data as jsonb, unless there are quite specialized needs, such as legacy assumptions about ordering of object keys. I'm pretty sure we don't care about key order or whitespace. We do care somewhat about insertion speed, but archived workflows are read much more frequently than written, so a slight reduction in write speed that gives a large improvement in read speed is a good tradeoff. Here's steps to test this: 1. Use argoproj#13715 to generate 100,000 randomized workflows, with https://gist.github.com/MasonM/52932ff6644c3c0ccea9e847780bfd90 as a template: ``` $ time go run ./hack/db fake-archived-workflows --template "@very-large-workflow.yaml" --rows 100000 Using seed 1935828722624432788 Clusters: [default] Namespaces: [argo] Inserted 100000 rows real 18m35.316s user 3m2.447s sys 0m44.972s ``` 2. Run the benchmarks using argoproj#13767: ``` make BenchmarkWorkflowArchive > postgres_before_10000_workflows.txt ``` 3. Run the migration the DB CLI: ``` $ time go run ./hack/db migrate INFO[0000] Migrating database schema clusterName=default dbType=postgres INFO[0000] applying database change change="alter table argo_archived_workflows alter column workflow set data type jsonb using workflow::jsonb" changeSchemaVersion=60 2024/10/17 18:07:42 Session ID: 00001 Query: alter table argo_archived_workflows alter column workflow set data type jsonb using workflow::jsonb Stack: fmt.(*pp).handleMethods@/usr/local/go/src/fmt/print.go:673 fmt.(*pp).printArg@/usr/local/go/src/fmt/print.go:756 fmt.(*pp).doPrint@/usr/local/go/src/fmt/print.go:1208 fmt.Append@/usr/local/go/src/fmt/print.go:289 log.(*Logger).Print.func1@/usr/local/go/src/log/log.go:261 log.(*Logger).output@/usr/local/go/src/log/log.go:238 log.(*Logger).Print@/usr/local/go/src/log/log.go:260 github.com/argoproj/argo-workflows/v3/persist/sqldb.ansiSQLChange.apply@/home/vscode/go/src/github.com/argoproj/argo-workflows/persist/sqldb/ansi_sql_change.go:11 github.com/argoproj/argo-workflows/v3/persist/sqldb.migrate.applyChange.func1@/home/vscode/go/src/github.com/argoproj/argo-workflows/persist/sqldb/migrate.go:295 github.com/argoproj/argo-workflows/v3/persist/sqldb.migrate.applyChange@/home/vscode/go/src/github.com/argoproj/argo-workflows/persist/sqldb/migrate.go:284 github.com/argoproj/argo-workflows/v3/persist/sqldb.migrate.Exec@/home/vscode/go/src/github.com/argoproj/argo-workflows/persist/sqldb/migrate.go:273 main.NewMigrateCommand.func1@/home/vscode/go/src/github.com/argoproj/argo-workflows/hack/db/main.go:50 github.com/spf13/cobra.(*Command).execute@/home/vscode/go/pkg/mod/github.com/spf13/[email protected]/command.go:985 github.com/spf13/cobra.(*Command).ExecuteC@/home/vscode/go/pkg/mod/github.com/spf13/[email protected]/command.go:1117 github.com/spf13/cobra.(*Command).Execute@/home/vscode/go/pkg/mod/github.com/spf13/[email protected]/command.go:1041 main.main@/home/vscode/go/src/github.com/argoproj/argo-workflows/hack/db/main.go:39 runtime.main@/usr/local/go/src/runtime/proc.go:272 runtime.goexit@/usr/local/go/src/runtime/asm_amd64.s:1700 Rows affected: 0 Error: upper: slow query Time taken: 69.12755s Context: context.Background real 1m10.087s user 0m1.541s sys 0m0.410s ``` 2. Re-run the benchmarks: ``` make BenchmarkWorkflowArchive > postgres_after_10000_workflows.txt ``` 4. Compare results using [benchstat](https://pkg.go.dev/golang.org/x/perf/cmd/benchstat): ``` $ benchstat postgres_before_10000_workflows3.txt postgres_after_10000_workflows2.txt goos: linux goarch: amd64 pkg: github.com/argoproj/argo-workflows/v3/test/e2e cpu: 12th Gen Intel(R) Core(TM) i5-12400 │ postgres_before_10000_workflows3.txt │ postgres_after_10000_workflows2.txt │ │ sec/op │ sec/op vs base │ WorkflowArchive/ListWorkflows-12 183.83m ± ∞ ¹ 24.69m ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/ListWorkflows_with_label_selector-12 192.71m ± ∞ ¹ 25.87m ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/CountWorkflows-12 13.04m ± ∞ ¹ 11.75m ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 77.31m 19.58m -74.68% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 │ postgres_before_10000_workflows3.txt │ postgres_after_10000_workflows2.txt │ │ B/op │ B/op vs base │ WorkflowArchive/ListWorkflows-12 497.2Ki ± ∞ ¹ 497.5Ki ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/ListWorkflows_with_label_selector-12 503.1Ki ± ∞ ¹ 503.9Ki ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/CountWorkflows-12 8.972Ki ± ∞ ¹ 8.899Ki ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 130.9Ki 130.7Ki -0.20% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 │ postgres_before_10000_workflows3.txt │ postgres_after_10000_workflows2.txt │ │ allocs/op │ allocs/op vs base │ WorkflowArchive/ListWorkflows-12 8.373k ± ∞ ¹ 8.370k ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/ListWorkflows_with_label_selector-12 8.410k ± ∞ ¹ 8.406k ± ∞ ¹ ~ (p=1.000 n=1) ² WorkflowArchive/CountWorkflows-12 212.0 ± ∞ ¹ 212.0 ± ∞ ¹ ~ (p=1.000 n=1) ³ geomean 2.462k 2.462k -0.03% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 ³ all samples are equal ``` Signed-off-by: Mason Malone <[email protected]>
I've mentioned having "performance regression tests" before (in #13566 (comment) and elsewhere) that would effectively check for, say, 10% or more drops in perf (note that some variability should be always expected as well, as can happen even with the same code on the same machine). Automated comments get missed pretty easily (see CodeCov comments that weren't used in the past) and are non-blocking, so seem insufficient as such. |
|
@agilgur5 That's a good idea, but I don't think that's feasible using the Github-hosted action runners due to noisy neighbors. We frequently see spikes in I/O latency during jobs, which is why I increased the timeout in #13769. Those spikes are likely to affect benchmarks and cause them to falsely report large performance regressions. There's not much we can do about noisy neighbors. If we had access to a self-hosted runner, then we'd have more options. |
|
We do have that problem, but afaik, most tests run within reasonable bounds and we rarely increase the timeouts, usually only due to adding things to the tests or CI jobs themselves (like the new freeing up disk space step as you mentioned in #13769). So if the frequency is low, I think it would be a worthwhile trade-off to have an occasional test flake that needs rerunning if we prevent performance regressions in doing so. Perhaps the worst case would be a test that only checks for less than 2x the average run time, which wouldn't catch smaller regressions, but would still catch the worst offenders |
|
A second problem I forgot to mention is ensuring the baseline results are generated by a runner with comparable performance characteristics to the runner used for the PR results. To be more concrete, imagine you implemented this by having a job run on commits to The problem is the runner used for Here's an article I found with some numbers: https://aakinshin.net/posts/github-actions-perf-stability/
|
|
I found this: https://github.com/cncf/cluster
That would be perfect for this. @agilgur5 should I go ahead and submit a request? |
|
Do you think the CNCF GitHub action runners would be consistent enough? https://contribute.cncf.io/resources/newsletters/2024-02-21-projectnewsletter/#github-action-runners |
|
@Joibel It could, but I don't know if https://github.com/argoproj/ is "part of the CNCF’s GitHub Enterprise account". Do you have access to https://github.com/argoproj/argo-workflows/settings/actions/runners? That should be the URL to manage self-hosted runners. It requires admin access to the repository. |
Not currently. Crenshaw (CD Lead) actually opened a Service Desk ticket to get access to them in late July: https://cncfservicedesk.atlassian.net/servicedesk/customer/portal/1/CNCFSD-2396 (the link is only accessible to maintainers). Summarizing, apparently CNCF hit some limits and then they were increased by GH and Argo was supposed to be added but still hasn't been, with Zach (Rollouts Lead) last asking for follow-up in mid-Sept. |
|
I've now followed up there as well with a gentle nag - partially so that I'll get notification if the status changes. |
Motivation
This adds basic benchmarks for listing and counting archived workflows so we can evaluate potential optimizations, e.g. #13601
Modifications
There weren't any other benchmarks in this repo that I could find, so these are the first. By default,
go testdoesn't run benchmarks unless you supply the-benchflag, so I added amake Benchmark%target for that.I had to make some adjustments to
test/e2e/fixtures/persistence.goso it exposedWorkflowArchivefor these tests.Due to stretchr/testify#811, we can't directly use
testifyto run these, so I used a workaround.Verification
Ran
make BenchmarkWorkflowArchivelocally after using #13715 to insert 100,000 rows intoargo_archived_workflows: