update trie on goroutines #6463

BeniaminDrasovean · 2024-09-17T07:58:39Z

Reasoning behind the pull request

All the trie changes are put in a batch. When a commit occurs, the changes batch is added to the trie, but the changes are done in serial.

Proposed changes

When the changes from the batch need to be added to the trie, use goroutines where possible in order to speed up the update of the trie

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?

ssd04 · 2024-10-07T17:52:32Z

trie/branchNode.go

+			newModifiedHashes, bnModified := bn.insertOnChild(dataForInsertion[childPos], childPos, goRoutinesManager, db)
+			if bnModified {
+				bnHasBeenModified.SetValue(true)
+			}
+			if len(newModifiedHashes) != 0 {
+				hashesMutex.Lock()
+				modifiedHashes = append(modifiedHashes, newModifiedHashes...)
+				hashesMutex.Unlock()
+			}


not a strong opinion on this, but we might consider extracting this block to a function even if there are a lot of parameters to pass

Indeed, will extract in a different function in order to avoid duplicated code.

ssd04 · 2024-10-07T18:06:23Z

trie/branchNode.go

+func (bn *branchNode) deleteChild(
+	dataForRemoval []core.TrieData,
+	childPos int,
+	hasBeenModified *atomic.Flag,


return hasBeenModified as a variable similar to insertOnChild and set atomic flag as in insert case? this way the dataForInsertion(Removal) flows are very similar, they might be able to be extracted in a common function or component since insetOnChild and deleteChild would have the same singature

Refactored.

ssd04 · 2024-10-07T18:09:22Z

trie/branchNode.go

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


for which variable(s) is the lock? i think we need to add lock also in other places

These are for dirty, hash, version. Indeed, this will need to be added in other places. Will do that in another PR, in which I will also write some tests for this cases.

ssd04 · 2024-10-07T18:11:27Z

trie/branchNode_test.go

+	th, _ := throttler.NewNumGoRoutinesThrottler(5)
+	goRoutinesManager, err := NewGoroutinesManager(th, errChan.NewErrChanWrapper(), make(chan struct{}))
+	assert.Nil(t, err)


add a function for creating goroutinesManager?

ssd04 · 2024-10-07T18:19:04Z

trie/goroutinesManager.go

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


do we need mutex here?

Yes, because the same manager will be passed on multiple goroutines.

ssd04 · 2024-10-07T18:19:13Z

trie/goroutinesManager.go

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


Yes, because the same manager will be passed on multiple goroutines.

ssd04 · 2024-10-07T18:21:59Z

trie/goroutinesManager_test.go

+
+		err = errCh.ReadFromChanNonBlocking()
+		assert.Equal(t, expectedErr, err)
+		assert.False(t, manager.(*goroutinesManager).canProcess)


maybe better expose canProcess in export_test.go to avoid casting here?

Actually, the problem was that the constructor was returning an interface, not the actual type. Changed the constructor.

…ate-trie-on-goroutines # Conflicts: # trie/branchNode_test.go # trie/extensionNode_test.go

sasurobert · 2024-12-02T12:43:11Z

common/modifiedHashes.go

+// NewModifiedHashesSlice is used to create a new instance of modifiedHashesSlice
+func NewModifiedHashesSlice() *modifiedHashesSlice {
+	return &modifiedHashesSlice{
+		hashes: make([][]byte, 0),


It would be better to allocate some capacity to the hashes.

sasurobert · 2024-12-02T13:03:49Z

trie/patriciaMerkleTrie.go

@@ -80,6 +83,12 @@ func NewTrie(
 		return nil, err
 	}

+	// TODO give this as an argument
+	trieThrottler, err := throttler.NewNumGoRoutinesThrottler(20)


only the config of how many go routines is enough for the arg.

Yes, that is what I was also thinking, but phrased it wrong. Rephrased the TODO. It will be implemented in a future PR.

sasurobert · 2024-12-02T13:25:16Z

trie/patriciaMerkleTrie.go

@@ -203,7 +213,14 @@ func (tr *patriciaMerkleTrie) insertBatch(sortedDataForInsertion []core.TrieData
 		tr.oldRoot = tr.root.getHash()
 	}

-	newRoot, oldHashes, err := tr.root.insert(sortedDataForInsertion, tr.trieStorage)
+	manager, err := NewGoroutinesManager(tr.goroutinesThrottler, errChan.NewErrChanWrapper(), tr.chanClose)


do you need to recreate this manager every time ?

can't you create it once and only clean up here ?

sasurobert · 2024-12-02T13:26:52Z

trie/patriciaMerkleTrie.go

@@ -232,11 +250,20 @@ func (tr *patriciaMerkleTrie) deleteBatch(data []core.TrieData) error {
 		tr.oldRoot = tr.root.getHash()
 	}

-	_, newRoot, oldHashes, err := tr.root.delete(data, tr.trieStorage)
+	manager, err := NewGoroutinesManager(tr.goroutinesThrottler, errChan.NewErrChanWrapper(), tr.chanClose)


same as above ? can't you have the manager created once, and only clean it up here ?

ssd04 · 2024-12-05T13:04:37Z

common/modifiedHashes.go

+	mhs.Lock()
+	defer mhs.Unlock()
+
+	mhs.hashes = nil


set to empty slice as in constructor?

Removed func as it was not used anywhere. Will add it again if needed.

ssd04 · 2024-12-05T13:05:25Z

common/modifiedHashes_test.go

+	assert.NotEqual(t, retrievedHashes, newRetrievedHashes)
+}
+
+func TestModifiedHashesSlice_Reset(t *testing.T) {


add a test with all these operations combined? append, reset, append

Removed the Reset() func

ssd04 · 2024-12-05T13:07:15Z

common/modifiedHashes.go

@@ -1,5 +1,7 @@
 package common

+import "sync"
+
 // ModifiedHashes is used to memorize all old hashes and new hashes from when a trie is committed
 type ModifiedHashes map[string]struct{}


do we still need this struct?

Yes, it is still used in other places.

The base branch was changed.

BeniaminDrasovean added 2 commits September 17, 2024 10:57

refactor the trie nodes insert functions to use goroutines

da6dd06

refactor the trie nodes delete function to use goroutines

29fb428

BeniaminDrasovean changed the title ~~refactor the trie nodes insert functions to use goroutines~~ update trie on goroutines Sep 25, 2024

BeniaminDrasovean added 2 commits September 25, 2024 13:13

fix race conditions

51ecae4

add unit tests for goroutinesManager

f2aa05a

BeniaminDrasovean marked this pull request as ready for review October 1, 2024 12:36

ssd04 reviewed Oct 7, 2024

View reviewed changes

BeniaminDrasovean added 4 commits November 25, 2024 12:30

Merge remote-tracking branch 'origin/update-trie-from-batch' into upd…

c0c8677

…ate-trie-on-goroutines # Conflicts: # trie/branchNode_test.go # trie/extensionNode_test.go

fix after review

fc37d5b

add mutex for branch node

e645926

add EndGoRoutineProcessing call

6c93eb3

sasurobert reviewed Dec 2, 2024

View reviewed changes

fix after review

cd6e342

sasurobert previously approved these changes Dec 5, 2024

View reviewed changes

ssd04 reviewed Dec 5, 2024

View reviewed changes

Base automatically changed from update-trie-from-batch to feat/trie-mutex-refactor December 5, 2024 13:55

BeniaminDrasovean and others added 2 commits December 5, 2024 15:55

Merge branch 'feat/trie-mutex-refactor' into update-trie-on-goroutines

a5d021f

fix after review

ce9af09

ssd04 approved these changes Dec 5, 2024

View reviewed changes

sasurobert approved these changes Dec 9, 2024

View reviewed changes

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

BeniaminDrasovean deleted the update-trie-on-goroutines branch December 9, 2024 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update trie on goroutines #6463

update trie on goroutines #6463

BeniaminDrasovean commented Sep 17, 2024 •

edited

Loading

ssd04 Oct 7, 2024

BeniaminDrasovean Nov 25, 2024

BeniaminDrasovean Nov 25, 2024

ssd04 Oct 7, 2024

BeniaminDrasovean Nov 25, 2024

ssd04 Oct 7, 2024

BeniaminDrasovean Nov 26, 2024

ssd04 Oct 7, 2024

BeniaminDrasovean Nov 26, 2024

ssd04 Oct 7, 2024

BeniaminDrasovean Nov 26, 2024

ssd04 Oct 7, 2024

BeniaminDrasovean Nov 26, 2024

ssd04 Oct 7, 2024

BeniaminDrasovean Nov 26, 2024

sasurobert Dec 2, 2024

BeniaminDrasovean Dec 4, 2024

sasurobert Dec 2, 2024

BeniaminDrasovean Dec 4, 2024

sasurobert Dec 2, 2024

BeniaminDrasovean Dec 4, 2024

sasurobert Dec 2, 2024

BeniaminDrasovean Dec 4, 2024

ssd04 Dec 5, 2024

BeniaminDrasovean Dec 5, 2024

ssd04 Dec 5, 2024

BeniaminDrasovean Dec 5, 2024

ssd04 Dec 5, 2024

BeniaminDrasovean Dec 5, 2024

update trie on goroutines #6463

update trie on goroutines #6463

Conversation

BeniaminDrasovean commented Sep 17, 2024 • edited Loading

Reasoning behind the pull request

Proposed changes

Pre-requisites

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 Sep 17, 2024 •

edited

Loading