Limit capacities of maps in sendersBatch

[Qjawko]

Rationale

The sendersBatch structure is currently using too much CPU and memory, as well as allocating a large amount of resources. As a result, when the flush function starts iterating over a map, it takes a significant amount of CPU time due to Go's standard map's evacuation mechanism. In order to improve the efficiency of Erigon's staged sync and other components, it would be helpful to optimize or limit the use of maps.

CPU profile
Memory profile

Implementation

Add flags txpool.senders.cache_capacity and txpool.flush_every or even another data structure instead of std map.
Let's discuss our ideas how to do it and I am willing to implement this feature.

[AskAlexSharov]

As I remember p.senders was added to enumerate all senders - give them uint64 ID instead of using their addr (in all other data structures).

In flush I see:

for addr, id := range p.senders.senderIDs { // no inverted index - tradeoff flush speed for memory usage
	if id == metaTx.Tx.SenderID {
		copy(v[:20], addr)
		break
	}
}		

But seems it's not true and we have inverted index already: p.senders.senderID2Addr
Just need use it instead of brude-force

I don't really remember now: p.senders is the cache (which can be limited) or enumeration (primary index) which can't. In my head it can't be limited, but it must not keep any trash: id's of senders which not used in any other in-mem data structures. (maybe I'm wrong).

In-theory can persist p.senders in TxPool's DB: but I don't have intuition - is it good idea or not.

p.senders has 2 fields: senderIDs and senderID2Addr which now seems store add twice. They must share memory: senderIDs string must keep the addr and senderID2Addr must refer to that memory (In Golang when cast string to []bytes - it doesn't copy).

About txpool.flush_every - yes need add such CLI option

[Qjawko]

In Golang when cast string to []bytes - it doesn't copy

Thanks for the reply.

I thought in strings without a copy it would be possible to modify the contents of the string (by changing the elements of the byte slice), WDYT?
Maybe we should try to use non-copy unsafe operation in getOrCreateID method ([]byte -> string) to reduce memory allocations, at the moment I don't see mutable operations on the address byte slice.

[AskAlexSharov]

@Qjawko you are right (or unsafe or pointer to original data). anyway that zero-copy thing is just 2x saving. likely we need 10x or hard-limit.

[Qjawko]

I'll try to fix and add features we discussed above and profile again. Will post it here or in PR

[Qjawko]

I had an idea. Since we're converting an H160 address type to a 20-byte array, why not use the [20]byte type instead of the []byte type? This would reduce memory allocations and simplify the code.

func (sc *sendersBatch) onNewBlock(stateChanges *remote.StateChangeBatch, unwindTxs, minedTxs types.TxSlots) error {
	for _, diff := range stateChanges.ChangeBatch {
		for _, change := range diff.Changes { // merge state changes
			addrB := gointerfaces.ConvertH160toAddress(change.Address) <--- here
			sc.getOrCreateID(addrB[:]) 
		}

		for i, txn := range unwindTxs.Txs {
			txn.SenderID, txn.Traced = sc.getOrCreateID(unwindTxs.Senders.At(i))
		}

		for i, txn := range minedTxs.Txs {
			txn.SenderID, txn.Traced = sc.getOrCreateID(minedTxs.Senders.At(i))
		}
	}
	return nil
}

[AskAlexSharov]

addrB is already [20]byte - just need add method getOrCreateID( for this type

[Qjawko]

Sorry, I mean change it in senderBatch structure and methods, here is an example

type sendersBatch struct {
	senderIDs     map[[20]byte]uint64
	senderID2Addr map[uint64][20]byte
	tracedSenders map[[20]byte]struct{}
	senderID      uint64
}

[AskAlexSharov]

yes, senderID2Addr to map[uint64]common.Address
I don't have intuition about map[string] vs map[[20]byte] perf. Maybe it also will reduce RAM pressure. you will not do allocation, but will do copy of non-pointer type.

[AskAlexSharov]

I think: it's not common.Address only because minedTxs.Senders.At(i) return this type.
As I remember - now in Go can cast []byte to *[20]byte without allocation.

FYI: We keep compatibility with go 1.18 now. But can move to 1.19 if some super-useful feature is needed here.

[Qjawko]

I'm thinking that we should also change it to common.Address. If we do, there's no need move to version 1.19.

[AskAlexSharov]

@Qjawko see https://go.dev/doc/go1.17#

"change it to common.Address" i don't know will it make things better or worse. now minedTxs.Senders it's single []byte

[github-actions]

This issue is stale because it has been open for 40 days with no activity. Remove stale label or comment, or this will be closed in 7 days.

[github-actions]

This issue was closed because it has been stalled for 7 days with no activity.