[Consensus] Own sized leveled forest tech depth #1046
Conversation
…/5670-own-sized-levelled-forest
… Added missing locks for execution_tree
| et.RLock() | ||
| defer et.RUnlock() | ||
| return et.size | ||
| } | ||
|
|
||
| // LowestHeight returns the lowest height, where results are still stored in the mempool. | ||
| func (et *ExecutionTree) LowestHeight() uint64 { | ||
| et.RLock() | ||
| defer et.RUnlock() |
There was a problem hiding this comment.
Nice work!
I'd like to request to add a test cases that verifies the GetSize method is concurrent-safe:
Verify that adding 100 assignment collectors to the tree concurrently and in the end calling GetSize should return 100.
Turning on race flag should not report race condition.
…/5670-own-sized-levelled-forest
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
….com/onflow/flow-go into yurii/5670-own-sized-levelled-forest
@zhangchiqing We currently don't have separate tests for assignment collector tree but I have an issue for it that I will be implementing soon. I would implement that test as part of it if you are ok with it. flow-go/issues/5719 |
Codecov Report
@@ Coverage Diff @@
## master #1046 +/- ##
==========================================
+ Coverage 53.16% 53.18% +0.02%
==========================================
Files 321 321
Lines 21734 21741 +7
==========================================
+ Hits 11554 11563 +9
+ Misses 8610 8609 -1
+ Partials 1570 1569 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| vertexId := strconv.FormatUint(1, 10) | ||
| parentId := strconv.FormatUint(parentLevel, 10) | ||
| for i := uint64(1); i <= numberOfNodes; i++ { | ||
| // add 10 nodes at same level | ||
| F.AddVertex(NewVertexMock(vertexId, i, parentId, parentLevel)) | ||
| } | ||
| assert.Equal(t, uint64(1), F.GetSize()) |
There was a problem hiding this comment.
we are adding the same nodes 10 times, I was thinking a test case that adds 10 different nodes twice, and verify the size didn't change:
// nodes := createNodes(10)
//
// for each node
// F.AddVertex(node)
//
// size := F.GetSize()
//
// for each node
// F.AddVertex(node)
//
// assert.Equal(size, F.GetSize())
basically I don't care what's the value for GetSize, I only care whether it's changed or not.
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
dapperlabs/flow-go/issues/5670
Context
This PR solves some technical depth around calculating size of leveled forest. Instead of doing this for every class that use/will use
LevelledForestwe would embed knowledge of own size into leveled forest.As part of this PR I have also made next changes:
AssignmentCollectorTreeExecutionTreeNote on
ExecutionTree:ExecutionTreestill has custom logic for calculating size, since it calculates size based on how many receipts are stored but the leveled forest is built using the results. Meaning that one result can have multiple associated receipts. That is the reason why we still need custom logic.