Add basic benchmarks #106
Add basic benchmarks #106
Conversation
add caching and rand benchmarks add fsutils benchmarks add logging, math, strings, templates and url benchmarks
|
Warning Rate limit exceeded@kashifkhan0771 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request adds numerous benchmark tests across multiple test files. New benchmark functions are introduced for Fibonacci calculations, context utilities, random data generation, file system operations, logging, mathematical computations, string manipulations, struct comparisons, template rendering, and URL processing. In addition to these benchmarks, the file system test module also includes improved error handling in the directory setup function. All changes are additive and focus on measuring performance by reporting memory allocations and iterating over test cases. Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
ctxutils/ctxutils_test.go (2)
9-9: Consider creating an issue to track the TODO.The TODO comment about converting tests to table tests is a good improvement suggestion. Would you like me to create an issue to track this task?
78-90: Optimize string value generation in benchmark.Using
fmt.Sprintfin a tight benchmark loop can affect the benchmark results by introducing additional allocations.Consider using a pre-generated slice of strings:
func BenchmarkSettingAndGettingStringKey(b *testing.B) { ctx := context.Background() + values := make([]string, b.N) + for i := 0; i < b.N; i++ { + values[i] = fmt.Sprintf("value-%d", i) + } b.ReportAllocs() b.ResetTimer() key := ContextKeyString{Key: "id"} for i := 0; i < b.N; i++ { - ctx = SetStringValue(ctx, key, fmt.Sprintf("value-%d", i)) + ctx = SetStringValue(ctx, key, values[i]) _, _ = GetStringValue(ctx, key) } }slice/slice_test.go (1)
77-85: Consider parameterizing the random number range.The random number range (0-999) is hardcoded. Consider making it configurable for more flexible testing scenarios.
-func generateRandomInts(n int) []int { +func generateRandomInts(n, maxVal int) []int { r := rand.New(rand.NewSource(99)) data := make([]int, n) for i := 0; i < n; i++ { - data[i] = r.Intn(1000) + data[i] = r.Intn(maxVal) } return data }fake/fake_test.go (1)
83-105: Add allocation reporting to benchmarks.For consistency with other benchmark functions in the codebase and to track memory usage, consider adding allocation reporting.
func BenchmarkGenerateUUID(b *testing.B) { + b.ReportAllocs() for i := 0; i < b.N; i++ { _, _ = RandomUUID() } } func BenchmarkRandomDate(b *testing.B) { + b.ReportAllocs() for i := 0; i < b.N; i++ { _, _ = RandomDate() } } func BenchmarkRandomPhoneNumber(b *testing.B) { + b.ReportAllocs() for i := 0; i < b.N; i++ { _, _ = RandomPhoneNumber() } } func BenchmarkRandomAddress(b *testing.B) { + b.ReportAllocs() for i := 0; i < b.N; i++ { _, _ = RandomAddress() } }templates/text_test.go (1)
178-190: Add memory allocation reporting and error handling.The benchmark should include memory allocation reporting and error handling for completeness.
Consider this improvement:
func BenchmarkRenderText(b *testing.B) { data := struct { Name string Date time.Time }{ Name: "alice", Date: time.Date(2024, 10, 1, 0, 0, 0, 0, time.UTC), } + b.ReportAllocs() + b.ResetTimer() for i := 0; i < b.N; i++ { - _, _ = RenderText(testTemplate1, data) + _, err := RenderText(testTemplate1, data) + if err != nil { + b.Fatal(err) + } } }templates/html_test.go (1)
215-232: Add error handling for template execution.While the benchmark is well-structured with proper memory allocation reporting, it should handle template execution errors.
Consider this improvement:
func BenchmarkRenderHTML(b *testing.B) { tmpl, _ := htmlTemplate.New("htmlTestTemplate").Funcs(GetCustomFuncMap()).Parse(normalizeWhitespace(htmlTestTemplate1)) data := struct { Name string Date time.Time }{ Name: "alice", Date: time.Date(2024, 10, 1, 0, 0, 0, 0, time.UTC), } b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { var sb strings.Builder - _ = tmpl.Execute(&sb, data) + if err := tmpl.Execute(&sb, data); err != nil { + b.Fatal(err) + } } }rand/rand_test.go (1)
354-366: Consider moving helper functions closer to their benchmarks.The helper functions
numberMathRandandnumberMathRandV2would be more maintainable if placed immediately before their respective benchmark functions.url/url_test.go (1)
414-447: Consider using more realistic URL test data.The current benchmarks use simple URLs that might not represent real-world complexity. Consider using more realistic examples:
func BenchmarkBuildURL(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - _, _ = BuildURL("http", "example.com", "onePath", map[string]string{"queryParamOne": "valueQueryParamOne"}) + _, _ = BuildURL("https", "api.example.com", "v1/users/profile", map[string]string{ + "userId": "12345", + "fields": "name,email,preferences", + "format": "json", + }) } }strings/strings_test.go (1)
573-585: Enhance string benchmarks with diverse test cases.Consider testing with a wider range of inputs to better represent different scenarios:
+var ( + prefixTestCases = [][]string{ + {"interstellar", "international", "interrupt"}, // Long common prefix + {"a", "b", "c"}, // No common prefix + {"prefix", "prefix", "prefix"}, // Identical strings + } + suffixTestCases = [][]string{ + {"running", "jumping", "sleeping"}, // Common suffix + {"test", "best", "quest"}, // Short common suffix + {"different", "words", "here"}, // No common suffix + } +) func BenchmarkCommonPrefix(b *testing.B) { b.ReportAllocs() + b.ResetTimer() for i := 0; i < b.N; i++ { - CommonPrefix("nation", "national", "nasty") + testCase := prefixTestCases[i%len(prefixTestCases)] + CommonPrefix(testCase...) } }fsutils/fsutils_test.go (3)
4-4: Consider usingmath/randinstead ofcrypto/rand.For benchmark data generation,
math/randwould be more efficient as cryptographic randomness isn't necessary here.- "crypto/rand" + "math/rand"
513-533: Consider adding sub-benchmarks with varied file counts.The benchmark could be more comprehensive by testing different scenarios:
- Different numbers of files (e.g., 10, 100, 1000)
- Mixed file extensions
- Deeper directory structures
func BenchmarkFindFiles(b *testing.B) { - tempDir, err := os.MkdirTemp("", "testdir") - if err != nil { - b.Fatal(err) - } - defer os.RemoveAll(tempDir) - - for i := 0; i < 100; i++ { - filePath := filepath.Join(tempDir, fmt.Sprintf("%d.txt", i)) - if err := os.WriteFile(filePath, []byte{}, 0644); err != nil { - b.Fatal(err) - } - } - - b.ReportAllocs() - b.ResetTimer() - - for i := 0; i < b.N; i++ { - _, _ = FindFiles(tempDir, ".txt") - } + fileCounts := []int{10, 100, 1000} + for _, count := range fileCounts { + b.Run(fmt.Sprintf("files_%d", count), func(b *testing.B) { + tempDir, err := os.MkdirTemp("", "testdir") + if err != nil { + b.Fatal(err) + } + defer os.RemoveAll(tempDir) + + for i := 0; i < count; i++ { + ext := ".txt" + if i%2 == 0 { + ext = ".log" + } + filePath := filepath.Join(tempDir, fmt.Sprintf("%d%s", i, ext)) + if err := os.WriteFile(filePath, []byte{}, 0644); err != nil { + b.Fatal(err) + } + } + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + _, _ = FindFiles(tempDir, ".txt") + } + }) + } }
560-588: Consider testing different file sizes.The benchmark uses a fixed file size of 1KB. Consider adding sub-benchmarks with different file sizes to better understand performance characteristics.
func BenchmarkFilesIdentical(b *testing.B) { - tempDir, err := os.MkdirTemp("", "testdir") - if err != nil { - b.Fatal(err) - } - defer os.RemoveAll(tempDir) - - file1 := filepath.Join(tempDir, "file1.txt") - file2 := filepath.Join(tempDir, "file2.txt") - - data := make([]byte, 1024) - if _, err := rand.Read(data); err != nil { - b.Fatal(err) - } - - if err := os.WriteFile(file1, data, 0644); err != nil { - b.Fatal(err) - } - if err := os.WriteFile(file2, data, 0644); err != nil { - b.Fatal(err) - } - - b.ReportAllocs() - b.ResetTimer() - - for i := 0; i < b.N; i++ { - _, _ = FilesIdentical(file1, file2) - } + sizes := []int{1024, 1024 * 1024, 10 * 1024 * 1024} // 1KB, 1MB, 10MB + for _, size := range sizes { + b.Run(fmt.Sprintf("size_%dB", size), func(b *testing.B) { + tempDir, err := os.MkdirTemp("", "testdir") + if err != nil { + b.Fatal(err) + } + defer os.RemoveAll(tempDir) + + file1 := filepath.Join(tempDir, "file1.txt") + file2 := filepath.Join(tempDir, "file2.txt") + + data := make([]byte, size) + if err := rand.Read(data); err != nil { + b.Fatal(err) + } + + if err := os.WriteFile(file1, data, 0644); err != nil { + b.Fatal(err) + } + if err := os.WriteFile(file2, data, 0644); err != nil { + b.Fatal(err) + } + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + _, _ = FilesIdentical(file1, file2) + } + }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
caching/caching_test.go(1 hunks)ctxutils/ctxutils_test.go(2 hunks)fake/fake_test.go(1 hunks)fsutils/fsutils_test.go(4 hunks)logging/logging_test.go(1 hunks)math/math_test.go(1 hunks)rand/rand_test.go(2 hunks)slice/slice_test.go(2 hunks)strings/strings_test.go(1 hunks)structs/structs_test.go(1 hunks)templates/html_test.go(1 hunks)templates/text_test.go(1 hunks)url/url_test.go(1 hunks)
🔇 Additional comments (8)
ctxutils/ctxutils_test.go (1)
92-104: LGTM!The benchmark for integer keys is well-implemented with proper allocation reporting and timer reset.
slice/slice_test.go (2)
62-75: LGTM!The string generation helper is well-implemented:
- Uses
strings.Builderfor efficient string concatenation- Generates varying length strings for better test coverage
87-107: LGTM!The benchmarks are well-implemented with:
- Large dataset size for meaningful results
- Proper allocation reporting and timer reset
caching/caching_test.go (1)
139-169: Well-structured benchmarks with good coverage of caching strategies!The benchmarks effectively test different caching implementations:
- Basic caching with CacheWrapper
- Thread-safe caching with SafeCacheWrapper
- Concurrent performance with RunParallel
structs/structs_test.go (1)
101-137: Well-implemented benchmarks for struct comparisons!The benchmarks effectively cover both simple and complex struct comparisons with proper error handling and memory allocation reporting.
rand/rand_test.go (1)
296-300: Well-organized benchmarks with clear performance context!Good job on clearly separating the benchmarks section and documenting the performance trade-offs between crypto/rand and math/rand packages.
fsutils/fsutils_test.go (2)
355-396: Well-structured error handling improvements!Great improvements to the
setupNestedDirsfunction:
- Proper error wrapping with
fmt.Errorf- Clean resource management with deferred cleanup
- Clear file structure definition using a map
502-504: LGTM! Clear section separation.The benchmark section is well-organized with clear separation from test functions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kashif Khan <70996046+kashifkhan0771@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kashif Khan <70996046+kashifkhan0771@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kashif Khan <70996046+kashifkhan0771@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kashif Khan <70996046+kashifkhan0771@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
caching/caching_test.go (3)
135-141: Improve benchmark coverage with varied inputs.The benchmark uses a fixed input of 30, which might not represent real-world usage patterns.
Consider using a range of inputs:
func BenchmarkFib(b *testing.B) { + inputs := []int{10, 20, 30, 40} b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - _ = fib(30) + _ = fib(inputs[i%len(inputs)]) } }
143-151: Improve benchmark setup for cached function.Two suggestions:
- Use varied inputs like in
BenchmarkFib.- Move cache creation outside the benchmark to avoid measuring cache initialization time.
Consider this implementation:
+var cachedFib = CacheWrapper(fib) + func BenchmarkCachedFib(b *testing.B) { - cachedFib := CacheWrapper(fib) + inputs := []int{10, 20, 30, 40} b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - _ = cachedFib(30) + _ = cachedFib(inputs[i%len(inputs)]) } }
163-173: Good use of b.RunParallel, but same improvements needed as other benchmarks.The concurrent benchmark correctly uses
b.RunParallel, but could be improved by:
- Moving cache creation outside the benchmark.
- Using varied inputs.
Consider this implementation:
+var safeCachedFib = SafeCacheWrapper(fib) + func BenchmarkConcurrentSafeCachedFib(b *testing.B) { - cachedFib := SafeCacheWrapper(fib) + inputs := []int{10, 20, 30, 40} b.ReportAllocs() b.ResetTimer() b.RunParallel(func(pb *testing.PB) { + i := 0 for pb.Next() { - _ = cachedFib(30) + _ = safeCachedFib(inputs[i%len(inputs)]) + i++ } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
caching/caching_test.go(1 hunks)logging/logging_test.go(1 hunks)math/math_test.go(1 hunks)rand/rand_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- logging/logging_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
rand/rand_test.go
357-357: File is not goimports-ed
(goimports)
caching/caching_test.go
125-125: File is not goimports-ed
(goimports)
🪛 GitHub Actions: golangci-lint
rand/rand_test.go
[warning] 357-357: File is not gofmt-ed with -s.
[warning] 366-366: File is not goimports-ed.
caching/caching_test.go
[warning] 125-125: File is not gofmt-ed with -s.
🔇 Additional comments (4)
caching/caching_test.go (2)
124-133: LGTM! Efficient iterative implementation.The iterative implementation is efficient with O(n) time complexity, which is much better than a recursive implementation.
🧰 Tools
🪛 golangci-lint (1.62.2)
125-125: File is not
goimports-ed(goimports)
🪛 GitHub Actions: golangci-lint
[warning] 125-125: File is not
gofmt-ed with-s.
153-161: Same improvements needed as in BenchmarkCachedFib.Move cache creation outside the benchmark and use varied inputs.
rand/rand_test.go (1)
354-393: LGTM! Efficient benchmark implementation.The implementation follows best practices:
- Array creation is moved outside benchmarks using
benchArray.- Array is reused across benchmarks using
copy.- Memory allocations are properly reported.
🧰 Tools
🪛 golangci-lint (1.62.2)
357-357: File is not
goimports-ed(goimports)
🪛 GitHub Actions: golangci-lint
[warning] 357-357: File is not
gofmt-ed with-s.
[warning] 366-366: File is not
goimports-ed.math/math_test.go (1)
628-664: LGTM! Well-structured benchmarks with appropriate input ranges.The implementation follows best practices:
- IntPow benchmark uses predefined bases and exponents to avoid overflow.
- Factorial benchmark uses appropriate range (0-19) to avoid overflow.
- GCD and LCM benchmarks use consecutive integers for good coverage.
- Memory allocations are properly reported.
Signed-off-by: Kashif Khan <70996046+kashifkhan0771@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
caching/caching_test.go (3)
143-151: Consider testing with varied inputs to demonstrate cache effectiveness.While the current benchmark is well-structured, testing with a single input (30) may not fully demonstrate the benefits of caching. Consider adding benchmarks with different inputs to show how the cache improves performance when the same values are requested multiple times.
Example improvement:
func BenchmarkCachedFib(b *testing.B) { cachedFib := CacheWrapper(fib) + + // Pre-warm cache with some values + inputs := []int{10, 20, 30} + for _, n := range inputs { + _ = cachedFib(n) + } b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - _ = cachedFib(30) + _ = cachedFib(inputs[i%len(inputs)]) } }
153-161: Add comparative benchmarks to measure thread-safety overhead.Consider adding sub-benchmarks to compare performance with the non-thread-safe version:
- Using varied inputs like the previous suggestion
- Measuring the overhead of thread-safety mechanisms
Example improvement:
func BenchmarkSafeCachedFib(b *testing.B) { - cachedFib := SafeCacheWrapper(fib) + b.Run("SingleValue", func(b *testing.B) { + cachedFib := SafeCacheWrapper(fib) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = cachedFib(30) + } + }) - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - _ = cachedFib(30) - } + b.Run("MultipleValues", func(b *testing.B) { + cachedFib := SafeCacheWrapper(fib) + inputs := []int{10, 20, 30} + for _, n := range inputs { + _ = cachedFib(n) + } + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = cachedFib(inputs[i%len(inputs)]) + } + }) }
163-173: Document contention patterns and add varied concurrent scenarios.The parallel benchmark is well-structured but could be enhanced by:
- Testing with varied inputs to simulate real-world cache access patterns
- Documenting expected contention patterns in comments
Example improvement:
func BenchmarkConcurrentSafeCachedFib(b *testing.B) { cachedFib := SafeCacheWrapper(fib) + + // Pre-warm cache with some values to simulate mixed hit/miss scenarios + inputs := []int{10, 20, 30, 40} + for _, n := range inputs { + _ = cachedFib(n) + } + + // Simulate real-world scenarios: + // 1. High contention: All goroutines accessing same value + // 2. Mixed contention: Goroutines accessing different values + scenarios := []struct { + name string + inputs []int + }{ + {"HighContention", []int{30}}, + {"MixedContention", inputs}, + } - b.ReportAllocs() - b.ResetTimer() - b.RunParallel(func(pb *testing.PB) { - for pb.Next() { - _ = cachedFib(30) - } - }) + for _, sc := range scenarios { + b.Run(sc.name, func(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + b.RunParallel(func(pb *testing.PB) { + i := 0 + for pb.Next() { + _ = cachedFib(sc.inputs[i%len(sc.inputs)]) + i++ + } + }) + }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
caching/caching_test.go(1 hunks)
🔇 Additional comments (2)
caching/caching_test.go (2)
124-133: LGTM! Efficient iterative Fibonacci implementation.The implementation uses an optimal iterative approach with O(n) time complexity, avoiding the exponential complexity of a recursive solution.
135-141: LGTM! Well-structured benchmark.The benchmark follows Go's best practices by:
- Reporting memory allocations
- Resetting the timer to exclude setup time
- Using a reasonable input size
Signed-off-by: Kashif Khan <70996046+kashifkhan0771@users.noreply.github.com>
|
@coderabbitai review |
|
Good work guys. |
Description:
Not all functions needed benchmarks; I've refrained from adding any for wrappers around the standard library, such as boolean, maps, and pointers, as they would mostly measure the overhead of calling Go's standard library functions.
Checklist:
make test.make lint.If added a new utility function or updated existing function logic:
Summary by CodeRabbit