Protocol (un)marshallers make many allocations

[jasdel]

The protocol marshallers make numerous allocations when marshaling and unmarshalling structs. This is much more noticeable for larger API operations such as ElasticTranscoder's CreateJob can does about 90k bytes in 4k allocations. Whereas encoding/json marshal on the same structure only does 4k bytes in 7 allocations.
#376 updates internal/protocol/json to perform a more efficient check if a field is exported or not. Reducing the allocations caused by strings.ToLower. Several of the other protocol's marshallers perform strings.ToLower check, and probably can be updated also.

Improving the SDK's marshallers to be more efficient with allocations would help to increase performance and reduce memory usage.

[jasdel]

#404 added a nice improvement to the performance of marshalling JSONRPC, JSONUTIL, and RESTJSON protocols. With a performance improvement of about 60%. Simple structures didn't see as much impact, but more complex input structs with nested types saw larger improvements.

make bench-protocol snip:

# BenchmarkJSONRPCBuild_Simple_dynamodbPutItem:
0.9.17             50000         32969 ns/op        9757 B/op        211 allocs/op
8236f00           100000         20773 ns/op        4194 B/op        105 allocs/op

# BenchmarkJSONUtilBuild_Simple_dynamodbPutItem:
0.9.17             50000         32709 ns/op        9693 B/op        209 allocs/op
8236f00           100000         20495 ns/op        4130 B/op        103 allocs/op

# BenchmarkRESTJSONBuild_Complex_elastictranscoderCreateJobInput
0.9.17              3000        439981 ns/op       89382 B/op       2554 allocs/op
8236f00             5000        280141 ns/op       41221 B/op       1131 allocs/op

# BenchmarkRESTJSONBuild_Simple_elastictranscoderListJobsByPipeline-8
0.9.17            100000         16040 ns/op        5104 B/op         98 allocs/op
8236f00           100000         14590 ns/op        4304 B/op         80 allocs/op

[bpot]

@jasdel The interface style looks good to me.

I have one other idea. Would it be possible to hang the payload tags off of the blank identifier instead of using the unexported metadata* field/SDKShapeTraits combination? For example,

type UploadPartInput struct {
  _ struct{} `type:"structure" payload:"Body"`
  ...
}

We would still need to use FieldByName("_") to fetch the payload tag but because the field we're looking for is on the top level struct it would take the fast path instead of the path the triggers all of the allocations.

[jasdel]

Hi @bpot Thanks for digging deeper into this pattern using _ is an interesting alternative. Ill review the PR at #453 and update you there with any feedback.

[tve]

Is there any work in progress on the XML front? The EC2 DescribeImages call seems like a good test case ;-)

[jasdel]

Updated in #722. We're currently investigating how the marshalers can be improved. Hopefully this will lead to replacing the memory duplication and reflection type walking with code generation.

[jonaskint]

Any upgrade on the status of this issue? We're currently running against the same problem as issue #1300 .

[jasdel]

Thanks for the feedback @jonaskint. We started the work with this task via #1554 targeting RESTXML and RESTJSON marshaling first. We discovered the SDK's protocol tests were not comprehensive and allowed a bug to leak to master. We reverted the change due to an error in the code generation and marshaling of the payloads. This PR represents the pending work on the RESTJSON and RESTXML marshalers. We've not yet started work on improving the unmarshalers.

For the s3 manager issue #1554 won't fix that. I think s3manager needs to be refactored significantly to improve its memory usage and footprint. This work is still in our backlog.

[jasdel]

Merging #722 and this issue together to unify the discussion and work on this issue. Both can be solved by improved SDK (un)marshaling support.

[teastburn]

Hello, thanks for the hard work! Any updates on this?

I am seeing ~9x memory growth using WriteAtBuffer from a Lambda. Here I am downloading a file that is 50949808 bytes and printing memory stats via runtime.MemStats:

# Alloc = 1 MiB TotalAlloc = 1 MiB Sys = 3 MiB NumGC = 0
buffer := aws.NewWriteAtBuffer([]byte{})
n, err := downloader.Download(buffer, &s3.GetObjectInput{
    Bucket: aws.String(s3Bucket),
    Key:    aws.String(s3Path),
})
# Alloc = 250 MiB TotalAlloc = 25973 MiB Sys = 438 MiB NumGC = 346

Note the Sys memory, which is what Lambda seems to care about, went up 435 MiB.

[jasdel]

@teastburn have you tried to preallocated the buffer used by the WriteAtBuffer? You can do this by passing in a preallocated []byte slice into NewWriteAtBuffer. Without a preallocated buffer each time the underlying buffer of WriteAtBuffer needs to grow it would need to allocate a new byte array and copy in the old data. The old buffer will eventually be garbage collected, but depending on the order of parts downloaded of the object this could cause many allocations.

[teastburn]

@jasdel thanks for the quick response! I did also try that after looking at the WriteAtBuffer code.

# Alloc = 1 MiB TotalAlloc = 1 MiB Sys = 5 MiB NumGC = 0
b := make([]byte, 50000000)
# Alloc = 49 MiB    TotalAlloc = 49 MiB    Sys = 55 MiB    NumGC = 1
buffer := aws.NewWriteAtBuffer(b)
# Alloc = 48 MiB    TotalAlloc = 49 MiB    Sys = 55 MiB    NumGC = 1
n, err := downloader.Download(buffer, &s3.GetObjectInput{
	Bucket: aws.String(s3Bucket),
	Key:    aws.String(s3Path),
})
# Alloc = 153 MiB    TotalAlloc = 5522 MiB    Sys = 525 MiB    NumGC = 55

Interestingly, the Lambda did not run out of memory (Memory Size: 512 MB Max Memory Used: 509 MB), and the NumGC is far lower when pre-allocating, along with TotalAlloc. However, the Sys memory used appears to be roughly equivalent or higher.

I'm using go version go1.x (lambda) and aws-sdk-go version v1.15.72.

[teastburn]

I switched to using

	resp, err := svc.GetObject(&s3.GetObjectInput{
		Bucket: aws.String(s3Bucket),
		Key:    aws.String(s3Path),
	})

which uses a constant amount of memory if used with an io.Reader on resp.Body.

[jasdel]

Thanks for the update @teastburn. Is your application using io.Copy from the resp.Body into another buffer, or is your application reading directly from the resp.Body reader?

[teastburn]

@jasdel do you have a recommendation there? I am wrapping it in a bufio.NewReader so that I can easily read by lines via a Scanner. One semi annoying issue with my solution is the caller to my struct receiver function still has to call Close() on the body via a struct method. The good thing, however, is that even loading a 256mb file takes a max memory of 50mb.

// Allows us to loop through s3 file line by line
type ByteReader interface {
	io.Reader
	ReadBytes(byte) ([]byte, error)
}

// Mockable interface
type S3FileGettable interface {
	Get(string, string) (ByteReader, error)
	io.Closer
}

 type S3Getter struct {
	body io.ReadCloser
}

 func (s3g *S3Getter) Close() error {
	return s3g.body.Close()
}

 // Must call S3Getter.Close() after done with ByteReader
func (s3g *S3Getter) Get(s3Bucket, s3Path string) (ByteReader, error) {
	// ...
	svc := s3.New(session.Must(session.NewSession(s3Cfg)))
	resp, err := svc.GetObject(&s3.GetObjectInput{
		Bucket: aws.String(s3Bucket),
		Key:    aws.String(s3Path),
	})
	// ...
	s3g.body = resp.Body
	reader := bufio.NewReader(resp.Body)
	return reader, nil
}

[jasdel]

Using the buff reader like your are is a good way to get the ByteReader interface. I suggest instead of the close method on S3Getter that the returned reader has the Close method on it. This would allow close handling to be performed by the downstream consumer of the stream, instead of making S3Getter stateful. If S3Getter.Get is accidentally called before the S3Getter.Close method is called, the previous object's body may never be closed.

Something like the following might work better for closing the body.

type ByteReadCloser interface {
    ByteReader
    io.Closer
}

type S3FileGetter struct {
     Client *s3.S3 // Add client shared across all object gets.
}

func (s3g *S3Getter) Get(s3Bucket, s3Path string) (ByteReadCloser, error) {
	// ...
	resp, err := s3g.Client.GetObject(&s3.GetObjectInput{
		Bucket: aws.String(s3Bucket),
		Key:    aws.String(s3Path),
	})
	// ...
	return NewBufferedReadCloser(resp.Body), nil
}

type BufferedReadCloser struct {
    *bufio.Reader
    io.Closer
}

func NewBufferedReadCloser(reader io.ReadCloser) *BufferedReadCloser {
    return &BufferedReadCloser{
         Reader: bufio.NewReader(reader),
         Closer: reader,
    }
}

func (b *ByteReadCloser) Close() error {
     return b.Closer.Close()
}

Then be used similar to the following. Also, try to only initialize the S3 client once per region/account to ensure that duplicate resources aren't created within the applications.

objGetter := S3Getter{Client: s3.New(session.Must(session.NewSession(s3Cfg)))}

// Get each object, and process it

reader, err := objGetter.Get(myBucket, myKey)
// handle error
defer reader.Close()

// process reader content.

[teastburn]

@jasdel That's great feedback. Thanks for the help!

[eran-levy]

Its possible to provide another buffer strategy that is using sync.Pool for download, just wrote the following post for more info: https://medium.com/@levyeran/high-memory-allocations-and-gc-cycles-while-downloading-large-s3-objects-using-the-aws-sdk-for-go-e776a136c5d0

[github-actions]

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.