trie_prefetcher: alternate structure #666

darioush · 2024-10-01T21:46:19Z

Why this should be merged

The current trie_prefetcher is modified significantly compared to upstream, which makes it difficult to merge changes from upstream.
This PR aims to preserve the existing behavior but only with minor inline modifications to upstream code.

I would be open to other structures with the same idea.

How this works

Uses a wrapper for DB to track the central worker pool.
Adds a couple hooks for stopping the background processes.

How this was tested

CI

core/state/trie_prefetcher.go

darioush · 2024-10-01T21:47:40Z

core/state/trie_prefetcher_test.go

@@ -59,7 +59,8 @@ func filledStateDB() *StateDB {

 func TestCopyAndClose(t *testing.T) {
 	db := filledStateDB()
-	prefetcher := newTriePrefetcher(db.db, db.originalRoot, "", maxConcurrency)
+	prefetchDb := newPrefetcherDatabase(db.db, maxConcurrency)


to preserve existing UT behavior for now

core/state/trie_prefetcher.go

core/state/prefetcher_database.go

Signed-off-by: Darioush Jalali <[email protected]>

…abs/coreth into trie-prefetcher-alt

core/state/trie_prefetcher.go

darioush · 2024-11-19T01:14:54Z

core/state/trie_prefetcher.go


 // triePrefetcher is an active prefetcher, which receives accounts or storage
 // items and does trie-loading of them. The goal is to get as much useful content
 // into the caches as possible.
 //
 // Note, the prefetcher's API is not thread safe.
 type triePrefetcher struct {
-	db       Database               // Database to fetch trie nodes through
+	db       PrefetcherDB           // Database to fetch trie nodes through


changed compared to upstream

darioush · 2024-11-19T01:15:07Z

core/state/trie_prefetcher.go

+	var pdb PrefetcherDB
+	pdb = withPrefetcherDefaults{db}
+	if db, ok := db.(withPrefetcherDB); ok {
+		pdb = db.PrefetcherDB()
+	}


changed compared to upstream

darioush · 2024-11-19T01:15:43Z

core/state/trie_prefetcher.go

-		storageFetchers int64
-		largestLoad     int64
-	)
+	defer p.db.Close()


added compared to upstream

darioush · 2024-11-19T01:16:04Z

core/state/trie_prefetcher.go

-	p *triePrefetcher
-
-	db    Database       // Database to load trie nodes through
+	db    PrefetcherDB   // Database to load trie nodes through


modified compared to upstream

darioush · 2024-11-19T01:16:24Z

core/state/trie_prefetcher.go

-		return 0
-	}
-	return sf.to.copies
+	sf.db.WaitTrie(sf.trie)


added compared to upstream

darioush · 2024-11-19T01:17:02Z

core/state/trie_prefetcher.go

+							sf.db.PrefetchAccount(sf.trie, common.BytesToAddress(task))
+						} else {
+							sf.db.PrefetchStorage(sf.trie, sf.addr, task)


modified. compared to upstream (made this more explicit to avoid overriding GetAccount and GetStorage)

darioush · 2024-11-19T01:19:19Z

Adding do not merge label prior to verifying performance

ARR4N · 2024-11-19T13:38:06Z

core/state/prefetcher_database.go

+}
+
+func (*prefetcherDatabase) PrefetchAccount(t Trie, address common.Address) {
+	t.(*prefetcherTrie).PrefetchAccount(address)


How can you be certain that t will be a *prefetcherTrie?

PrefetcherDB type is designed to minimize changes in trie_prefetcher so these methods are only called from the trie_prefetcher.
In this case, sf.trie is initialized with the return values of OpenTrie or OpenStorageTrie, which is the intended use of the PrefetcherDB type.

We could try to return another type from OpenTrie, however this lead to more changes in trie_prefetcher than I would consider "minimal", open to your suggestions.

Also existing code https://github.com/ava-labs/coreth/blob/master/core/state/database.go#L218-L225 couples the DB type to the trie type (CopyTrie). This code seems "gone" https://github.com/ethereum/go-ethereum/blob/master/core/state/database.go#L48 from the current versions of upstream which is very promising for the future

ARR4N · 2024-11-19T13:39:26Z

core/state/prefetcher_database.go

+	p.workers.Wait()
+}
+
+type PrefetcherDB interface {


(nit) Please move this to the top of the file as it's pertinent from the very beginning so helps in understanding this file with context.

(question) Why are you defining both the interface and the implementation here instead of just using the implementation directly?

Moving it to the top.

The implementation will later stay in coreth but the interfaces (and trie_prefetcher modifications) will be part of libevm.

core/state/prefetcher_database.go

ARR4N · 2024-11-20T08:57:04Z

core/state/prefetcher_database.go

+}
+
+func (withPrefetcherDefaults) PrefetchAccount(t Trie, address common.Address) {
+	_, _ = t.GetAccount(address)


Since you're defining the method signature, why not return an error if it occurs and allow the call-site to decide to drop it? Same applies to storage.

I added this 66e4674 however I don't really think it is helping with clarity. I'd slightly prefer to revert it and have the empty return signal that the callsite would be ignoring any errors so the callee should eg, log it if they care.
Let me know which you prefer.

Yeah I'm happy if you revert and log instead.

core/state/prefetcher_database.go

core/state/trie_prefetcher_extra_test.go

ARR4N · 2024-11-20T09:18:52Z

core/state/trie_prefetcher.go

@@ -332,6 +330,29 @@ func (sf *subfetcher) loop() {
 		}
 		sf.trie = trie
 	}
+	handleTask := func(task []byte) {


Is this just from upstream?

I moved this from https://github.com/ava-labs/coreth/pull/666/files/7b9959050f89377e30b15957fd5081e012cd720c..96575b96ada7cc778f0a53c5045a9539f5ec530b#diff-58140c10803d5c33def41482e87d096548e88382aacc21aa7dbee1e918915870L361-L370 so I can call it in defer.

Using defer seems cleaner than to modify the channel logic in the loop or repeating the logic in defer, which could diverge from upstream without us noticing.
As we would encounter a merge conflict for the code that handles the task since we modified GetAccount to PrefetchAccount (to avoid wrapping GetAccount), this seems fine to me.

Here's another approach for example ava-labs/subnet-evm@5a48a7d#diff-58140c10803d5c33def41482e87d096548e88382aacc21aa7dbee1e918915870R361 (but this does not support the original pattern).

Overall I am open to changing it to a different approach if you have one in mind.

This reverts commit 66e4674.

ceyonur

few nits, otherwise LGTM.

ceyonur · 2024-11-21T10:14:38Z

core/state/prefetcher_database.go

+// withPrefetcherDefaults extends Database and implements PrefetcherDB by adding
+// default implementations for PrefetchAccount and PrefetchStorage that read the
+// account and storage slot from the trie.
+type withPrefetcherDefaults struct {


is this no-op prefetcher?

nit: should we add a type assertion and maybe move this to no_op_prefetcher file?

trie_prefetcher: alternate structure

8d51b81

darioush commented Oct 1, 2024

View reviewed changes

core/state/trie_prefetcher.go Outdated Show resolved Hide resolved

darioush commented Oct 1, 2024

View reviewed changes

core/state/trie_prefetcher.go Outdated Show resolved Hide resolved

darioush commented Oct 1, 2024

View reviewed changes

fix import

f3612cd

darioush force-pushed the trie-prefetcher-alt branch from 3d6f372 to f3612cd Compare October 1, 2024 22:20

copies return unwrapped

ed77f19

darioush commented Oct 1, 2024

View reviewed changes

core/state/trie_prefetcher.go Outdated Show resolved Hide resolved

darioush commented Oct 1, 2024

View reviewed changes

core/state/prefetcher_database.go Outdated Show resolved Hide resolved

darioush added 2 commits October 16, 2024 07:56

Update core/state/prefetcher_database.go

1cd4c52

Signed-off-by: Darioush Jalali <[email protected]>

Merge branches 'master' and 'trie-prefetcher-alt' of github.com:ava-l…

219cc16

…abs/coreth into trie-prefetcher-alt

darioush closed this Nov 18, 2024

darioush reopened this Nov 18, 2024

darioush added 3 commits November 18, 2024 14:34

handle passing of concurrency

5640be0

fix maxConcurrency, make Prefetch calls explicit

2f9e984

modify Wait style

87d49a6

darioush commented Nov 18, 2024

View reviewed changes

core/state/trie_prefetcher.go Outdated Show resolved Hide resolved

darioush commented Nov 18, 2024

View reviewed changes

core/state/trie_prefetcher.go Outdated Show resolved Hide resolved

darioush commented Nov 18, 2024

View reviewed changes

core/state/trie_prefetcher.go Outdated Show resolved Hide resolved

trie_prefetcher: alternate structure (possible improvements) (#689)

7b99590

darioush commented Nov 19, 2024

View reviewed changes

darioush added the Do not merge label Nov 19, 2024

darioush marked this pull request as ready for review November 19, 2024 01:35

darioush requested review from ceyonur and a team as code owners November 19, 2024 01:35

ARR4N reviewed Nov 19, 2024

View reviewed changes

darioush added 3 commits November 19, 2024 08:38

move interface, add comments

6ac2bb7

nit

fba8a01

add correct waiting pattern and benchmark

5419db9

nits

96575b9

ARR4N requested changes Nov 20, 2024

View reviewed changes

darioush added 2 commits November 20, 2024 07:48

review comments

49cb033

add error for PrefetchAccount

66e4674

ARR4N approved these changes Nov 20, 2024

View reviewed changes

darioush added 2 commits November 20, 2024 08:37

Revert "add error for PrefetchAccount"

bfb906e

This reverts commit 66e4674.

log error

adc1020

ceyonur reviewed Nov 21, 2024

View reviewed changes

darioush marked this pull request as draft November 25, 2024 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trie_prefetcher: alternate structure #666

trie_prefetcher: alternate structure #666

darioush commented Oct 1, 2024 •

edited

Loading

darioush Oct 1, 2024

darioush Nov 19, 2024

darioush Nov 19, 2024

darioush Nov 19, 2024

darioush Nov 19, 2024

darioush Nov 19, 2024

darioush Nov 19, 2024

darioush commented Nov 19, 2024

ARR4N Nov 19, 2024

darioush Nov 19, 2024

darioush Nov 19, 2024 •

edited

Loading

ARR4N Nov 19, 2024

darioush Nov 19, 2024

ARR4N Nov 20, 2024

darioush Nov 20, 2024 •

edited

Loading

ARR4N Nov 20, 2024

ARR4N Nov 20, 2024

darioush Nov 20, 2024 •

edited

Loading

ceyonur left a comment

ceyonur Nov 21, 2024

ceyonur Nov 21, 2024

trie_prefetcher: alternate structure #666

Are you sure you want to change the base?

trie_prefetcher: alternate structure #666

Conversation

darioush commented Oct 1, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

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

darioush commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Nov 19, 2024 • edited Loading

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

darioush Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

ceyonur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush commented Oct 1, 2024 •

edited

Loading

darioush Nov 19, 2024 •

edited

Loading

darioush Nov 20, 2024 •

edited

Loading

darioush Nov 20, 2024 •

edited

Loading