Reuse Large Buffers in MigrateSession #623

vazois · 2024-08-29T22:26:28Z

This PR tries to improve memory utilization and reduce fragmentation by reusing large buffers that were allocated across different migrate sessions.
The PR includes the following:

Augment the network stack to support using separate send and receive buffer size allocation by providing separate buffer pool objects
Declare a shared NetworkBuffers object in MigrationManager and ReplicationManager in order to re-use allocated buffers across different scenarios.
Utilize the shared NetworkBuffers object to allocate buffer space for managing keys that actively being migrated.
Added PURGEBP command. Issuing PURGEBP [MM|RM] will attempt to release any buffer not being used in LFBP of the migration manager or replication manager.
Added INFO BPSTATS to list information about the shared buffer pool of the migration and replication managers.

Notes:

There is an upper limit on the number of entries per level in the LimitedBufferPool which may cause fragmentation of the LOH due to the way we allocate and return buffers to the pool itself (shown below)

garnet/libs/common/Memory/LimitedFixedBufferPool.cs

Lines 55 to 61 in 68a7a85

    
           if (Interlocked.Increment(ref pool[level].size) <= maxEntriesPerLevel) 
        
           { 
        
               Array.Clear(buffer.entry, 0, buffer.entry.Length); 
        
               pool[level].items.Enqueue(buffer); 
        
           } 
        
           else 
        
               Interlocked.Decrement(ref pool[level].size);

The default limit is 16 entries which should be enough for common scenarios (i.e. up to 16 parallel migrate sessions and up to 16 replication sessions).

Separate buffer pool from send and receive spec.
Remove unused parts of NetworkSenderBase.
Resize allocation for send/receive of replication code.

badrishc · 2024-09-19T00:05:53Z

libs/server/Metrics/Info/GarnetInfoMetrics.cs

@@ -224,15 +225,15 @@ private void PopulateObjectStoreStats(StoreWrapper storeWrapper)
                ];
        }

-        public void PopulateStoreHashDistribution(StoreWrapper storeWrapper) => storeHashDistrInfo = [new("", storeWrapper.store.DumpDistribution())];
+        private void PopulateStoreHashDistribution(StoreWrapper storeWrapper) => storeHashDistrInfo = [new("", storeWrapper.store.DumpDistribution())];


no need to explicitly call out "private" members in the codebase.

badrishc · 2024-09-19T00:13:28Z

libs/common/NetworkBuffers.cs

+    /// <summary>
+    /// Create a NetworkBuffers instance
+    /// </summary>
+    public struct NetworkBuffers


This struct does not do much, can create challenges with single-copy as it is a struct with value semantics, creates another level of indirection everywhere, and generally pervades across the codebase. The main thing it does is to use two allocation sizes, which can be maintained by callers or within LFBP itself.

badrishc

See comments.

badrishc · 2024-09-19T00:20:20Z

libs/cluster/Server/Migration/MigrationManager.cs

+        /// <summary>
+        /// Used to free up buffer pool
+        /// </summary>
+        public void Purge() => networkBuffers.Purge();


It can be dangerous to call potentially mutating methods (purge sounds mutating, although its implementation here is safe as it probably passes through into the LFBP class). As this could be a potential copy of the struct, it can create subtle bugs, as well as correctness and maintenance challenges.

badrishc · 2024-09-19T00:24:54Z

libs/common/LightClient.cs

@@ -212,7 +211,7 @@ public override void Dispose()
        {
            networkSender.ReturnResponseObject();
            networkHandler?.Dispose();
-            networkPool.Dispose();
+            networkBuffers.Dispose();


can't really reliably dispose structs.

TedHartMS · 2024-09-19T18:22:45Z

libs/cluster/Server/Failover/ReplicaFailoverSession.cs

@@ -77,6 +77,8 @@ private GarnetClient CreateConnection(string nodeId)
                address,
                port,
                clusterProvider.serverOptions.TlsOptions?.TlsClientOptions,
+                sendPageSize: 1 << 17,


This is the 3rd time (so far) I've seen the magic # 17, so best to make it a const int somewhere

TedHartMS · 2024-09-19T18:26:22Z

benchmark/Resp.benchmark/RespOnlineBench.cs

+                client = new GarnetClientSession(
+                    address,
+                    port,
+                    new(Math.Max(131072, opts.IntraThreadParallelism * opts.ValueLength)),


just new() by itself (with or without args) requires an extra step to figure it out. Better to prefix with param name or include the type.

TedHartMS · 2024-09-19T18:40:51Z

libs/cluster/Server/Migration/MigrationManager.cs

            this.clusterProvider = clusterProvider;
+            var bufferSize = 1 << clusterProvider.serverOptions.PageSizeBits();
+            this.networkBufferSettings = new NetworkBufferSettings(bufferSize, 1 << 12);


where does 12 come from? These magic numbers should be a "const long" or a config

TedHartMS · 2024-09-19T18:51:43Z

libs/cluster/Server/Replication/PrimaryOps/AofSyncTaskInfo.cs

@@ -46,8 +46,10 @@ public AofSyncTaskInfo(

        public void Dispose()
        {
-            iter?.Dispose();
+            cts.Cancel();


TedHartMS · 2024-09-19T18:53:44Z

libs/cluster/Server/Replication/ReplicationManager.cs

            this.clusterProvider = clusterProvider;
            this.storeWrapper = clusterProvider.storeWrapper;
+
+            this.networkBufferSettings = new NetworkBufferSettings(1 << 22, 1 << 12);


magic numberrrrrrrrs

TedHartMS · 2024-09-19T19:05:54Z

libs/common/Memory/LimitedFixedBufferPool.cs

@@ -61,8 +69,8 @@ public void Return(PoolEntry buffer)
                        Interlocked.Decrement(ref pool[level].size);


This needs commenting. My first thought on seeing .size was to wonder why we can't just use the queue.Count method. Now it looks like .size is total number of allocations? The PoolLevel field name comments are uninformative.

TedHartMS · 2024-09-19T19:13:57Z

libs/common/Memory/LimitedFixedBufferPool.cs

            {
 #if HANGDETECT
                    if (++count % 10000 == 0)
                        logger?.LogTrace("Dispose iteration {count}, {activeHandlerCount}", count, activeHandlerCount);
 #endif
                Thread.Yield();
            }
-            for (int i = 0; i < numLevels; i++)
+            for (var i = 0; i < numLevels; i++)
            {
                if (pool[i] == null) continue;
                while (pool[i].size > 0)


If I'm understanding correctly that .size is total # of allocations which may be > maxEntriesPerLevel, then this can spin forever if there are unReturned items. This might warrant an Assert or logging with an exit. (Holding an unReturned item and calling Dispose() (and Purge()?) is bad anyway, so let's make it easier to catch).

TedHartMS · 2024-09-19T19:26:25Z

libs/common/NetworkBuffers.cs

+        /// <summary>
+        /// Initial allocation size for receive network buffer.
+        /// (NOTE: Receive buffers can automatically grow to accomodate larger payloads.)
+        /// </summary>


Good comments. "Accommodate"

TedHartMS · 2024-09-19T19:33:22Z

libs/server/Resp/PurgeBPCommand.cs

+        /// MigrationManager Buffer Pool
+        /// </summary>
+        MM,
+        /// <summary>


Why not Migration, Replication, ServerSocket?

TedHartMS · 2024-09-19T19:34:29Z

libs/server/Resp/PurgeBPCommand.cs

+    internal sealed unsafe partial class RespServerSession : ServerSessionBase
+    {
+        private bool NetworkBurgeBP()
+        {


vazois force-pushed the vazois/migration-reuse-buffers branch 4 times, most recently from 29aa345 to 146da1a Compare September 4, 2024 20:26

vazois force-pushed the vazois/migration-reuse-buffers branch 3 times, most recently from e13374e to 9d65b87 Compare September 10, 2024 02:23

vazois marked this pull request as ready for review September 10, 2024 16:35

vazois marked this pull request as draft September 10, 2024 17:24

vazois force-pushed the vazois/migration-reuse-buffers branch 3 times, most recently from 18e9c50 to d34f522 Compare September 12, 2024 00:52

vazois marked this pull request as ready for review September 12, 2024 00:52

TalZaccai requested review from yrajas, TedHartMS and badrishc September 12, 2024 18:31

vazois force-pushed the vazois/migration-reuse-buffers branch 2 times, most recently from 2ab3868 to c362324 Compare September 17, 2024 23:34

vazois added 12 commits September 18, 2024 10:09

add buffer pool to migration manager

b2365e2

expose separate receive and send buffers from NetworkHandler

5cb9824

add network buffers wrapper to gcs

137ed1e

push NetworkBuffers to NetworkHandler

0f0c9c2

add networkBuffers instance in migration manager

ae4ee08

add constructor

5660bbf

reuse buffer for replication communication

466285b

consolidate network buffer pool

38aa84d

nit

7316873

add gc collect for migration buffer pool

96fbaa8

add ACL test for MIGRATEGC

1f27986

make migrategc management commands

d4a04ca

vazois added 15 commits September 18, 2024 10:09

expose maxEntries from NetworkBuffers.Allocate

88d90ed

configure GarnetClient for Failover

114c8ba

cleanup ReplicationManager

09bdac8

Cancel cts before disposing in AofSyncTask

c55512f

consolidate buffer pool purge under a single command

b4250b3

purge without disposing pool

e918085

introduce info bpstats metrics

5f2a8df

ensure shared bp uses correct allocation size for send and recv

d9f1129

fix bp stats call

663d717

change format of info bpstats

0362142

fix out of bounds level request for LFBP

016fe16

introduce purge for server buffer pool

fd5e71f

move PURGEBP in server namespace

9bfe50f

fix migrate bench keys option

5d9409f

fix formatting

dfc86d7

vazois force-pushed the vazois/migration-reuse-buffers branch from c362324 to dfc86d7 Compare September 18, 2024 17:10

add outOfBound allocation request metric

60e49cb

badrishc reviewed Sep 19, 2024

View reviewed changes

badrishc requested changes Sep 19, 2024

View reviewed changes

badrishc reviewed Sep 19, 2024

View reviewed changes

augment migrate bench

5649cd4

badrishc reviewed Sep 19, 2024

View reviewed changes

vazois added 5 commits September 19, 2024 10:48

separate pool definition from buffer spec

fed65a5

rename to NetworkBufferSettings

d77d1a8

rename NetworkBuffers to NetworkBufferSettings

a0dd73f

revert dispose order

e918347

revert replication networkSettings

3f8a52e

TedHartMS requested changes Sep 19, 2024

View reviewed changes

add timeout to dispose of LFBP

0ade0d9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse Large Buffers in MigrateSession #623

Reuse Large Buffers in MigrateSession #623

vazois commented Aug 29, 2024 •

edited

Loading

badrishc Sep 19, 2024

badrishc Sep 19, 2024 •

edited

Loading

badrishc left a comment

badrishc Sep 19, 2024

badrishc Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

TedHartMS Sep 19, 2024

	if (Interlocked.Increment(ref pool[level].size) <= maxEntriesPerLevel)
	{
	Array.Clear(buffer.entry, 0, buffer.entry.Length);
	pool[level].items.Enqueue(buffer);
	}
	else
	Interlocked.Decrement(ref pool[level].size);

		@@ -61,8 +69,8 @@ public void Return(PoolEntry buffer)
		Interlocked.Decrement(ref pool[level].size);

Reuse Large Buffers in MigrateSession #623

Are you sure you want to change the base?

Reuse Large Buffers in MigrateSession #623

Conversation

vazois commented Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

badrishc Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

badrishc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vazois commented Aug 29, 2024 •

edited

Loading

badrishc Sep 19, 2024 •

edited

Loading