Skip to content

persist: skip lgalloc memcpy in S3 get path#36305

Merged
antiguru merged 1 commit intoMaterializeInc:mainfrom
antiguru:s3-skip-lgalloc-memcpy
Apr 28, 2026
Merged

persist: skip lgalloc memcpy in S3 get path#36305
antiguru merged 1 commit intoMaterializeInc:mainfrom
antiguru:s3-skip-lgalloc-memcpy

Conversation

@antiguru
Copy link
Copy Markdown
Member

The S3 get path copied each Bytes chunk returned by the SDK into a freshly-allocated MetricsRegion<u8>.
With lgalloc disabled in practice that region falls back to the heap, making the copy pure overhead.
Push SDK Bytes straight into SegmentedBytes instead.

Also removes the now-unused persist_enable_s3_lgalloc_{cc,noncc}_sizes dyncfgs, the cfg/is_cc_active fields on S3BlobConfig/S3Blob, and the dead LgBytesMetrics::persist_s3 metric.

Motivation

Reduces CPU and allocator pressure on every S3 read in persist.

Tips for reviewer

The behavior change is in Blob::get in src/persist/src/s3.rs; everything else is dead-code removal that follows.

Checklist

  • This PR has adequate test coverage / QA is not needed.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

The S3 get path copied every chunk fetched from the SDK into a
freshly-allocated `MetricsRegion<u8>`. With lgalloc disabled in practice
the region falls back to the heap, so the copy is pure overhead. Push
SDK `Bytes` straight into `SegmentedBytes` instead.

Removes the `persist_enable_s3_lgalloc_{cc,noncc}_sizes` dyncfgs, the
unused `cfg`/`is_cc_active` fields on `S3BlobConfig`/`S3Blob`, and the
now-dead `LgBytesMetrics::persist_s3` field.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@antiguru antiguru requested a review from a team as a code owner April 28, 2026 12:53
@antiguru
Copy link
Copy Markdown
Member Author

We should do the same for other parts, too, like azure and arrow.

Copy link
Copy Markdown
Member

@DAlperin DAlperin left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@antiguru antiguru merged commit fb595f2 into MaterializeInc:main Apr 28, 2026
120 checks passed
@antiguru antiguru deleted the s3-skip-lgalloc-memcpy branch April 28, 2026 14:55
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