Skip to content

pool byte buffers on deserialize + string arena - #693

Merged
lucix-aws merged 3 commits into
mainfrom
bufpool
Aug 7, 2026
Merged

pool byte buffers on deserialize + string arena#693
lucix-aws merged 3 commits into
mainfrom
bufpool

Conversation

@lucix-aws

Copy link
Copy Markdown
Collaborator
  • io.ReadAll is inefficient for response buffering because it starts at like 512 bytes and repeatedly doubles until it consumes the reader, which means we waste a bunch of intermediate allocations. pool byte buffers on deserialization for all protocols, up to a limit
  • add a string arena, the goal here is to reduce allocation pressure for repeated small strings. It sizes to 1/4th of the response payload but I've also capped it at 4k for now, so big responses etc. won't cause a lot of memory to be pinned
  • attempt to presize buffers for @httpPayload + blob rather than io.ReadAll

@lucix-aws
lucix-aws requested review from a team as code owners August 6, 2026 15:55
@lucix-aws

Copy link
Copy Markdown
Collaborator Author

performance numbers when running this patch against our service/internal/serdebenchmark in the SDK

image

@Madrigal Madrigal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, super happy to see this!

req.URL.Path = "/"
}
req.Header.Set("X-Amz-Target", fmt.Sprintf("%s.%s", p.serviceName, middleware.GetOperationName(ctx)))
req.Header.Set("X-Amz-Target", fmt.Sprintf("%s.%s", p.serviceName, schema.ID().Name))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this available even on non-migrated services?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in non-schema serde services? they're not calling this

return &ShapeDeserializer{p: p, head: serde.NewStack[deserCtx](), opts: o}

d := &ShapeDeserializer{p: p, head: serde.NewStack[deserCtx](), opts: o}
d.arena.Reset(len(p) / 4)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why /4?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a fourth of the payload up to 4k, i'll const this

}
}

// An indefinite-length byte string is legal CBOR that ReadBlob does not

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent corner case

Comment thread transport/http/protocol/internal/httpbinding/ext_test.go Outdated
}
}

func BenchmarkHasBlobPayload(b *testing.B) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we run testing.B tests?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was mostly intermediate to check the schema extension perf improved it, i'm probably jst gonna delete this

presize = maxPresize
}

// + bytes.MinRead prevents pointless doubling on EOF

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

lucix-aws and others added 2 commits August 7, 2026 13:40
Co-authored-by: Luis Madrigal <599908+Madrigal@users.noreply.github.com>
@lucix-aws
lucix-aws merged commit 6a87093 into main Aug 7, 2026
15 checks passed
@lucix-aws
lucix-aws deleted the bufpool branch August 7, 2026 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants