GODRIVER-2778 Reduce memory usage on compressed loads. #1200

qingyang-hu · 2023-03-09T23:05:44Z

Summary

Modified around Operation.decompressWireMessage to eliminate duplicate memory allocation;
Reused zlib.Writer;
Removed unused parameter of ReadWireMessage in the interface of driver.Connection.

Background & Motivation

name                          old time/op    new time/op    delta
ClientRead/not_compressed-10    33.5µs ±17%    29.6µs ± 7%  -11.61%  (p=0.000 n=10+10)
ClientRead/snappy-10            37.2µs ±12%    34.9µs ± 8%     ~     (p=0.095 n=10+9)
ClientRead/zlib-10               130µs ± 1%      57µs ± 6%  -55.78%  (p=0.000 n=9+9)
ClientRead/zstd-10              46.3µs ±16%    48.5µs ±22%     ~     (p=0.436 n=10+10)

name                          old alloc/op   new alloc/op   delta
ClientRead/not_compressed-10    17.4kB ± 0%    17.4kB ± 0%     ~     (p=0.900 n=10+10)
ClientRead/snappy-10            25.3kB ± 0%    20.1kB ± 0%  -20.67%  (p=0.000 n=10+10)
ClientRead/zlib-10               882kB ± 0%      61kB ± 0%  -93.11%  (p=0.000 n=10+10)
ClientRead/zstd-10              84.0kB ± 0%    78.8kB ± 0%   -6.18%  (p=0.000 n=7+10)

name                          old allocs/op  new allocs/op  delta
ClientRead/not_compressed-10      73.0 ± 0%      73.0 ± 0%     ~     (all equal)
ClientRead/snappy-10              76.0 ± 0%      75.0 ± 0%   -1.32%  (p=0.000 n=9+10)
ClientRead/zlib-10                 118 ± 0%        90 ± 0%  -23.73%  (p=0.000 n=10+10)
ClientRead/zstd-10                 178 ± 0%       177 ± 0%   -0.56%  (p=0.000 n=9+9)

name                           old time/op    new time/op    delta
ClientWrite/not_compressed-10    40.6µs ±16%    39.9µs ±16%     ~     (p=0.529 n=10+10)
ClientWrite/snappy-10            47.8µs ±18%    42.5µs ±13%  -11.10%  (p=0.009 n=10+10)
ClientWrite/zlib-10               131µs ± 4%     136µs ± 1%   +3.21%  (p=0.043 n=10+9)
ClientWrite/zstd-10              55.4µs ±11%    50.2µs ± 5%   -9.46%  (p=0.006 n=10+9)

name                           old alloc/op   new alloc/op   delta
ClientWrite/not_compressed-10    19.3kB ± 0%    19.4kB ± 0%   +0.10%  (p=0.022 n=10+10)
ClientWrite/snappy-10            24.3kB ± 0%    24.2kB ± 0%   -0.42%  (p=0.001 n=10+10)
ClientWrite/zlib-10               884kB ± 0%      61kB ± 1%  -93.07%  (p=0.000 n=10+10)
ClientWrite/zstd-10              33.9kB ± 0%    33.8kB ± 0%   -0.14%  (p=0.000 n=10+7)

name                           old allocs/op  new allocs/op  delta
ClientWrite/not_compressed-10      57.0 ± 0%      57.0 ± 0%     ~     (all equal)
ClientWrite/snappy-10              60.0 ± 0%      59.0 ± 0%   -1.67%  (p=0.000 n=10+10)
ClientWrite/zlib-10                97.0 ± 0%      67.0 ± 0%  -30.93%  (p=0.000 n=10+9)
ClientWrite/zstd-10                 149 ± 0%       148 ± 0%   -0.67%  (p=0.000 n=10+9)

Memory usage of "ClientRead/snappy" shows a ~20% reduction because we have already allocated memory for the uncompressed wiremessage (code).

prestonvasquez

Looks really good, just a few considerations.

prestonvasquez · 2023-03-13T17:59:31Z

x/mongo/driver/compression.go

 		if err != nil {
 			return nil, err
 		}
+		defer r.Close()


Suggested change

defer r.Close()

defer func() {

if err := r.Close(); err != nil {

panic(err)

}

}()

DecompressPayload returns an error, so we should return the error instead of panicking.

E.g.

// ... _, err = io.ReadFull(r, uncompressed) if err != nil { return nil, err } if err := r.Close(); err != nil { return nil, err } return uncompressed, nil // ...

Note that calling Close on the reader is probably redundant because it basically returns the last seen error (see Close code here and here), so io.ReadFull will already have encountered that error. We can include it for completeness, or a comment that describes that we never expect Close to return a new error.

I've modified it to use a named return value to return the error from Close in defer.

prestonvasquez · 2023-03-13T18:08:33Z

x/mongo/driver/compression.go

+type zlibEncoder struct {
+	l sync.Locker
+	w *zlib.Writer
+	b *bytes.Buffer
+}


Can we use more readable field names like "mu", "writer" and "buf"?

prestonvasquez · 2023-03-13T18:11:29Z

x/mongo/driver/compression.go

@@ -41,6 +40,50 @@ func getZstdEncoder(l zstd.EncoderLevel) (*zstd.Encoder, error) {
 	return encoder, nil
 }

+var zlibEncoders = &sync.Map{}


IIUC the pointer to this map is unecessary.

Suggested change

var zlibEncoders = &sync.Map{}

var zlibEncoders = sync.Map

prestonvasquez · 2023-03-13T18:12:28Z

x/mongo/driver/compression.go

+	if v, ok := zlibEncoders.Load(l); ok {
+		return v.(*zlibEncoder), nil
+	}
+	b := bytes.NewBuffer(nil)


Optional way of constructing a new bytes.Buffer without the awkward "nil" initializer.

Suggested change

b := bytes.NewBuffer(nil)

b := new(bytes.Buffer)

prestonvasquez · 2023-03-13T18:19:37Z

x/mongo/driver/compression.go

+	defer func() {
+		e.b.Reset()
+		e.w.Reset(e.b)
+	}()


What are your thoughts on removing the defer functoin here and executing the "Reset" methods at the top of the calls to "Encode". This seems marginally safer.

kevinAlbs · 2023-03-16T18:44:25Z

x/mongo/driver/operation.go

@@ -1602,25 +1599,10 @@ func (Operation) canCompress(cmd string) bool {
 // includesHeader: specifies whether or not wm includes the message header
 // Returns the decoded OP_REPLY. If the err field of the returned opReply is non-nil, an error occurred while decoding
 // or validating the response and the other fields are undefined.
-func (Operation) decodeOpReply(wm []byte, includesHeader bool) opReply {
+func (Operation) decodeOpReply(wm []byte) opReply {


FYI: OP_MSG is supported in MongoDB server version 3.6+: https://github.com/mongodb/specifications/blob/master/source/wireversion-featurelist.rst

I think OP_REPLY is only used for the initial command to a server until the wire version is discovered here. The Go driver requires server version 3.6+. I expect decompression for OP_REPLY is not commonly executed.

No need to change.

kevinAlbs · 2023-03-16T18:44:53Z

x/mongo/driver/operation.go

@@ -1602,25 +1599,10 @@ func (Operation) canCompress(cmd string) bool {
 // includesHeader: specifies whether or not wm includes the message header


Remove comment for includesHeader.

matthewdale · 2023-03-17T16:50:58Z

x/mongo/driver/compression.go

 		if err != nil {
 			return nil, err
 		}
+		defer r.Close()


DecompressPayload returns an error, so we should return the error instead of panicking.

E.g.

// ... _, err = io.ReadFull(r, uncompressed) if err != nil { return nil, err } if err := r.Close(); err != nil { return nil, err } return uncompressed, nil // ...

Note that calling Close on the reader is probably redundant because it basically returns the last seen error (see Close code here and here), so io.ReadFull will already have encountered that error. We can include it for completeness, or a comment that describes that we never expect Close to return a new error.

x/mongo/driver/compression.go

x/mongo/driver/operation.go

matthewdale

Looks good! 👍

prestonvasquez

LGTM!

Reduce memory usage on compressed loads.

6c750fd

qingyang-hu force-pushed the 20230309MemImprovement branch 3 times, most recently from 7a59d89 to c7ff6fc Compare March 10, 2023 18:59

Reuse zlib.Writer.

51f9261

qingyang-hu force-pushed the 20230309MemImprovement branch from c7ff6fc to 51f9261 Compare March 10, 2023 21:05

qingyang-hu requested review from matthewdale and prestonvasquez March 13, 2023 14:34

prestonvasquez reviewed Mar 13, 2023

View reviewed changes

kevinAlbs reviewed Mar 16, 2023

View reviewed changes

qingyang-hu changed the title ~~Reduce memory usage on compressed loads.~~ GODRIVER-2778 Reduce memory usage on compressed loads. Mar 17, 2023

matthewdale reviewed Mar 17, 2023

View reviewed changes

refactor on operation decompress.

017e473

qingyang-hu requested review from matthewdale and prestonvasquez March 20, 2023 21:47

matthewdale approved these changes Mar 22, 2023

View reviewed changes

prestonvasquez approved these changes Mar 23, 2023

View reviewed changes

qingyang-hu merged commit 785bf9b into mongodb:master Mar 23, 2023

prestonvasquez pushed a commit that referenced this pull request Apr 4, 2023

GODRIVER-2778 Reduce memory usage on compressed loads. (#1200)

b6bdeda

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-2778 Reduce memory usage on compressed loads. #1200

GODRIVER-2778 Reduce memory usage on compressed loads. #1200

qingyang-hu commented Mar 9, 2023 •

edited

prestonvasquez left a comment

prestonvasquez Mar 13, 2023

matthewdale Mar 17, 2023

qingyang-hu Mar 20, 2023

prestonvasquez Mar 13, 2023 •

edited

prestonvasquez Mar 13, 2023

prestonvasquez Mar 13, 2023

prestonvasquez Mar 13, 2023

kevinAlbs Mar 16, 2023

kevinAlbs Mar 16, 2023

matthewdale Mar 17, 2023

matthewdale left a comment

prestonvasquez left a comment

		@@ -1602,25 +1599,10 @@ func (Operation) canCompress(cmd string) bool {
		// includesHeader: specifies whether or not wm includes the message header

GODRIVER-2778 Reduce memory usage on compressed loads. #1200

GODRIVER-2778 Reduce memory usage on compressed loads. #1200

Conversation

qingyang-hu commented Mar 9, 2023 • edited

Summary

Background & Motivation

prestonvasquez 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

prestonvasquez Mar 13, 2023 • edited

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

matthewdale left a comment

Choose a reason for hiding this comment

prestonvasquez left a comment

Choose a reason for hiding this comment

qingyang-hu commented Mar 9, 2023 •

edited

prestonvasquez Mar 13, 2023 •

edited