From 8cf114175a140ba1cc8c9996e6e04a1c2fe81c6f Mon Sep 17 00:00:00 2001
From: benaadams <thundercat@illyriad.co.uk>
Date: Thu, 9 Feb 2023 11:17:43 +0000
Subject: [PATCH] Feedback

---
 .../Nethermind.Core/Caching/LinkedListNode.cs | 62 ++++++++++++++
 .../Nethermind.Core/Caching/LruCache.cs       | 79 +++--------------
 .../Nethermind.Core/Caching/LruKeyCache.cs    | 85 +++----------------
 .../Caching/MemCountingCache.cs               | 82 ++++--------------
 4 files changed, 103 insertions(+), 205 deletions(-)

diff --git a/src/Nethermind/Nethermind.Core/Caching/LinkedListNode.cs b/src/Nethermind/Nethermind.Core/Caching/LinkedListNode.cs
index 4724702e8d4..c1ae9158f78 100644
--- a/src/Nethermind/Nethermind.Core/Caching/LinkedListNode.cs
+++ b/src/Nethermind/Nethermind.Core/Caching/LinkedListNode.cs
@@ -2,6 +2,8 @@
 // SPDX-License-Identifier: LGPL-3.0-only
 
 using System.Collections.Generic;
+using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 
 namespace Nethermind.Core.Caching;
 
@@ -15,4 +17,64 @@ public LinkedListNode(T value)
     {
         Value = value;
     }
+    
+    public static void MoveToMostRecent([NotNull] ref LinkedListNode<T>? leastRecentlyUsed, LinkedListNode<T> node)
+    {
+        if (node.Next == node)
+        {
+            Debug.Assert(leastRecentlyUsed == node, "this should only be true for a list with only one node");
+            // Do nothing only one node
+        }
+        else
+        {
+            Remove(ref leastRecentlyUsed, node);
+            AddMostRecent(ref leastRecentlyUsed, node);
+        }
+    }
+
+    public static void Remove(ref LinkedListNode<T>? leastRecentlyUsed, LinkedListNode<T> node)
+    {
+        Debug.Assert(leastRecentlyUsed is not null, "This method shouldn't be called on empty list!");
+        if (node.Next == node)
+        {
+            Debug.Assert(leastRecentlyUsed == node, "this should only be true for a list with only one node");
+            leastRecentlyUsed = null;
+        }
+        else
+        {
+            node.Next!.Prev = node.Prev;
+            node.Prev!.Next = node.Next;
+            if (ReferenceEquals(leastRecentlyUsed, node))
+            {
+                leastRecentlyUsed = node.Next;
+            }
+        }
+    }
+
+    public static void AddMostRecent([NotNull] ref LinkedListNode<T>? leastRecentlyUsed, LinkedListNode<T> node)
+    {
+        if (leastRecentlyUsed is null)
+        {
+            SetFirst(ref leastRecentlyUsed, node);
+        }
+        else
+        {
+            InsertMostRecent(leastRecentlyUsed, node);
+        }
+    }
+
+    private static void InsertMostRecent(LinkedListNode<T> leastRecentlyUsed, LinkedListNode<T> newNode)
+    {
+        newNode.Next = leastRecentlyUsed;
+        newNode.Prev = leastRecentlyUsed.Prev;
+        leastRecentlyUsed.Prev!.Next = newNode;
+        leastRecentlyUsed.Prev = newNode;
+    }
+
+    private static void SetFirst([NotNull] ref LinkedListNode<T>? leastRecentlyUsed, LinkedListNode<T> newNode)
+    {
+        newNode.Next = newNode;
+        newNode.Prev = newNode;
+        leastRecentlyUsed = newNode;
+    }
 }
diff --git a/src/Nethermind/Nethermind.Core/Caching/LruCache.cs b/src/Nethermind/Nethermind.Core/Caching/LruCache.cs
index 0bf1788f72e..5c9ed57158c 100644
--- a/src/Nethermind/Nethermind.Core/Caching/LruCache.cs
+++ b/src/Nethermind/Nethermind.Core/Caching/LruCache.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Runtime.CompilerServices;
 using Nethermind.Core.Extensions;
@@ -47,7 +48,7 @@ public TValue Get(TKey key)
             if (_cacheMap.TryGetValue(key, out LinkedListNode<LruCacheItem>? node))
             {
                 TValue value = node.Value.Value;
-                MoveToMostRecent(node);
+                LinkedListNode<LruCacheItem>.MoveToMostRecent(ref _leastRecentlyUsed, node);
                 return value;
             }
 
@@ -63,7 +64,7 @@ public bool TryGet(TKey key, out TValue value)
             if (_cacheMap.TryGetValue(key, out LinkedListNode<LruCacheItem>? node))
             {
                 value = node.Value.Value;
-                MoveToMostRecent(node);
+                LinkedListNode<LruCacheItem>.MoveToMostRecent(ref _leastRecentlyUsed, node);
                 return true;
             }
 
@@ -85,7 +86,7 @@ public bool Set(TKey key, TValue val)
             if (_cacheMap.TryGetValue(key, out LinkedListNode<LruCacheItem>? node))
             {
                 node.Value.Value = val;
-                MoveToMostRecent(node);
+                LinkedListNode<LruCacheItem>.MoveToMostRecent(ref _leastRecentlyUsed, node);
                 return false;
             }
             else
@@ -97,7 +98,7 @@ public bool Set(TKey key, TValue val)
                 else
                 {
                     LinkedListNode<LruCacheItem> newNode = new(new(key, val));
-                    AddMostRecent(newNode);
+                    LinkedListNode<LruCacheItem>.AddMostRecent(ref _leastRecentlyUsed, newNode);
                     _cacheMap.Add(key, newNode);
                 }
 
@@ -110,7 +111,7 @@ public bool Delete(TKey key)
         {
             if (_cacheMap.TryGetValue(key, out LinkedListNode<LruCacheItem>? node))
             {
-                Remove(node);
+                LinkedListNode<LruCacheItem>.Remove(ref _leastRecentlyUsed, node);
                 _cacheMap.Remove(key);
                 return true;
             }
@@ -129,76 +130,20 @@ private void Replace(TKey key, TValue value)
             LinkedListNode<LruCacheItem>? node = _leastRecentlyUsed;
             if (node is null)
             {
-                throw new InvalidOperationException(
-                    $"{nameof(LruCache<TKey, TValue>)} called {nameof(Replace)} when empty.");
+                ThrowInvalidOperationException();
             }
 
             _cacheMap.Remove(node!.Value.Key);
 
             node.Value = new(key, value);
-            MoveToMostRecent(node);
+            LinkedListNode<LruCacheItem>.MoveToMostRecent(ref _leastRecentlyUsed, node);
             _cacheMap.Add(key, node);
-        }
 
-        private void MoveToMostRecent(LinkedListNode<LruCacheItem> node)
-        {
-            if (node.Next == node)
+            [DoesNotReturn]
+            static void ThrowInvalidOperationException()
             {
-                Debug.Assert(_cacheMap.Count == 1 && _leastRecentlyUsed == node, "this should only be true for a list with only one node");
-                // Do nothing only one node
-            }
-            else
-            {
-                Remove(node);
-                AddMostRecent(node);
-            }
-        }
-
-        private void AddMostRecent(LinkedListNode<LruCacheItem> node)
-        {
-            if (_leastRecentlyUsed is null)
-            {
-                SetFirst(node);
-            }
-            else
-            {
-                InsertMostRecent(node);
-            }
-        }
-
-        private void InsertMostRecent(LinkedListNode<LruCacheItem> newNode)
-        {
-            LinkedListNode<LruCacheItem> first = _leastRecentlyUsed!;
-            newNode.Next = first;
-            newNode.Prev = first.Prev;
-            first.Prev!.Next = newNode;
-            first.Prev = newNode;
-        }
-
-        private void SetFirst(LinkedListNode<LruCacheItem> newNode)
-        {
-            Debug.Assert(_leastRecentlyUsed is null && _cacheMap.Count == 0, "LinkedList must be empty when this method is called!");
-            newNode.Next = newNode;
-            newNode.Prev = newNode;
-            _leastRecentlyUsed = newNode;
-        }
-
-        private void Remove(LinkedListNode<LruCacheItem> node)
-        {
-            Debug.Assert(_leastRecentlyUsed is not null, "This method shouldn't be called on empty list!");
-            if (node.Next == node)
-            {
-                Debug.Assert(_cacheMap.Count == 1 && _leastRecentlyUsed == node, "this should only be true for a list with only one node");
-                _leastRecentlyUsed = null;
-            }
-            else
-            {
-                node.Next!.Prev = node.Prev;
-                node.Prev!.Next = node.Next;
-                if (_leastRecentlyUsed == node)
-                {
-                    _leastRecentlyUsed = node.Next;
-                }
+                throw new InvalidOperationException(
+                    $"{nameof(LruCache<TKey, TValue>)} called {nameof(Replace)} when empty.");
             }
         }
 
diff --git a/src/Nethermind/Nethermind.Core/Caching/LruKeyCache.cs b/src/Nethermind/Nethermind.Core/Caching/LruKeyCache.cs
index c21992806a7..775aea8d68e 100644
--- a/src/Nethermind/Nethermind.Core/Caching/LruKeyCache.cs
+++ b/src/Nethermind/Nethermind.Core/Caching/LruKeyCache.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 using Nethermind.Core.Extensions;
 
@@ -42,7 +43,7 @@ public bool Get(TKey key)
         {
             if (_cacheMap.TryGetValue(key, out LinkedListNode<TKey>? node))
             {
-                MoveToMostRecent(node);
+                LinkedListNode<TKey>.MoveToMostRecent(ref _leastRecentlyUsed, node);
                 return true;
             }
 
@@ -54,7 +55,7 @@ public bool Set(TKey key)
         {
             if (_cacheMap.TryGetValue(key, out LinkedListNode<TKey>? node))
             {
-                MoveToMostRecent(node);
+                LinkedListNode<TKey>.MoveToMostRecent(ref _leastRecentlyUsed, node);
                 return false;
             }
             else
@@ -66,7 +67,7 @@ public bool Set(TKey key)
                 else
                 {
                     LinkedListNode<TKey> newNode = new(key);
-                    AddMostRecent(newNode);
+                    LinkedListNode<TKey>.AddMostRecent(ref _leastRecentlyUsed, newNode);
                     _cacheMap.Add(key, newNode);
                 }
 
@@ -79,88 +80,30 @@ public void Delete(TKey key)
         {
             if (_cacheMap.TryGetValue(key, out LinkedListNode<TKey>? node))
             {
-                Remove(node);
+                LinkedListNode<TKey>.Remove(ref _leastRecentlyUsed, node);
                 _cacheMap.Remove(key);
             }
         }
 
-        private void MoveToMostRecent(LinkedListNode<TKey> node)
-        {
-            if (node.Next == node)
-            {
-                Debug.Assert(_cacheMap.Count == 1 && _leastRecentlyUsed == node, "this should only be true for a list with only one node");
-                // Do nothing only one node
-            }
-            else
-            {
-                Remove(node);
-                AddMostRecent(node);
-            }
-        }
-
-        private void AddMostRecent(LinkedListNode<TKey> node)
-        {
-            if (_leastRecentlyUsed is null)
-            {
-                SetFirst(node);
-            }
-            else
-            {
-                InsertMostRecent(node);
-            }
-        }
-
-        private void InsertMostRecent(LinkedListNode<TKey> newNode)
-        {
-            LinkedListNode<TKey> first = _leastRecentlyUsed!;
-            newNode.Next = first;
-            newNode.Prev = first.Prev;
-            first.Prev!.Next = newNode;
-            first.Prev = newNode;
-        }
-
-        private void SetFirst(LinkedListNode<TKey> newNode)
-        {
-            Debug.Assert(_leastRecentlyUsed is null && _cacheMap.Count == 0, "LinkedList must be empty when this method is called!");
-            newNode.Next = newNode;
-            newNode.Prev = newNode;
-            _leastRecentlyUsed = newNode;
-        }
-
-        private void Remove(LinkedListNode<TKey> node)
-        {
-            Debug.Assert(_leastRecentlyUsed is not null, "This method shouldn't be called on empty list!");
-            if (node.Next == node)
-            {
-                Debug.Assert(_cacheMap.Count == 1 && _leastRecentlyUsed == node, "this should only be true for a list with only one node");
-                _leastRecentlyUsed = null;
-            }
-            else
-            {
-                node.Next!.Prev = node.Prev;
-                node.Prev!.Next = node.Next;
-                if (_leastRecentlyUsed == node)
-                {
-                    _leastRecentlyUsed = node.Next;
-                }
-            }
-        }
-
         private void Replace(TKey key)
         {
-            // TODO: some potential null ref issue here?
-
             LinkedListNode<TKey>? node = _leastRecentlyUsed;
             if (node is null)
             {
-                throw new InvalidOperationException(
-                    $"{nameof(LruKeyCache<TKey>)} called {nameof(Replace)} when empty.");
+                ThrowInvalidOperation();
             }
 
             _cacheMap.Remove(node.Value);
             node.Value = key;
-            MoveToMostRecent(node);
+            LinkedListNode<TKey>.MoveToMostRecent(ref _leastRecentlyUsed, node);
             _cacheMap.Add(key, node);
+
+            [DoesNotReturn]
+            static void ThrowInvalidOperation()
+            {
+                throw new InvalidOperationException(
+                                    $"{nameof(LruKeyCache<TKey>)} called {nameof(Replace)} when empty.");
+            }
         }
 
         public long MemorySize => CalculateMemorySize(0, _cacheMap.Count);
diff --git a/src/Nethermind/Nethermind.Core/Caching/MemCountingCache.cs b/src/Nethermind/Nethermind.Core/Caching/MemCountingCache.cs
index 0c3106305cf..d89dfcd680d 100644
--- a/src/Nethermind/Nethermind.Core/Caching/MemCountingCache.cs
+++ b/src/Nethermind/Nethermind.Core/Caching/MemCountingCache.cs
@@ -3,7 +3,7 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 using Nethermind.Core.Crypto;
 
@@ -50,7 +50,7 @@ public MemCountingCache(int maxCapacity, string name)
             if (_cacheMap.TryGetValue(key, out LinkedListNode<LruCacheItem>? node))
             {
                 byte[] value = node.Value.Value;
-                MoveToMostRecent(node);
+                LinkedListNode<LruCacheItem>.MoveToMostRecent(ref _leastRecentlyUsed, node);
                 return value;
             }
 
@@ -63,7 +63,7 @@ public bool TryGet(KeccakKey key, out byte[]? value)
             if (_cacheMap.TryGetValue(key, out LinkedListNode<LruCacheItem>? node))
             {
                 value = node.Value.Value;
-                MoveToMostRecent(node);
+                LinkedListNode<LruCacheItem>.MoveToMostRecent(ref _leastRecentlyUsed, node);
                 return true;
             }
 
@@ -82,7 +82,7 @@ public bool Set(KeccakKey key, byte[]? val)
             if (_cacheMap.TryGetValue(key, out LinkedListNode<LruCacheItem>? node))
             {
                 node.Value.Value = val;
-                MoveToMostRecent(node);
+                LinkedListNode<LruCacheItem>.MoveToMostRecent(ref _leastRecentlyUsed, node);
                 return false;
             }
             else
@@ -104,7 +104,7 @@ public bool Set(KeccakKey key, byte[]? val)
                 {
                     MemorySize = newMemorySize;
                     LinkedListNode<LruCacheItem> newNode = new(new(key, val));
-                    AddMostRecent(newNode);
+                    LinkedListNode<LruCacheItem>.AddMostRecent(ref _leastRecentlyUsed, newNode);
                     _cacheMap.Add(key, newNode);
                 }
                 else
@@ -123,7 +123,7 @@ public bool Delete(KeccakKey key)
             if (_cacheMap.TryGetValue(key, out LinkedListNode<LruCacheItem>? node))
             {
                 MemorySize -= node.Value.MemorySize;
-                Remove(node);
+                LinkedListNode<LruCacheItem>.Remove(ref _leastRecentlyUsed, node);
                 _cacheMap.Remove(key);
 
                 return true;
@@ -138,7 +138,10 @@ public bool Delete(KeccakKey key)
         private void Replace(KeccakKey key, byte[] value)
         {
             LinkedListNode<LruCacheItem> node = _leastRecentlyUsed!;
-            Debug.Assert(node != null);
+            if (node is null)
+            {
+                ThrowInvalidOperation();
+            }
 
             // ReSharper disable once PossibleNullReferenceException
             MemorySize += MemorySizes.Align(value.Length) - MemorySizes.Align(node.Value.Value.Length);
@@ -146,69 +149,14 @@ private void Replace(KeccakKey key, byte[] value)
             _cacheMap.Remove(node!.Value.Key);
 
             node.Value = new(key, value);
-            MoveToMostRecent(node);
+            LinkedListNode<LruCacheItem>.MoveToMostRecent(ref _leastRecentlyUsed, node);
             _cacheMap.Add(key, node);
-        }
-
-        private void MoveToMostRecent(LinkedListNode<LruCacheItem> node)
-        {
-            if (node.Next == node)
-            {
-                Debug.Assert(_cacheMap.Count == 1 && _leastRecentlyUsed == node, "this should only be true for a list with only one node");
-                // Do nothing only one node
-            }
-            else
-            {
-                Remove(node);
-                AddMostRecent(node);
-            }
-        }
 
-        private void AddMostRecent(LinkedListNode<LruCacheItem> node)
-        {
-            if (_leastRecentlyUsed is null)
+            [DoesNotReturn]
+            static void ThrowInvalidOperation()
             {
-                SetFirst(node);
-            }
-            else
-            {
-                InsertMostRecent(node);
-            }
-        }
-
-        private void InsertMostRecent(LinkedListNode<LruCacheItem> newNode)
-        {
-            LinkedListNode<LruCacheItem> first = _leastRecentlyUsed!;
-            newNode.Next = first;
-            newNode.Prev = first.Prev;
-            first.Prev!.Next = newNode;
-            first.Prev = newNode;
-        }
-
-        private void SetFirst(LinkedListNode<LruCacheItem> newNode)
-        {
-            Debug.Assert(_leastRecentlyUsed is null && _cacheMap.Count == 0, "LinkedList must be empty when this method is called!");
-            newNode.Next = newNode;
-            newNode.Prev = newNode;
-            _leastRecentlyUsed = newNode;
-        }
-
-        private void Remove(LinkedListNode<LruCacheItem> node)
-        {
-            Debug.Assert(_leastRecentlyUsed is not null, "This method shouldn't be called on empty list!");
-            if (node.Next == node)
-            {
-                Debug.Assert(_cacheMap.Count == 1 && _leastRecentlyUsed == node, "this should only be true for a list with only one node");
-                _leastRecentlyUsed = null;
-            }
-            else
-            {
-                node.Next!.Prev = node.Prev;
-                node.Prev!.Next = node.Next;
-                if (_leastRecentlyUsed == node)
-                {
-                    _leastRecentlyUsed = node.Next;
-                }
+                throw new InvalidOperationException(
+                                    $"{nameof(MemCountingCache)} called {nameof(Replace)} when empty.");
             }
         }