[coordinator] Productization of the simple dependency manager#65
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR productionizes the simple dependency manager with significant improvements to performance, metrics, and testing. The changes focus on optimizing the simple dependency manager through various enhancements including TX limit support, comprehensive metrics integration, and performance optimizations.
- Adds waiting TX limit support and comprehensive metrics to the simple dependency manager
- Optimizes the
constructCompositeKey()function for better performance with pre-allocation and efficient encoding - Replaces the
Configstruct withParametersand removes theDummySignerin favor of nil checks for better performance
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/signature/sigtest/signer.go | Removes DummySigner struct and optimizes signature handling with nil checks |
| service/coordinator/dependencygraph/simple.go | Major refactoring with waiting TX limits, metrics integration, and performance improvements |
| service/coordinator/dependencygraph/transaction_node.go | Optimizes constructCompositeKey function with better memory allocation and encoding |
| service/coordinator/dependencygraph/parameters.go | New file defining Parameters struct to replace Config |
| service/coordinator/dependencygraph/manager_test.go | Extensive test additions and benchmark improvements |
| service/coordinator/dependencygraph/manager.go | Updates to use Parameters instead of Config |
| Multiple config files | Updates default waiting TX limits and removes deprecated configuration options |
860d5de to
3c31349
Compare
cendhu
left a comment
There was a problem hiding this comment.
LGTM. One small test needs to be added for waiting tx limit.
| import "github.com/hyperledger/fabric-x-committer/utils/monitoring" | ||
|
|
||
| // Parameters holds the configuration for the dependency graph manager. | ||
| type Parameters struct { |
There was a problem hiding this comment.
why are we renaming it to Parameters from Config. I think, we have consistently used Config everywhere.
There was a problem hiding this comment.
We use config for configurations (user-facing configurations with map-structure annotations). And parameters for function parameters. This struct is only used internally as parameters to the dependency graph, so it should be named as such.
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
3c31349 to
5635d02
Compare
Type of change
Description
Simple manager:
constructCompositeKey()Performance
Benchmarking was done on an Apple M1 machine.
When dependency was introduced, it was with 30% dependent TXs.
The following tests are conducted with 2, 4, or 6 independent read-write keys, and dependent keys are added accordingly.
All combinations of dependencies were tested. However, all conflicts with write had the same performance as write-write conflicts.
Benchmark on i9-14900K

Related issues