chore: Add source plugin benchmark#415
Merged
kodiakhq[bot] merged 7 commits intomainfrom Nov 21, 2022
Merged
Conversation
Member
Same |
Contributor
Author
|
@disq That's expected - it's a theoretical maximum we calculate, so you have something to compare the |
disq
reviewed
Nov 18, 2022
disq
reviewed
Nov 18, 2022
Member
Yes interesting calculation 👀 |
disq
approved these changes
Nov 18, 2022
This was referenced Nov 18, 2022
disq
reviewed
Nov 21, 2022
| s.b.Fatal(err) | ||
| } | ||
|
|
||
| end := time.Now() |
Member
There was a problem hiding this comment.
diff := time.Since(start) (there's no call to s.b.StopTimer() now?)
yevgenypats
approved these changes
Nov 21, 2022
Contributor
yevgenypats
left a comment
There was a problem hiding this comment.
Looks great! we can add benchtest in follow-up PR.
kodiakhq bot
pushed a commit
that referenced
this pull request
Nov 21, 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
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
3 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 adds a source plugin benchmark that runs in-memory. It allows for different scenarios to be set up, but currently comes with only two: one scenario without child tables, and one with child tables.
Here are the results on my machine:
The output is different from normal Go benchmarks, and I feel it better fits our use-case. It shows the actual
resources/s, similar to what the CLI would show. It also shows atargetResources/s, an attempt to calculate the theoretical best value we could aim for.As the above results show, we are doing pretty well in the case with no child relations, but we are leaving a lot of room for improvement when child relations are involved. In theory, these could be sped up by almost 100x (this might use a lot of memory though, of course). In both cases it seems like we could also be doing better in terms of number of allocs and B/op, but that is pending further research.
Closes cloudquery/cloudquery#3721