[DO NOT MERGE] Refactor - Move command execution responsibility to CommandHandler from IOThread #1358

psrvere · 2024-12-04T11:13:46Z

TODO
[ ] Investigate CopyTest failure
[ ] Investigate SDK tests failure

This PR is a preparatory step towards solving nearly 50-60% CPU utilisation due to context shift between user space and kernel space for frequent read and write calls.

As first step, this PR moves command execution logic out of IOThread to a new entity CommandHandler. IOThread is now responsible for reading from and writing to the client connection, and CommandHandler is responsible for execution of command by interacting with ShardManager and WatchManager.

Currently, there is 1:1 mapping between IOThread and CommandHandler, and both of them are spawned by Resp Server. IOThread and CommandHandler communicate using 3 channels

ioThreadReadChan - to send command from IOThread to CommandHandler
ioThreadWriteChan - to send response from CommandHandler to IOThread
ioThreadErrChan - to send connection error signal from IOThread to CommandHandler, so that CommandHandler shuts down when IOHandler does

…l exit

psrvere · 2024-12-05T11:56:28Z

@soumya-codes @lucifercr07 - please review.

soumya-codes

The code changes look great Prateek. I have done a lot of nit picking around naming convention. Please feel free to open a separate PR to address them.

soumya-codes · 2024-12-06T13:27:44Z

config/config.go

@@ -136,6 +136,7 @@ type performance struct {
 	ShardCronFrequency     time.Duration `config:"shard_cron_frequency" default:"1s"`
 	MultiplexerPollTimeout time.Duration `config:"multiplexer_poll_timeout" default:"100ms"`
 	MaxClients             int32         `config:"max_clients" default:"20000" validate:"min=0"`
+	MaxCmdHandlers         int32         `config:"max_cmd_handlers" default:"20000" validate:"min=0"`


Isn't this same as MaxClients?
Also since we do not plan to have more than 65536 client's can we have the type as int16 instead?

soumya-codes · 2024-12-07T14:04:21Z

internal/commandhandler/manager.go

+	"github.com/dicedb/dice/internal/shard"
+)
+
+type Manager struct {


Given the behaviour, I think we got naming convention for Manager wrong in DiceDB. What do you think about Registrar/Registry?

soumya-codes · 2024-12-07T14:05:57Z

internal/commandhandler/manager.go

+func (m *Manager) UnregisterCommandHandler(id string) error {
+	m.ShardManager.UnregisterCommandHandler(id)
+	if cmdHandler, loaded := m.activeCmdHandlers.LoadAndDelete(id); loaded {
+		ch := cmdHandler.(*BaseCommandHandler)


We can use a comma, ok idiom here to be safe. Not needed here, but it will be failproof for future code changes.

soumya-codes · 2024-12-07T14:55:11Z

internal/commandhandler/commandhandler.go

+	id                       string
+	parser                   requestparser.Parser
+	shardManager             *shard.ShardManager
+	adhocReqChan             chan *cmd.DiceDBCmd


Since you are refactoring this I would request you to change the name of adhocReqChan field to something more meaningful like watchReqChan.

soumya-codes · 2024-12-07T14:57:45Z

internal/commandhandler/commandhandler.go

+}
+
+type BaseCommandHandler struct {
+	CommandHandler


Please make sure the structures are aligned in a way that they add lowest memory footprint..

soumya-codes · 2024-12-07T17:13:33Z

internal/commandhandler/cmd_custom.go

@@ -12,7 +12,7 @@ import (

 // RespAuth returns with an encoded "OK" if the user is authenticated
 // If the user is not authenticated, it returns with an encoded error message
-func (t *BaseIOThread) RespAuth(args []string) interface{} {
+func (h *BaseCommandHandler) RespAuth(args []string) interface{} {


I would suggest we move this to the commandhandler.go file.

soumya-codes · 2024-12-09T07:17:20Z

internal/iothread/iothread.go

-const defaultRequestTimeout = 6 * time.Second
-
-var requestCounter uint32
-
 // IOThread interface
 type IOThread interface {


I like the Handler naming convention. It follows Go naming convention, where the data-structure names are encouraged to be verbs name. Based on this should we rename IOThread to IOHandler?

soumya-codes · 2024-12-09T07:18:56Z

internal/iothread/manager.go

@@ -21,10 +18,9 @@ var (
 	ErrIOThreadNotFound  = errors.New("io-thread not found")
 )

-func NewManager(maxClients int32, sm *shard.ShardManager) *Manager {
+func NewManager(maxClients int32) *Manager {


Same as earlier, what do you think of naming Manager to Registar/Registry? This name provides the precise nature of the work done by this type.

soumya-codes · 2024-12-09T07:22:01Z

internal/shard/shard_manager.go

@@ -104,15 +104,16 @@ func (manager *ShardManager) GetShard(id ShardID) *ShardThread {
 	return nil
 }

-// RegisterIOThread registers a io-thread with all Shards present in the ShardManager.
-func (manager *ShardManager) RegisterIOThread(id string, request, processing chan *ops.StoreResponse) {
+// RegisterCommandHandler registers a command handler with all Shards present in the ShardManager.


Same as earlier, what do you think of naming Manager to Registar/Registry? This name provides the precise nature of the work done by this type.

lucifercr07

LGTM, minor comments.

lucifercr07 · 2024-12-09T16:57:27Z

internal/server/resp/server.go

 func GenerateUniqueIOThreadID() string {
 	count := atomic.AddUint64(&ioThreadCounter, 1)
 	timestamp := time.Now().UnixNano()/int64(time.Millisecond) - startTime
 	return fmt.Sprintf("W-%d-%d", timestamp, count)
 }

+func GenerateUniqueCommandHandlerID() string {
+	count := atomic.AddUint64(&cmdHandlerCounter, 1)
+	timestamp := time.Now().UnixNano()/int64(time.Millisecond) - startTime
+	return fmt.Sprintf("W-%d-%d", timestamp, count)
+}
+


Shall we optimise this as below? Also any reasons why we're keeping ID format for cmdHandler and IOThread same?

func GenerateUniqueID(prefix string, counter *uint64) string { count := atomic.AddUint64(counter, 1) timestamp := time.Now().UnixMilli() - startTime return fmt.Sprintf("%s-%d-%d", prefix, timestamp, count) } func GenerateUniqueIOThreadID() string { return GenerateUniqueID("W", &ioThreadCounter) } func GenerateUniqueCommandHandlerID() string { return GenerateUniqueID("W", &cmdHandlerCounter) }

lucifercr07 · 2024-12-09T17:18:22Z

internal/commandhandler/manager.go

+	m.mu.Lock()
+	defer m.mu.Unlock()
+
+	if m.CommandHandlerCount() >= m.maxCmdHandlers {


When this returns error we've already registered an iothread, may be we can move this check earlier?
Also this'd initiate a server shutdown I believe would that be okay? Shouldn't we just drop the new client gracefully and continue server ops?

lucifercr07 · 2024-12-09T17:22:01Z

internal/commandhandler/manager.go

+
+type Manager struct {
+	activeCmdHandlers sync.Map
+	numCmdHandlers    atomic.Int32


Shall we use atomic.Uint32 instead?

lucifercr07 · 2024-12-09T17:22:26Z

internal/commandhandler/manager.go

+type Manager struct {
+	activeCmdHandlers sync.Map
+	numCmdHandlers    atomic.Int32
+	maxCmdHandlers    int32


Same as above uint32?

lucifercr07 · 2024-12-09T17:29:39Z

internal/commandhandler/manager.go

+	if responseChan != nil && preprocessingChan != nil {
+		m.ShardManager.RegisterCommandHandler(cmdHandler.ID(), responseChan, preprocessingChan) // TODO: Change responseChan type to ShardResponse
+	} else if responseChan != nil && preprocessingChan == nil {
+		m.ShardManager.RegisterCommandHandler(cmdHandler.ID(), responseChan, nil)
+	}


Shall we change it to?

m.ShardManager.RegisterCommandHandler(cmdHandler.ID(),cmdHandler.responseChan,cmdHandler.preprocessingChan)

lucifercr07 · 2024-12-09T17:30:51Z

internal/commandhandler/manager.go

+	responseChan := cmdHandler.responseChan
+	preprocessingChan := cmdHandler.preprocessingChan


May be we can have checks in handler registration to mandate responseChan presence?

lucifercr07

Thanks for the changes, minor comments.

psrvere added 3 commits December 4, 2024 16:40

refactored to separate commandHandler from ioThread

16ab8bd

consolidated write to io channel in one place

0ffb45d

fixed linter warnings

d54d4bb

psrvere changed the title ~~Refactor - Separate out command execution responsibility to CommandHandler from IOThread~~ Refactor - Move command execution responsibility to CommandHandler from IOThread Dec 4, 2024

psrvere added 4 commits December 5, 2024 11:48

don't stop command handler at command level error

9b86beb

fixed type assertion for command handler

690430d

added an error channel between io thread and command handler to signa…

3e15eca

…l exit

bug fix

07ca2b7

psrvere changed the title ~~Refactor - Move command execution responsibility to CommandHandler from IOThread~~ [DO NOT MERGE] Refactor - Move command execution responsibility to CommandHandler from IOThread Dec 5, 2024

psrvere marked this pull request as ready for review December 5, 2024 11:39

soumya-codes self-requested a review December 5, 2024 13:31

soumya-codes reviewed Dec 9, 2024

View reviewed changes

lucifercr07 approved these changes Dec 9, 2024

View reviewed changes

lucifercr07 requested changes Dec 9, 2024

View reviewed changes

psrvere added 3 commits December 16, 2024 13:22

merged with master

c89b688

fix build fail

4aab95c

merged with master

05ec942

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] Refactor - Move command execution responsibility to CommandHandler from IOThread #1358

[DO NOT MERGE] Refactor - Move command execution responsibility to CommandHandler from IOThread #1358

psrvere commented Dec 4, 2024 •

edited

Loading

psrvere commented Dec 5, 2024

soumya-codes left a comment

soumya-codes Dec 6, 2024

soumya-codes Dec 7, 2024 •

edited

Loading

soumya-codes Dec 7, 2024

soumya-codes Dec 7, 2024

soumya-codes Dec 7, 2024

soumya-codes Dec 7, 2024

soumya-codes Dec 9, 2024

soumya-codes Dec 9, 2024

soumya-codes Dec 9, 2024

lucifercr07 left a comment

lucifercr07 Dec 9, 2024

lucifercr07 Dec 9, 2024

lucifercr07 Dec 9, 2024

lucifercr07 Dec 9, 2024

lucifercr07 Dec 9, 2024

lucifercr07 Dec 9, 2024

lucifercr07 left a comment

		responseChan := cmdHandler.responseChan
		preprocessingChan := cmdHandler.preprocessingChan

[DO NOT MERGE] Refactor - Move command execution responsibility to CommandHandler from IOThread #1358

Are you sure you want to change the base?

[DO NOT MERGE] Refactor - Move command execution responsibility to CommandHandler from IOThread #1358

Conversation

psrvere commented Dec 4, 2024 • edited Loading

psrvere commented Dec 5, 2024

soumya-codes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumya-codes Dec 7, 2024 • edited Loading

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

lucifercr07 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

lucifercr07 left a comment

Choose a reason for hiding this comment

psrvere commented Dec 4, 2024 •

edited

Loading

soumya-codes Dec 7, 2024 •

edited

Loading