Fix memory leak as transactions are continuously received.

[yongjiema]

Closes #509

[vncoelho]

Sounds very good, this has been in the list of pending things. Thanks for this good innitiave.

I will review and test asap.

[shargon]

Dictionary is not needed, i changed it to a simple List, we save more memory

[jsolman]

@shargon I disagree with your change to a list. Lookup is slow which is what is normally being done, and we want to hold 32K or more items.

[shargon]

Contains method is the same in a List and a Dictionary, we can use a SortedList or made some benchmarks, but one Dictionary require a Key and a Value, null as value is weird, is a waste of memory and resources ...

[shargon]

SortedList is based on a Dictionary too :(, we should create our SortedList if we want to improve the performance.

Maybe we should use a SortedSet

[vncoelho]

This PR closes #509.

Let's do the Bloom Filter...kkkkkkkkkkkkk
I am joking. Let's go in the current direction.

[shargon]

@jsolman what do you think like this?

using System;
using System.Linq;
using System.Collections.Generic;

namespace Neo.IO.Caching
{
    internal class FIFOSet<T> where T : IEquatable<T>
    {
        private int maxCapacity;
        private int removeCount;
        private SortedSet<T> orderedList;

        public FIFOSet(int maxCapacity, decimal? batchSize = 0.1m)
        {
            if (maxCapacity <= 0) throw new ArgumentOutOfRangeException(nameof(maxCapacity));
            if (batchSize <= 0) throw new ArgumentOutOfRangeException(nameof(batchSize));

            this.maxCapacity = maxCapacity;
            this.removeCount = batchSize != null ? (int)(batchSize <= 1.0m ? maxCapacity * batchSize : maxCapacity) : 1;
            this.orderedList = new SortedSet<T>();
        }

        public bool Add(T item)
        {
            if (orderedList.Contains(item)) return false;
            if (orderedList.Count >= maxCapacity)
            {
                if (removeCount == maxCapacity)
                {
                    orderedList.Clear();
                }
                else
                {
                    for (int i = 0; i < removeCount; i++)
                        orderedList.Remove(orderedList.First());
                }
            }
            orderedList.Add(item);
            return true;
        }
    }
}

[jsolman]

@shargon a hashset that has linked list nodes in the values is fastest and best. (SortedSet is not optimal for this use - it uses a red black tree)

[yongjiema]

Contains method is the same in a List and a Dictionary, we can use a SortedList or made some benchmarks, but one Dictionary require a Key and a Value, null as value is weird, is a waste of memory and resources ...

From the document and the source code, I can see it lookup from hashtable, that's why I chosen it.

[jsolman]

@yongjiema I think you made a reasonable choice

[jsolman]

IMO memory savings is trivial and we can just revert this: 27bf008

[shargon]

I will make some benchmarks

[shargon]

According to my tests, we should use a HashSet

BenchmarkDotNet=v0.11.3, OS=Windows 10.0.17134.648 (1803/April2018Update/Redstone4)
Intel Core i7-6700HQ CPU 2.60GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
Frequency=2531253 Hz, Resolution=395.0613 ns, Timer=TSC
.NET Core SDK=2.2.104
  [Host]     : .NET Core 2.2.2 (CoreCLR 4.6.27317.07, CoreFX 4.6.27318.02), 64bit RyuJIT
  DefaultJob : .NET Core 2.2.2 (CoreCLR 4.6.27317.07, CoreFX 4.6.27318.02), 64bit RyuJIT


            Method |         Mean |        Error |       StdDev |
------------------ |-------------:|-------------:|-------------:|
         SortedSet |     855.6 ms |    10.556 ms |     9.874 ms |
              List | 165,354.0 ms | 3,204.526 ms | 2,997.516 ms |
 OrderedDictionary |     295.0 ms |     3.159 ms |     2.638 ms |
           HashSet |     119.6 ms |     2.350 ms |     3.517 ms |

The test is like this:

 const int maxCapacity = 32_000;
        const int add = 64_000;

        UInt160[] samples = new UInt160[100_000];

        [GlobalSetup]
        public void Setup()
        {
            var random = new Random();

            for(var x = 0; x < samples.Length; x++)
            {
                var item = new byte[20];
                random.NextBytes(item);

                samples[x] = new UInt160(item);
            }
        }

        [Benchmark]
        public void SortedSet()
        {
            var list = new FIFOSortedSet<UInt160>(maxCapacity);
            for(var x = 0; x < add; x++)
            {
                list.Add(samples[x]);
            }
        }

[jsolman]

@shargon you cannot use only a hash set. You will not have fifo behavior with hashset on its own. You would have to benchmark hashset with list nodes in the values and handling the links (which is basically what OrderedDictionary is doing). The fact that ordered dictionaries holds references to values is likely not going to slow it down much so writing our own data structure will just save a small amount of memory.

[jsolman]

One thing to note though if we do write our own data structure that uses a hashset in combination with linked list nodes, we could use a singly linked list, since we only need the FIFO behavior, which will be even a little more space efficient than the OrderedDictionary that uses a doubly linked list internally.

[shargon]

I understand you ... then i will revert it to use OrderedDictionary but in the future we should use other methods because this one use more memory.

[erikzhang]

Anyone working on this?

[shargon]

If we decide to use 2x the mempool size, i can do it

[jsolman]

@shargon go for it

[shargon]

@yongjiema , please take a look at yongjiema#1

[erikzhang]

Any update?

The problem has been fixed.