feat: Resolve table relations in parallel#416
Merged
kodiakhq[bot] merged 2 commits intomainfrom Nov 21, 2022
Merged
Conversation
Contributor
|
@hermanschaaf Really awesome tests! Im still reviewing and playing with it as quite a lot to intake but posting some thoughts in the meanwhile. Maybe add golang.org/x/perf/cmd/benchstat to also compare those in CI and catch if anything change the performance significantly as otherwise we would forget to run it |
Contributor
comment on the wrong thread :) |
hermanschaaf
added a commit
that referenced
this pull request
Nov 22, 2022
This reverts commit aadbde9.
kodiakhq bot
pushed a commit
that referenced
this pull request
Nov 23, 2022
This changes the resolver for table relations to work concurrently, like parent tables. To do so safely and without the risk of deadlock, we instantiate one semaphore per depth level. (The size of the semaphore is decreased logarithmically for each depth.) This change will only improve performance for tables with child relations. To compare the performance before and after, I used the benchmarks in #415 ## Before ``` goos: darwin goarch: arm64 pkg: github.com/cloudquery/plugin-sdk/plugins BenchmarkDefaultConcurrency-8 1 11957 resources/s 12626 targetResources/s 73597280 B/op 1032425 allocs/op BenchmarkTablesWithChildrenDefaultConcurrency-8 1 545.9 resources/s 40606 targetResources/s 461622584 B/op 6690105 allocs/op PASS ok github.com/cloudquery/plugin-sdk/plugins 150.596s ``` ## After ``` goos: darwin goarch: arm64 pkg: github.com/cloudquery/plugin-sdk/plugins BenchmarkDefaultConcurrency-8 1 11373 resources/s 12626 targetResources/s 73692608 B/op 1033614 allocs/op BenchmarkTablesWithChildrenDefaultConcurrency-8 1 30162 resources/s 40606 targetResources/s 464285672 B/op 6697508 allocs/op PASS ok github.com/cloudquery/plugin-sdk/plugins 6.241s ``` ## Analysis This change focuses on the `BenchmarkTablesWithChildrenDefaultConcurrency` case. The change improves `resources/s` from 545.9 to 30162, an improvement of about 55x. Memory and allocs are mostly the same. The small downward change in `resources/s` in the `BenchmarkDefaultConcurrency` case is likely due to the few additional allocs needed. Closes #358 Copy of #416 after it was reverted to schedule its release for another time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes the resolver for table relations to work concurrently, like parent tables. To do so safely and without the risk of deadlock, we instantiate one semaphore per depth level. (The size of the semaphore is decreased logarithmically for each depth.)
This change will only improve performance for tables with child relations.
To compare the performance before and after, I used the benchmarks in #415
Before
After
Analysis
This change focuses on the
BenchmarkTablesWithChildrenDefaultConcurrencycase. The change improvesresources/sfrom 545.9 to 30162, an improvement of about 55x. Memory and allocs are mostly the same.The small downward change in
resources/sin theBenchmarkDefaultConcurrencycase is likely due to the few additional allocs needed.Closes #358