Skip to content

Removes unnecessary pointer indirection from a member of DelegatingStreamFilter#44652

Open
birenroy wants to merge 3 commits into
envoyproxy:mainfrom
birenroy:depointerize-http-matching-data
Open

Removes unnecessary pointer indirection from a member of DelegatingStreamFilter#44652
birenroy wants to merge 3 commits into
envoyproxy:mainfrom
birenroy:depointerize-http-matching-data

Conversation

@birenroy
Copy link
Copy Markdown
Contributor

@birenroy birenroy commented Apr 24, 2026

We have noticed that these filters are using more memory than expected. This is one attempt to slightly reduce the cost.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

…reamFilter.

Signed-off-by: Biren Roy <birenroy@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #44652 was opened by birenroy.

see: more, trace.

Signed-off-by: Biren Roy <birenroy@google.com>
Signed-off-by: Biren Roy <birenroy@google.com>
@birenroy birenroy marked this pull request as ready for review April 24, 2026 22:23
@birenroy birenroy requested review from tyxia and wbpcode as code owners April 24, 2026 22:23
@birenroy
Copy link
Copy Markdown
Contributor Author

/assign @adisuissa
/assign @RyanTheOptimist

Copy link
Copy Markdown
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks, overall LGTM, but wanted to highlight the memory tradeoffs and see that it was intentional.

Envoy::Http::StreamFilterBase* base_filter_{};

std::unique_ptr<Envoy::Http::Matching::HttpMatchingDataImpl> matching_data_;
absl::optional<Envoy::Http::Matching::HttpMatchingDataImpl> matching_data_;
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.

Pointing out, to make sure it was intentional:
This will now have a larger memory footprint than the unique_ptr.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, even though optional has better runtime performance: more cache friendly and no heap allocation, it has bigger size which is this PR aims to?

Also, it disables assignment operator= since HttpMatchingDataImpl has reference member i.e. future usage flexibility

Copy link
Copy Markdown
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this.

Envoy::Http::StreamFilterBase* base_filter_{};

std::unique_ptr<Envoy::Http::Matching::HttpMatchingDataImpl> matching_data_;
absl::optional<Envoy::Http::Matching::HttpMatchingDataImpl> matching_data_;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, even though optional has better runtime performance: more cache friendly and no heap allocation, it has bigger size which is this PR aims to?

Also, it disables assignment operator= since HttpMatchingDataImpl has reference member i.e. future usage flexibility

@tyxia tyxia self-assigned this Apr 27, 2026
@RyanTheOptimist
Copy link
Copy Markdown
Contributor

/wait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants