remove mutex from trie operations #6526

BeniaminDrasovean · 2024-10-08T09:09:35Z

Reasoning behind the pull request

The trie accesses are serialized using a mutex on all of the public functions. While this assures that parallel calls will not result in data races, it also introduces a bottleneck.

Proposed changes

Remove the mutex from the public trie functions, and synchronize the accesses at the trie node level.
Remove GetAllHashes func from the trie. This was not used, and it could lead to oom if used for a large trie.
Refactor get, update and delete from the trie so that they can be called in parallel

Testing procedure

Unit tests
System tests when all the PRs are merged in the feat branch

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

…refactor-trie-mutex-usage # Conflicts: # trie/branchNode.go # trie/branchNode_test.go # trie/extensionNode.go # trie/extensionNode_test.go # trie/interface.go # trie/leafNode.go # trie/leafNode_test.go # trie/patriciaMerkleTrie.go

…refactor-trie-mutex-usage # Conflicts: # trie/branchNode_test.go # trie/extensionNode_test.go # trie/leafNode_test.go # trie/patriciaMerkleTrie.go

codecov · 2024-12-09T11:30:33Z

Codecov Report

Attention: Patch coverage is 90.65421% with 30 lines in your changes missing coverage. Please review.

Project coverage is 79.94%. Comparing base (a5bc666) to head (63d7061).
Report is 24 commits behind head on feat/trie-mutex-refactor.

Files with missing lines	Patch %	Lines
trie/modifiedChildren.go	0.00%	13 Missing ⚠️
trie/branchNode.go	88.00%	6 Missing and 3 partials ⚠️
trie/extensionNode.go	92.00%	2 Missing and 2 partials ⚠️
trie/patriciaMerkleTrie.go	96.19%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@                    Coverage Diff                     @@
##           feat/trie-mutex-refactor    #6526    +/-   ##
==========================================================
  Coverage                     79.93%   79.94%            
==========================================================
  Files                           733      737     +4     
  Lines                         96458    96604   +146     
==========================================================
+ Hits                          77106    77227   +121     
- Misses                        13944    13981    +37     
+ Partials                       5408     5396    -12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sasurobert · 2024-12-11T09:27:40Z

trie/interface.go

@@ -110,3 +107,12 @@ type IdleNodeProvider interface {
 	IsIdle() bool
 	IsInterfaceNil() bool
 }
+
+type RootManager interface {


comment missing.

sasurobert · 2024-12-11T10:02:11Z

trie/extensionNode.go

+	defer en.childMutex.Unlock()
+
+	isChildCollapsed := en.child == nil && len(en.EncodedChild) != 0
+	if !isChildCollapsed {


why not use en.isCollaped? exactly the same functions is wrote on top of this function.

sasurobert · 2024-12-11T10:24:24Z

trie/patriciaMerkleTrie.go

@@ -249,12 +255,14 @@ func (tr *patriciaMerkleTrie) deleteBatch(data []core.TrieData) error {
 		return nil
 	}

-	if tr.root == nil {
+	rootNode := tr.GetRootNode()
+	if rootNode == nil {


check.IfNil ?

sasurobert · 2024-12-11T10:30:40Z

trie/patriciaMerkleTrie.go


-	if tr.root == nil {
+	rootNode := tr.GetRootNode()
+	if rootNode == nil {


check.ifnil?

sasurobert · 2024-12-11T10:33:26Z

trie/rootManager.go

+	mutOperation sync.RWMutex
+}
+
+func NewRootManager() *rootManager {


a lot of comments are missing here. on all the file.

ssd04 · 2024-12-13T13:54:40Z

state/accountsDB.go

 		return nil
 	}

-	err := adb.removeCode(baseAcc)
-	if err != nil {
-		return err
-	}
-
-	err = adb.removeDataTrie(baseAcc)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func (adb *AccountsDB) removeDataTrie(baseAcc baseAccountHandler) error {
-	rootHash := baseAcc.GetRootHash()
-	if len(rootHash) == 0 {
-		return nil
-	}
-
-	dataTrie, err := adb.mainTrie.Recreate(rootHash)
-	if err != nil {
-		return err
-	}
-
-	hashes, err := dataTrie.GetAllHashes()
-	if err != nil {
-		return err
-	}
-
-	adb.obsoleteDataTrieHashes[string(rootHash)] = hashes
-
-	entry, err := NewJournalEntryDataTrieRemove(rootHash, adb.obsoleteDataTrieHashes)
-	if err != nil {
-		return err
-	}
-	adb.journalize(entry)
-
-	return nil
+	return adb.removeCode(baseAcc)


rename removeCodeAndDataTrie func? or remove it and use removeCode directly?

We need to keep track of this. When RemoveAccount() will be used, we need to implement an efficient way of marking the removed hashes for pruning. Added a TODO

ssd04 · 2024-12-13T14:56:24Z

trie/patriciaMerkleTrie.go

@@ -201,22 +205,25 @@ func (tr *patriciaMerkleTrie) insertBatch(sortedDataForInsertion []core.TrieData
 		return nil
 	}

-	if tr.root == nil {
+	rootNode := tr.GetRootNode()
+	if rootNode == nil {


check.IfNil ?

ssd04 · 2024-12-13T15:48:08Z

trie/baseNode.go

+	hash   []byte
+	dirty  bool


shouldn't we protect these fields by mutex in setters and getters?

Indded. Added TODO and will add mutex protection in a future PR (to minimize conflicts with already opened PRs)

ssd04 · 2024-12-13T16:15:56Z

trie/branchNode.go

+	bn.mutex.Lock()
+	defer bn.mutex.Unlock()


if setters in baseNode are protected by mutex, i think this protection here is not needed

It is needed in order to keep all changes atomic. If I remove this there might be edgecases where different baseNode fields change from different goroutines.

ssd04 · 2024-12-13T16:18:53Z

trie/interface.go

+	setDirty(bool)
+	isDirty() bool
+	getMarshalizer() marshal.Marshalizer
+	setMarshalizer(marshal.Marshalizer)
+	getHasher() hashing.Hasher
+	setHasher(hashing.Hasher)


maybe we can have a baseNode interface

ssd04 · 2024-12-13T16:21:13Z

trie/branchNode.go

 	bn.hash = nil
+	bn.dirty = true


if mutex protection is added in baseNode, cand we use setters here and avoid mutex in this file entirely?

Not really, all these operations need to happen atomically.

ssd04 · 2024-12-13T16:22:45Z

trie/leafNode.go

+	ln.mutex.RLock()
+	defer ln.mutex.RUnlock()


do we need mutex here?

this mutex is protecting hash and dirty fields, right?

It protects everything that can change on the node, so ln.Key & ln.Value also

ssd04 · 2024-12-13T16:24:12Z

trie/modifiedChildren.go

add unit tests for this file (or TODO for future PRs)

Removed the file as it is no longer used.

ssd04 · 2024-12-13T16:24:32Z

trie/rootManager.go

add unit tests for this file (or TODO for future PRs)

ssd04 · 2024-12-13T16:26:06Z

trie/baseNode.go

+func (bn *baseNode) isDirty() bool {
+	return bn.dirty


i think rootManager is accessing this so it needs protection here

Indeed, will refactor mutex usage in a future PR

BeniaminDrasovean added 7 commits October 8, 2024 12:08

remove mutex from trie operations

97abe3f

update and get from trie in parallel

f9aa79e

fix after merge

99babd1

small refactor, fix failing tests, add mutex protection

9bba06e

Merge remote-tracking branch 'origin/update-trie-on-goroutines' into …

b291e33

…refactor-trie-mutex-usage # Conflicts: # trie/branchNode_test.go # trie/extensionNode_test.go # trie/leafNode_test.go # trie/patriciaMerkleTrie.go

fix after merge

71b652b

Base automatically changed from update-trie-on-goroutines to feat/trie-mutex-refactor December 9, 2024 09:30

BeniaminDrasovean and others added 2 commits December 9, 2024 13:10

Merge branch 'feat/trie-mutex-refactor' into refactor-trie-mutex-usage

ae2b9f7

remove unused code

63d7061

BeniaminDrasovean marked this pull request as ready for review December 9, 2024 11:32

sasurobert self-requested a review December 11, 2024 07:24

sasurobert requested changes Dec 11, 2024

View reviewed changes

fix after review

c801937

ssd04 self-requested a review December 12, 2024 16:38

ssd04 requested changes Dec 13, 2024

View reviewed changes

fix after review

3259480

ssd04 approved these changes Dec 16, 2024

View reviewed changes

sasurobert approved these changes Dec 18, 2024

View reviewed changes

BeniaminDrasovean merged commit be9a565 into feat/trie-mutex-refactor Dec 18, 2024
5 checks passed

BeniaminDrasovean deleted the refactor-trie-mutex-usage branch December 18, 2024 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove mutex from trie operations #6526

remove mutex from trie operations #6526

BeniaminDrasovean commented Oct 8, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024

sasurobert Dec 11, 2024

BeniaminDrasovean Dec 12, 2024

sasurobert Dec 11, 2024

BeniaminDrasovean Dec 12, 2024

sasurobert Dec 11, 2024

BeniaminDrasovean Dec 12, 2024

sasurobert Dec 11, 2024

BeniaminDrasovean Dec 12, 2024

sasurobert Dec 11, 2024

BeniaminDrasovean Dec 12, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

ssd04 Dec 13, 2024

BeniaminDrasovean Dec 16, 2024

remove mutex from trie operations #6526

remove mutex from trie operations #6526

Conversation

BeniaminDrasovean commented Oct 8, 2024 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Dec 9, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeniaminDrasovean commented Oct 8, 2024 •

edited

Loading