-
Notifications
You must be signed in to change notification settings - Fork 229
perf(block): use sync/atomic
instead of mutexes
#2735
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
Conversation
Summary of ChangesHello @julienrbrt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a performance enhancement by refactoring critical state management within the block execution and submission components. It replaces traditional mutex-based synchronization with Go's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
Code Review: Performance Optimization using
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a performance optimization that replaces sync.RWMutex
with sync/atomic
for managing state in Executor
and Submitter
. The changes are well-implemented and correctly use atomic operations for uint64
and struct pointers. I have one suggestion for Executor.GetLastState
to improve thread safety by preventing potential data races that could occur if callers modify the returned state object. This is a pre-existing condition, but worth addressing during this refactoring.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2735 +/- ##
==========================================
- Coverage 62.28% 62.17% -0.12%
==========================================
Files 79 79
Lines 8490 8497 +7
==========================================
- Hits 5288 5283 -5
- Misses 2711 2721 +10
- Partials 491 493 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review - PR #2735: Use
|
PR Review: perf(block): use
|
Code Review: Performance Optimization with sync/atomicSummaryThis PR replaces mutex-based synchronization with ✅ Strengths
|
Code Review: Performance Optimization with
|
Code Review: Performance Optimization with sync/atomicSummaryThis PR replaces mutex-based synchronization with 🚨 Critical Issues1. Data Race in
|
Code Review: perf(block): use
|
benchmark.go// BenchmarkGetLastState measures the performance of reading state
func BenchmarkGetLastState(b *testing.B) {
state := types.State{
Version: types.Version{Block: 1, App: 1},
LastBlockHeight: 100,
AppHash: []byte("apphash1234567890"),
LastResultsHash: []byte("results1234567890"),
}
e := &Executor{
lastState: &atomic.Pointer[types.State]{},
}
e.lastState.Store(&state)
b.ResetTimer()
b.RunParallel(func(pb *testing.PB) {
for pb.Next() {
_ = e.GetLastState()
}
})
}
// BenchmarkStateReadWrite_Mixed benchmarks mixed read/write workload (90% reads, 10% writes)
func BenchmarkStateReadWrite_Mixed(b *testing.B) {
state := types.State{
Version: types.Version{Block: 1, App: 1},
LastBlockHeight: 100,
AppHash: []byte("apphash1234567890"),
LastResultsHash: []byte("results1234567890"),
}
e := &Executor{
lastState: &atomic.Pointer[types.State]{},
}
e.lastState.Store(&state)
b.ResetTimer()
b.RunParallel(func(pb *testing.PB) {
i := 0
for pb.Next() {
if i%10 == 0 {
// 10% writes
newState := state
newState.LastBlockHeight = uint64(i)
e.SetLastState(newState)
} else {
// 90% reads
_ = e.GetLastState()
}
i++
}
})
} // BenchmarkGetLastState_WithMutex compares against mutex-based implementation
func BenchmarkGetLastState_WithMutex(b *testing.B) {
state := types.State{
Version: types.Version{Block: 1, App: 1},
LastBlockHeight: 100,
AppHash: []byte("apphash1234567890"),
LastResultsHash: []byte("results1234567890"),
}
type mutexExecutor struct {
lastState types.State
lastStateMtx sync.RWMutex
}
e := &mutexExecutor{lastState: state}
b.ResetTimer()
b.RunParallel(func(pb *testing.PB) {
for pb.Next() {
e.lastStateMtx.RLock()
s := e.lastState
e.lastStateMtx.RUnlock()
_ = s
}
})
}
// BenchmarkStateReadWrite_Mixed_WithMutex compares mixed workload against mutex
func BenchmarkStateReadWrite_Mixed_WithMutex(b *testing.B) {
state := types.State{
Version: types.Version{Block: 1, App: 1},
LastBlockHeight: 100,
AppHash: []byte("apphash1234567890"),
LastResultsHash: []byte("results1234567890"),
}
type mutexExecutor struct {
lastState types.State
lastStateMtx sync.RWMutex
}
e := &mutexExecutor{lastState: state}
b.ResetTimer()
b.RunParallel(func(pb *testing.PB) {
i := 0
for pb.Next() {
if i%10 == 0 {
// 10% writes
e.lastStateMtx.Lock()
e.lastState.LastBlockHeight = uint64(i)
e.lastStateMtx.Unlock()
} else {
// 90% reads
e.lastStateMtx.RLock()
_ = e.lastState
e.lastStateMtx.RUnlock()
}
i++
}
})
} goos: linux
goarch: arm64
pkg: github.com/evstack/ev-node/block/internal/executing
BenchmarkGetLastState-12 71430471 18.99 ns/op 48 B/op 2 allocs/op
PASS
ok github.com/evstack/ev-node/block/internal/executing 1.384s
goos: linux
goarch: arm64
pkg: github.com/evstack/ev-node/block/internal/executing
BenchmarkStateReadWrite_Mixed-12 40779867 25.97 ns/op 56 B/op 1 allocs/op
PASS
ok github.com/evstack/ev-node/block/internal/executing 1.109s
goos: linux
goarch: arm64
pkg: github.com/evstack/ev-node/block/internal/executing
BenchmarkGetLastState_WithMutex-12 8119104 150.4 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/evstack/ev-node/block/internal/executing 1.381s
goos: linux
goarch: arm64
pkg: github.com/evstack/ev-node/block/internal/executing
BenchmarkStateReadWrite_Mixed_WithMutex-12 49345582 79.34 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/evstack/ev-node/block/internal/executing 3.952s Allocations are due to the deep copy, suggested by Claude. I personally think we can remove it, we do not mutate current state. We create a new one each block with Removing it gives as well obviously an even better result: goos: linux
goarch: arm64
pkg: github.com/evstack/ev-node/block/internal/executing
BenchmarkGetLastState-12 1000000000 0.05766 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/evstack/ev-node/block/internal/executing 0.078s
goos: linux
goarch: arm64
pkg: github.com/evstack/ev-node/block/internal/executing
BenchmarkStateReadWrite_Mixed-12 130789657 9.246 ns/op 12 B/op 0 allocs/op
PASS
ok github.com/evstack/ev-node/block/internal/executing 2.150s |
Thanks for adding some benchmark results! There is a significant difference without the deep copy. When I looked at the code, there is no mutation indeed but with public methods this is not guaranteed to be true in the future. The |
Code Review: Performance Optimization with sync/atomicThank you for this performance optimization PR! The migration from mutexes to ✅ Strengths
🔴 Critical Issues1. Data Race in GetLastState() - BLOCKINGLocation: The current implementation has a critical data race: func (e *Executor) GetLastState() types.State {
state := e.lastState.Load()
if state == nil {
return types.State{}
}
stateCopy := *state // Race condition here!
stateCopy.AppHash = bytes.Clone(state.AppHash)
stateCopy.LastResultsHash = bytes.Clone(state.LastResultsHash)
return stateCopy
} Problem: While the pointer load is atomic, dereferencing ( Why bytes.Clone doesn't help: Cloning happens after the struct copy, so the race already occurred when reading the slice pointers from the original struct. Solution: Store the entire state including cloned slices: func (e *Executor) SetLastState(state types.State) {
stateCopy := types.State{
Version: state.Version,
ChainID: state.ChainID,
InitialHeight: state.InitialHeight,
LastBlockHeight: state.LastBlockHeight,
LastBlockTime: state.LastBlockTime,
DAHeight: state.DAHeight,
AppHash: bytes.Clone(state.AppHash),
LastResultsHash: bytes.Clone(state.LastResultsHash),
}
e.lastState.Store(&stateCopy)
}
func (e *Executor) GetLastState() types.State {
state := e.lastState.Load()
if state == nil {
return types.State{}
}
return *state // Safe now - stored state already has cloned slices
} This ensures the stored pointer always points to an immutable, fully independent copy. 2. Missing time.Time handling
|
Code Review: PR #2735 - Use
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪 Well done
Overview
Use sync/atomic` instead of mutexes.