Proofs pool improvements #6878

ssd04 · 2025-03-13T14:49:40Z

Reasoning behind the pull request

Remove sort operation when adding to pool
Added more unit tests to proofsCache

Testing procedure

to be tested with 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?

sstanculeanu · 2025-03-17T10:24:57Z

dataRetriever/dataPool/proofsCache/proofsBucket.go

+func (p *proofNonceBucket) insertInNew(proof data.HeaderProofHandler) {
+	p.insert(proof)
+	p.maxNonce = proof.GetHeaderNonce()
+}
+
+func (p *proofNonceBucket) insertInExisting(proof data.HeaderProofHandler) {
+	p.insert(proof)
+
+	if proof.GetHeaderNonce() > p.maxNonce {
+		p.maxNonce = proof.GetHeaderNonce()
+	}
+}
+
+func (p *proofNonceBucket) insert(proof data.HeaderProofHandler) {
+	p.proofsByNonce = append(p.proofsByNonce, &proofNonceMapping{
+		headerHash: string(proof.GetHeaderHash()),
+		nonce:      proof.GetHeaderNonce(),
+	})
+}


why not move

if proof.GetHeaderNonce() > p.maxNonce { p.maxNonce = proof.GetHeaderNonce() }

into insert method and only keep one method for all 3 operations?

right, pushed

andreibancioiu · 2025-03-17T10:13:13Z

dataRetriever/dataPool/proofsCache/proofsCache_bench_test.go

@@ -0,0 +1,66 @@
+package proofscache_test


If we move the test to package proofscache, maybe we can drop some code from the export_test file. Can also stay as it is.

i would keep it in _test package since there are other useful functions from test

and we plan to make this component more generic and to export it in other packages as well (for headers pool)

andreibancioiu · 2025-03-17T10:17:37Z

dataRetriever/dataPool/proofsCache/proofsCache_bench_test.go

+func Benchmark_AddProof_Bucket10_Pool1000(b *testing.B) {
+	benchmarkAddProof(b, 10, 1000)
+}
+
+func Benchmark_AddProof_Bucket100_Pool10000(b *testing.B) {
+	benchmarkAddProof(b, 100, 10000)
+}
+
+func Benchmark_AddProof_Bucket1000_Pool100000(b *testing.B) {
+	benchmarkAddProof(b, 1000, 100000)
+}


Maybe also add some output / statistics in the PR description.

andreibancioiu · 2025-03-17T10:35:24Z

dataRetriever/dataPool/proofsCache/proofsCache.go

+	mutProofsByNonce     sync.RWMutex
+	proofsByNonceBuckets []*proofNonceBucket


Alternatively, maybe sync.Map is useful?

the way proofsByNonceBuckets is implemented right now, it needs order, so it doesn't fit very well

Sorry, you are right. No maps here 👍

andreibancioiu · 2025-03-17T10:50:37Z

dataRetriever/dataPool/proofsCache/proofsBucket.go

+func (p *proofNonceBucket) insertInNew(proof data.HeaderProofHandler) {
+	p.insert(proof)
+	p.maxNonce = proof.GetHeaderNonce()
+}
+
+func (p *proofNonceBucket) insertInExisting(proof data.HeaderProofHandler) {
+	p.insert(proof)
+
+	if proof.GetHeaderNonce() > p.maxNonce {
+		p.maxNonce = proof.GetHeaderNonce()
+	}
+}


Can we merge them into a single insert function or does it break the logic? If it was done for the purpose of optimizing the nonce check - the if proof.GetHeaderNonce() > p.maxNonce { is just a quick comparison (should be negligible).

Outdated comment, fixed upon Sorin's suggestion.

andreibancioiu · 2025-03-17T10:56:40Z

dataRetriever/dataPool/proofsCache/proofsCache.go

+	pc.mutProofsByHash.Lock()
+	defer pc.mutProofsByHash.Unlock()


The caller function also locks (sorry if I'm mistaken).

there are 2 separate locks

Indeed, my bad / overlooked!

andreibancioiu · 2025-03-17T11:00:11Z

dataRetriever/dataPool/proofsCache/proofsCache.go

+	for _, bucket := range pc.proofsByNonceBuckets {
+		if nonce > bucket.maxNonce {
+			wg.Add(1)

-	proofsByNonce := make([]*proofNonceMapping, 0)
+			go func(bucket *proofNonceBucket) {
+				pc.cleanupProofsInBucket(bucket)
+				wg.Done()
+			}(bucket)


Maybe allow the cleanup to happen synchronously? I see the workload is lock, delete from map in a loop, unlock. Do we have a speed gain using concurrency in this context (since map deletions are not quite subject to parallelization)?

removed concurrency here

andreibancioiu · 2025-03-17T14:35:15Z

dataRetriever/dataPool/proofsCache/proofsCache.go


-	pc.mutProofsCache.Lock()
-	defer pc.mutProofsCache.Unlock()
+	pc.mutProofsByNonce.Lock()


If cleanup procedure does not happen extremely often (~rare event), maybe both locks can happen one near the other - e.g. lock & unlock mutProofsByHash for all buckets at once (I think this was Sorin's internal suggestion).

andreibancioiu · 2025-03-17T14:38:36Z

dataRetriever/dataPool/proofsCache/proofsCache.go

-	defer pc.mutProofsCache.Unlock()
+	pc.insertProofByNonce(proof)

-	pc.proofsByNonce = append(pc.proofsByNonce, &proofNonceMapping{
-		headerHash: string(proof.GetHeaderHash()),
-		nonce:      proof.GetHeaderNonce(),
-	})
+	pc.mutProofsByHash.Lock()
+	pc.proofsByHash[string(proof.GetHeaderHash())] = proof
+	pc.mutProofsByHash.Unlock()


Having these two locks separated does not seem to be an optimization (at least, not at a first glance). Since:

lock A do ~trivial (& with short duration) logic around insertion in bucket unlock A lock B do ~trivial (& with short duration) operation of adding a map entry unlock B

Is not necessarily better than:

lock C do ~trivial (& with short duration) logic around insertion in bucket do ~trivial (& with short duration) operation of adding a map entry unlock C

Especially if extreme concurrency (e.g. a lot of new insertions bootstrapped during each of the ~trivial two parts) isn't expected.

right, updated to keep only one mutex

AdoAdoAdo · 2025-03-18T11:43:26Z

dataRetriever/dataPool/proofsCache/proofsBucket.go

+
+type proofNonceBucket struct {
+	maxNonce      uint64
+	proofsByNonce []*proofNonceMapping


maybe use a map then the upsert would not need to iterate

changed to use map

AdoAdoAdo · 2025-03-18T12:11:24Z

dataRetriever/dataPool/proofsCache/proofsBucket.go

+	return len(p.proofsByNonce)
+}
+
+func (p *proofNonceBucket) isFull() bool {


I would use fixed size and deterministic range for each bucket.
e.g a proof with nonce x should be added into the bucket with first nonce x/bucketSize * bucketSize (integer division)

changed to use fixed size buckets

AdoAdoAdo · 2025-03-18T12:12:14Z

dataRetriever/dataPool/proofsCache/proofsCache.go

-	proofsByNonce  []*proofNonceMapping
-	proofsByHash   map[string]data.HeaderProofHandler
+	mutProofsCache       sync.RWMutex
+	proofsByNonceBuckets []*proofNonceBucket


I think this can be a map as well with the key the lowest nonce that the bucket would hold

changed to use sync.Map

AdoAdoAdo · 2025-03-18T12:13:14Z

dataRetriever/dataPool/proofsCache/proofsCache.go

+
+	headBucket := pc.proofsByNonceBuckets[0]
+
+	if headBucket.isFull() {


in case of syncing node this will cause it to keep accumulating new nonces in each old nonces buckets, which will delay their cleanup.

new model with range buckets should cover this case

sstanculeanu · 2025-03-18T13:18:29Z

dataRetriever/dataPool/proofsCache/proofsCache.go

 	mutProofsCache       sync.RWMutex
-	proofsByNonceBuckets []*proofNonceBucket
-	bucketSize           int
+	proofsByNonceBuckets sync.Map


proofNonceMapping struct not used anymore

👍 deleted it

sstanculeanu · 2025-03-18T13:27:02Z

dataRetriever/dataPool/proofsCache/proofsCache.go


-	pc.proofsByNonceBuckets = buckets
+	for _, key := range bucketsToDelete {
+		pc.proofsByNonceBuckets.Delete(key)


this is already done on L86

right, old code; removed

remove sort operation from proofs pool insert

35f8e8a

ssd04 self-assigned this Mar 13, 2025

ssd04 added 5 commits March 17, 2025 10:23

added proofs cache buckets

91fd46e

add parallel cleanup

7066189

proofs cache benchmarks

147c0d3

added bucket size to config

c21c78c

Merge branch 'feat/andromeda-fixes' into fix-proofs-pool-insert

5704d4e

sstanculeanu reviewed Mar 17, 2025

View reviewed changes

fix after review

3a1a9e2

sstanculeanu previously approved these changes Mar 17, 2025

View reviewed changes

andreibancioiu reviewed Mar 17, 2025

View reviewed changes

remove concurrency from cleanup

e03e7a1

ssd04 dismissed sstanculeanu’s stale review via e03e7a1 March 17, 2025 12:07

andreibancioiu previously approved these changes Mar 17, 2025

View reviewed changes

AdoAdoAdo mentioned this pull request Mar 17, 2025

Rc/andromeda #6747

Merged

fix after review: keep only one cache

b2fc09a

ssd04 dismissed andreibancioiu’s stale review via b2fc09a March 17, 2025 15:31

sstanculeanu previously approved these changes Mar 17, 2025

View reviewed changes

andreibancioiu previously approved these changes Mar 17, 2025

View reviewed changes

ssd04 changed the title ~~Remove sort operation from proofs pool insert~~ Proofs pool improvements Mar 17, 2025

sstanculeanu added 5 commits March 18, 2025 11:27

Merge branch 'feat/andromeda-fixes' into fix-proofs-pool-insert

ac7f4ab

Merge branch 'feat/andromeda-fixes' into fix-proofs-pool-insert

b759ac8

Merge branch 'feat/andromeda-fixes' into fix-proofs-pool-insert

3cb4bce

Merge branch 'feat/andromeda-fixes' into fix-proofs-pool-insert

57532ba

Merge branch 'feat/andromeda-fixes' into fix-proofs-pool-insert

36ae586

AdoAdoAdo reviewed Mar 18, 2025

View reviewed changes

use range buckets instead of stack buckets

4ea5d8e

ssd04 dismissed stale reviews from andreibancioiu and sstanculeanu via 4ea5d8e March 18, 2025 13:02

sstanculeanu reviewed Mar 18, 2025

View reviewed changes

fixes after review

21fd3ea

sstanculeanu previously approved these changes Mar 18, 2025

View reviewed changes

Merge branch 'feat/andromeda-fixes' into fix-proofs-pool-insert

210f52b

AdoAdoAdo previously approved these changes Mar 18, 2025

View reviewed changes

use simple map instead of sync.Map

acf47c7

ssd04 dismissed stale reviews from AdoAdoAdo and sstanculeanu via acf47c7 March 18, 2025 14:51

delete map entry while iterating

ecd4e54

sstanculeanu approved these changes Mar 18, 2025

View reviewed changes

AdoAdoAdo approved these changes Mar 18, 2025

View reviewed changes

ssd04 merged commit d163e58 into feat/andromeda-fixes Mar 18, 2025
4 checks passed

ssd04 deleted the fix-proofs-pool-insert branch March 18, 2025 15:05

		mutProofsByNonce sync.RWMutex
		proofsByNonceBuckets []*proofNonceBucket


		headBucket := pc.proofsByNonceBuckets[0]

		if headBucket.isFull() {

Proofs pool improvements #6878

Proofs pool improvements #6878

Uh oh!

Conversation

ssd04 commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reasoning behind the pull request

Testing procedure

Pre-requisites

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssd04 Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreibancioiu Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssd04 Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

ssd04 commented Mar 13, 2025 •

edited

Loading

ssd04 Mar 17, 2025 •

edited

Loading

andreibancioiu Mar 17, 2025 •

edited

Loading

ssd04 Mar 18, 2025 •

edited

Loading