Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fileservice: add write benchmark #22670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
reusee wants to merge 1 commit into matrixorigin:main
base: main
Choose a base branch
Loading
from reusee:benchfswrite

Conversation

@reusee
Copy link
Contributor

@reusee reusee commented Oct 24, 2025
edited by qodo-merge-pro bot
Loading

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue https://github.com/matrixorigin/MO-cloud/issues/6517

What this PR does / why we need it:

add benchmark for write


PR Type

Enhancement, Tests


Description

  • Add comprehensive write benchmarks for FileService with multiple file sizes

  • Implement parallel write benchmark testing with 1K, 4K, and 128K file sizes

  • Define size constants (KB, MB, GB, TB, PB) for reusable benchmark configurations

  • Update S3FS benchmark test with missing ObjectStorageArguments fields


Diagram Walkthrough

flowchart LR
 A["benchmarkFileService"] -- "adds write sub-benchmarks" --> B["benchmarkWrite function"]
 B -- "tests different file sizes" --> C["1K, 4K, 128K variants"]
 D["utils.go"] -- "defines size constants" --> B
 E["s3_fs_test.go"] -- "updates with missing fields" --> F["ObjectStorageArguments"]
Loading

File Walkthrough

Relevant files
Tests
file_service_bench_test.go
Add parallel write benchmarks with multiple file sizes

pkg/fileservice/file_service_bench_test.go

  • Add fmt and sync/atomic imports for benchmark implementation
  • Remove redundant context creation in parallel read benchmark
  • Implement new benchmarkWrite function with parallel write testing
  • Add nested write benchmarks for 1K, 4K, and 128K file sizes
  • Use atomic counter for unique file path generation during parallel
    writes
+50/-1
Enhancement
utils.go
Add reusable size constants for benchmarks

pkg/fileservice/utils.go

  • Define size constants KB, MB, GB, TB, PB for benchmark and general use
  • Use bit shift operations for efficient size calculations
+8/-0
Bug fix
s3_fs_test.go
Complete ObjectStorageArguments with missing fields

pkg/fileservice/s3_fs_test.go

  • Add missing IsMinio field set to false in ObjectStorageArguments
  • Add missing Region field set to empty string
  • Add missing KeyID and KeySecret fields set to empty strings
+4/-0

@reusee reusee requested a review from fengttt as a code owner October 24, 2025 02:29
Copy link

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->
</details></td></tr>
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion Impact
High-level
Benchmark design may not reflect real-world usage

The current benchmark only tests creating and writing new files. It should be
expanded to include other common scenarios, like overwriting files, to provide a
more realistic performance evaluation.

Examples:

pkg/fileservice/file_service_bench_test.go [94-128]
func benchmarkWrite(ctx context.Context, b *testing.B, fileSize int64, newFS func() FileService) {
	data := make([]byte, fileSize)
	for i := range data {
		data[i] = byte(i % 256)
	}
	b.ResetTimer()
	b.SetBytes(fileSize)
	fs := newFS()
 ... (clipped 25 lines)

Solution Walkthrough:

Before:

func benchmarkWrite(ctx context.Context, b *testing.B, fileSize int64, newFS func() FileService) {
 // ...
 var n atomic.Int64
 b.RunParallel(func(pb *testing.PB) {
 for pb.Next() {
 // Each operation creates a new, unique file.
 filePath := fmt.Sprintf("benchmark_write_%d_%d_%d", fileSize, b.N, n.Add(1))
 vector := IOVector{
 FilePath: filePath,
 Entries: []IOEntry{{Data: data, Size: fileSize}},
 }
 err := fs.Write(ctx, vector)
 // ...
 }
 })
}

After:

func benchmarkWrite(ctx context.Context, b *testing.B, fileSize int64, newFS func() FileService) {
 // ...
 // Scenario 1: Create new files (as before)
 b.Run("create new", func(b *testing.B) {
 // ... existing parallel write logic ...
 })
 // Scenario 2: Overwrite existing files
 b.Run("overwrite", func(b *testing.B) {
 // Pre-create a pool of files
 // In parallel, repeatedly write to files from the pool
 // This may require a Delete-then-Write pattern
 })
}
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the benchmark only tests new file creation, which is a valid but narrow scenario, and proposes expanding it to better reflect diverse real-world write patterns.

Low
Possible issue
Clean up benchmark-generated files

Add cleanup logic to delete files created during the benchmark test. This
involves collecting file paths during the parallel execution and deleting them
in a batch after the timer is stopped to prevent resource exhaustion.

pkg/fileservice/file_service_bench_test.go [106-127]

 var n atomic.Int64
+var filePaths []string
+var filePathsMutex sync.Mutex
+
 b.RunParallel(func(pb *testing.PB) {
 	for pb.Next() {
-		filePath := fmt.Sprintf("benchmark_write_%d_%d_%d", fileSize, b.N, n.Add(1))
+		filePath := fmt.Sprintf("benchmark_write_%d_%d", fileSize, n.Add(1))
 
 		vector := IOVector{
 			FilePath: filePath,
 			Entries: []IOEntry{
 				{
 					Size: fileSize,
 					Data: data,
 				},
 			},
 		}
 
 		err := fs.Write(ctx, vector)
 		if err != nil {
 			b.Fatal(err)
 		}
 
+		filePathsMutex.Lock()
+		filePaths = append(filePaths, filePath)
+		filePathsMutex.Unlock()
 	}
 })
 
+b.StopTimer()
+err := fs.Delete(ctx, filePaths...)
+if err != nil {
+	b.Logf("failed to delete files: %v", err)
+}
+
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that benchmark-generated files are not cleaned up, and proposes a robust solution using fs.Delete to prevent potential resource exhaustion during long test runs.

Low
General
Simplify unique file path generation

Simplify the benchmark's file path generation by removing the redundant b.N
component. The atomic counter n already ensures file path uniqueness.

pkg/fileservice/file_service_bench_test.go [109]

-filePath := fmt.Sprintf("benchmark_write_%d_%d_%d", fileSize, b.N, n.Add(1))
+filePath := fmt.Sprintf("benchmark_write_%d_%d", fileSize, n.Add(1))
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly points out that b.N is redundant for filename uniqueness, and removing it simplifies the code with a minor improvement to readability.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fengttt fengttt Awaiting requested review from fengttt fengttt is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

kind/enhancement Review effort 2/5 size/S Denotes a PR that changes [10,99] lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /