[GitHub] [pulsar-client-go] merlimat commented on a change in pull request #253: Improve go client sending performance

[GitBox <gi...@apache.org>]

merlimat commented on a change in pull request #253:
URL: https://github.com/apache/pulsar-client-go/pull/253#discussion_r426704126



##########
File path: pulsar/internal/batch_builder.go
##########
@@ -34,7 +34,7 @@ const (
 	// MaxBatchSize will be the largest size for a batch sent from this particular producer.
 	// This is used as a baseline to allocate a new buffer that can hold the entire batch
 	// without needing costly re-allocations.
-	MaxBatchSize = 128 * 1024
+	MaxBatchSize = 1024 * 1024

Review comment:
       The reason for using 128 KB is that we measured that there's no advantage in using bigger batches, as in we can easily saturate the network bandwidth already.

##########
File path: pulsar/internal/batch_builder.go
##########
@@ -66,7 +66,7 @@ func NewBatchBuilder(maxMessages uint, producerName string, producerID uint64,
 		maxMessages = DefaultMaxMessagesPerBatch
 	}
 	bb := &BatchBuilder{
-		buffer:       NewBuffer(4096),
+		buffer:       NewBuffer(MaxBatchSize),

Review comment:
       That's increasing the min memory size for each producer from 4KB to 1MB. That would be very bad for overall memory usage.

##########
File path: pulsar/internal/buffer.go
##########
@@ -77,13 +86,43 @@ type buffer struct {
 
 // NewBuffer creates and initializes a new Buffer using buf as its initial contents.
 func NewBuffer(size int) Buffer {
+	var memSeg []byte
+	memSegmentQueue := getMemorySegmentQueue(size)

Review comment:
       This will introduce a contention point on a global mutex, plus reading from a global channel, which again uses a mutex. I'd like to see some data indicating how this change is going to perform compared to the current code.

##########
File path: pulsar/internal/connection.go
##########
@@ -166,7 +166,7 @@ func newConnection(logicalAddr *url.URL, physicalAddr *url.URL, tlsOptions *TLSO
 		connectionTimeout:    connectionTimeout,
 		logicalAddr:          logicalAddr,
 		physicalAddr:         physicalAddr,
-		writeBuffer:          NewBuffer(4096),
+		writeBuffer:          NewBuffer(512 * 1024),

Review comment:
       The buffer is auto-expanding as needed. I don't think increasing the minimum size to 0.5MB is going to be making any difference other than increasing the overall memory usage.

##########
File path: pulsar/internal/connection_reader.go
##########
@@ -36,7 +36,7 @@ func newConnectionReader(cnx *connection) *connectionReader {
 	return &connectionReader{
 		cnx:    cnx,
 		reader: bufio.NewReader(cnx.cnx),
-		buffer: NewBuffer(4096),
+		buffer: NewBuffer(4 * 1024 * 1024),

Review comment:
       Again, the buffer is auto-expanding. Increasing here from 4 KB to 4 MB is going to just blow up the memory with no advantage.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org