DEV2: Implementation of multiple goroutines for concurrent flushing#15
Merged
maoueh merged 35 commits intofirehose-fh3.0from Jun 3, 2025
Merged
DEV2: Implementation of multiple goroutines for concurrent flushing#15maoueh merged 35 commits intofirehose-fh3.0from
maoueh merged 35 commits intofirehose-fh3.0from
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces configurable concurrent flushing of firehose blocks via multiple goroutines, updates tests to support this new configuration, and includes benchmarking reports.
- Add
ConcurrentBlockFlushingconfig and implement worker queues for asynchronous block serialization and ordered output - Update tests to run in sequential and concurrent modes and add new concurrency-specific tests
- Include performance benchmarking report in
firehose_concurrency_2.md
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| eth/tracers/internal/tracetest/firehose/helpers_test.go | Update newFirehoseTestTracer signature to accept FirehoseConfig |
| eth/tracers/internal/tracetest/firehose/firehose_test.go | Add loops for sequential and concurrent test runs and pass new config |
| eth/tracers/firehose_concurrency_test.go | New unit tests validating concurrent flushing behavior |
| eth/tracers/firehose_concurrency_2.md | Benchmarking report for concurrent block flushing |
| eth/tracers/firehose.go | Implement core concurrency logic, added config field, worker queues, and clean shutdown |
Comments suppressed due to low confidence (2)
eth/tracers/firehose.go:154
- [nitpick] The field name
closeChannelsis ambiguous for async.Once; consider renaming it tocloseOnceor similar to better reflect its purpose.
closeChannels sync.Once
eth/tracers/internal/tracetest/firehose/firehose_test.go:43
- Tests only cover
ConcurrentBlockFlushingvalues 0 and 1; add cases with higher values (e.g., >1) to verify behavior under multiple worker goroutines.
for _, concurrent := range []int{0, 1} {
…f goroutine. All tests passing
# Conflicts: # eth/tracers/firehose.go # eth/tracers/internal/tracetest/firehose/firehose_test.go
Rampex1
force-pushed
the
david-zhou/worker-queue-2
branch
from
f1ff669 to
8eb3d90
Compare
Rampex1
changed the title
Rampex1
force-pushed
the
david-zhou/worker-queue-2
branch
from
12070e8 to
b105796
Compare
Author
|
Ready for review |
maoueh
requested changes
May 22, 2025
maoueh
requested changes
May 23, 2025
Author
|
One last change I made which isn't showing on Github for some reasonn (Github is saying "prcessing update") |
maoueh
approved these changes
Jun 3, 2025
maoueh
reviewed
Jun 3, 2025
| // if it's a network for which we must reproduce the legacy bugs. | ||
| applyBackwardCompatibility *bool | ||
| concurrentBlockFlushing bool | ||
| concurrentBlockFlushing int |
There was a problem hiding this comment.
Let's remove and use config block flushing count directly everywehre.
maoueh
reviewed
Jun 3, 2025
| } | ||
|
|
||
| type ConcurrentFlushQueue struct { | ||
| bufferSize int |
maoueh
reviewed
Jun 3, 2025
Comment on lines
+32
to
+36
| func NewConcurrentFlushQueue(bufferSize int, printBlockFunc func(*pbeth.Block, *FinalityStatus), outputFunc func([]byte)) *ConcurrentFlushQueue { | ||
| return &ConcurrentFlushQueue{ | ||
| startSignal: make(chan uint64, 1), | ||
| jobQueue: make(chan *blockPrintJob, bufferSize), | ||
| outputQueue: make(chan *outputJob, bufferSize), |
There was a problem hiding this comment.
Buffer size should be the same as number of worker taken in start, might be good to merge Start input with this one, which would mean that we keep the bufferSize struct variable. Maybe with another name though.
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
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.
Changes Introduced
Implemention of multiple goroutines for concurrent flushing
Benchmarking report
Note