Remove 14MB from RunPeerCommit allocations

[benaadams]

Rebased #5271 as was having issues with github 🤔

Changes

• Only full refresh and parse from DB when nodes change
• Don't allocate fresh node prior to checking if node already exists (compare prior rather then post)
• Reduces by about ~10MB/min (see trace below)

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

[benaadams]

Not sure why this change would crash the Test host process; isn't doing anything wild 🤔

[LukaszRozmej]

Does it have to return an Array? Can it just return IReadOnlyCollection and we could leverage the list itself? We could avoid storing array and list and allocating array all the time.

[benaadams]

Recreating the array gets around the concurrency problem of the list being used while the list is being adjusted since its used in a few places and the nodes are created via events from the network; so it would need tighter synchronisation on the iteration in DiscoveryApp.AddPersistedNodes; NodesLoader.LoadPeersFromDb and PeerPool.UpdateReputationAndMaxPeersCount() with the node creation form ProtocolsManager.InitSyncPeerProtocol; DiscoveryApp.RunDiscoveryCommit(); DiscoveryManager.GetNodeLifecycleManager and removal from DiscoveryManager.OnIncomingMsg and PeerPool.CleanupPersistedPeers

It doesn't have the issue currently as it completely recreates the NetworkNode[] from scratch from the DB each time the method is called; whereas here its reused form most method calls (and it only reads from the DB once for app lifetime, though it continues to write changes to it, but updates in memory structure instead of reading back)

tl;dr yes it could, but is complicated due to the concurrency

[LukaszRozmej]

Fair enough, but you could have 2 lists that you would only copy from one to the other without any allocations or GC.

Or maybe we can do without copying and just exchange them?

[benaadams]

🤔 could work; though use of the other list isn't guaranteed to have finished before starting to repopulate it

However thinking about that, could maybe get away with one List by taking a bounded Span reference via CollectionsMarshal.AsSpan<T>(List<T>); then adding some null checks when the items are returned and skipping them. Would need to look into it a bit more as might be a bit wild 😅

[LukaszRozmej]

We have this two list approach in TxBroadcaster with _accumulatedTemporaryTxs and _txsToSend

[benaadams]

Looks like its a single threaded reader and when the method is re-entered you know the iteration is complete? (e.g. timer fire) so safe; whereas here its read and written from a bunch of sources?

[benaadams]

Tried the 2 approach + Span, but is tricky to workout when to switch without it immediately switching back (e.g. UpdateNodes(IEnumerable<NetworkNode> nodes) calls UpdateNode(NetworkNode node) multiple times); and the Span still suffers from nodes being removed in the middle of the list, so skipping nodes 🤔

[benaadams]

Probably better to look at in future PR?

[LukaszRozmej]

We could potentially reuse already existing nodes instead of creating new ones all the time

[benaadams]

RemoveNode and UpdateNode don't set the _nodes array to null but just replace it so GenerateNodes() should only be creating the nodes from scratch once during app lifetime (i.e. this bit of code) and UpdateNode gets passed in the preconstructed NetworkNode so it would require a fair bit of refactoring to reuse the nodes from RemoveNode 🤔

[LukaszRozmej]

So rlp starts with public key, which could be decoded on stack, then it could be reused from existing node instead of creating new one.

We can say it is out of scope of this PR and try to do it in some next one if ever - but sounds doable.

[benaadams]

Yeah I think would be good to look at that; as well as wider, as are a lot of temporary byte[] created for Rlp only to be fed into KeccakHash.Compute then thrown away; but the temp byte[] could be bypassed by feeding direct into KeccakHash which would also reduce copies by one

[benaadams]

Rebased

[benaadams]

Not sure why this test failed

  Failed Should_count_total_coinbase_payments [435 ms]
  Error Message:
   Expected object to be equal to 100000000000, but found 200000000000.

MevRpc doesn't involve the peer nodes? 🤔

[benaadams]

Rebased again and squashed commits to see if persistent