Reducing clientHandler memory footprint

[fclairamb]

Mostly for @drakkan (but everyone else is welcome),

With #334 we are adding one more field to the clientHandler struct.

And many of these fields are optional ones, in the sense that they are frequently never filled and could be used to a default value:

• clnt can often be unspecified
• ctxRnfr is used during the renaming
• ctxRest is used in the scope of file write restart
• debug is used only when we need debugging
• transferTLS and controlTLS
• selectedHashAlgo will often be not specified because HASH won't be used

So I wonder if using a map[string]interface{} or map[int]interface{} wouldn't be a good option to not consume memory for parameters that are usually not specified.

[drakkan]

Assuming the following memory allocations:

• int 8 bytes
• string 16 bytes
• bool, 1 byte
• int8, 1 byte

removing the indicated fields, the size of the struct should be reduced by 51 bytes (without considering the alignment), initializing a map should allocate 48 bytes, so we save 3 bytes for each connected client. If we'll add other fields to the clientHandler struct the memory save will increase in the future.

If we switch HASHAlgo from int to int8 we'll save 7 bytes (we can do the same for TLSRequirement, TransferType and DataChannel if we want to reduce memory allocation), so moving the parameters above to a map should increase memory usage in this case. The tests below show that the allocation does not change for the struct alignment.

I did some quick benchmarks

type clientHandlerWithMap struct {
	id                  uint32          // ID of the client
	server              *FtpServer      // Server on which the connection was accepted
	driver              ClientDriver    // Client handling driver
	conn                net.Conn        // TCP connection
	writer              *bufio.Writer   // Writer on the TCP connection
	reader              *bufio.Reader   // Reader on the TCP connection
	user                string          // Authenticated user
	path                string          // Current path
	command             string          // Command received on the connection
	connectedAt         time.Time       // Date of connection
	logger              log.Logger      // Client handler logging
	currentTransferType TransferType    // current transfer type
	transferWg          sync.WaitGroup  // wait group for command that open a transfer connection
	transferMu          sync.Mutex      // this mutex will protect the transfer parameters
	transfer            transferHandler // Transfer connection (passive or active)s
	lastDataChannel     DataChannel     // Last data channel mode (passive or active)
	isTransferOpen      bool            // indicate if the transfer connection is opened
	isTransferAborted   bool            // indicate if the transfer was aborted
	tlsRequirement      TLSRequirement  // TLS requirement to respect
	paramsMap           map[int]any
	paramsMutex         sync.RWMutex // mutex to protect the parameters exposed to the library users
}

// GetLastCommand returns the last received command
func (c *clientHandlerWithMap) GetLastCommand() string {
	c.paramsMutex.RLock()
	defer c.paramsMutex.RUnlock()

	return c.command
}

func BenchmarkClientHandler(b *testing.B) {
	for i := 0; i < b.N; i++ {
		handler := clientHandler{
			id:                  uint32(i),
			connectedAt:         time.Now().UTC(),
			path:                "/",
			currentTransferType: TransferTypeBinary,
		}
		handler.GetLastCommand()
	}
}

func BenchmarkClientHandlerWithMap(b *testing.B) {
	for i := 0; i < b.N; i++ {
		handler := clientHandlerWithMap{
			id:                  uint32(i),
			connectedAt:         time.Now().UTC(),
			path:                "/",
			currentTransferType: TransferTypeBinary,
			paramsMap:           make(map[int]any),
		}
		handler.GetLastCommand()
	}
}

Here are the results (after converting from int to int8 all the enum fields mentioned above):

BenchmarkClientHandler-12    	 6868653	       198.5 ns/op	     288 B/op	       1 allocs/op

BenchmarkClientHandlerWithMap-12    	 5745822	       293.9 ns/op	     288 B/op	       2 allocs/op

so no real benefit.

If I don't convert the enum from int to int8 we have these results (clientHandlerWithMap is better aligned, so 16 bytes are saved):

BenchmarkClientHandler-12    	 7595534	       251.1 ns/op	     320 B/op	       1 allocs/op

BenchmarkClientHandlerWithMap-12    	 5613898	       302.7 ns/op	     304 B/op	       2 allocs/op

Based on these results, my opinion is it doesn't matter (at least for now).

However, if you prefer, I have no problem to send a patch that adds this feature.
We could also convert enums to int8. Let me know and thank you for your continued support.

P.S. I'm on a laptop ns/op will vary between runs, sometime ns/op is smaller for BenchmarkClientHandlerWithMap than BenchmarkClientHandler

[fclairamb]

That's the right way to look at it, the added complexity is not worth it.

[drakkan]

That's the right way to look at it, the added complexity is not worth it.

I agree, do you want a patch to use int8 for enums? This way we reduce the allocated memory from 320 bytes to 288 bytes as you can see from the above benchmark