Skip to content

Conversation

@kiroxas
Copy link
Contributor

@kiroxas kiroxas commented Apr 2, 2025

Spotted during this PR.

Used span to avoid some substring allocations. Avoid find that could be done at compile time. Remove useless push_back, parse_utf8 does not have to be 0 terminated.

Extracted strip_edges logic into a static function, to be able to call it with span and strings (without the need to allocate a string).
The name may not be very good, but naming it strip_edges brought conflict with Variant, probably because the name is bound.
Maybe we should come up with a long term solution for this because I guess the need will arise with other functions (applying strings functions to span created from strings).

@kiroxas kiroxas requested a review from a team as a code owner April 2, 2025 15:37
@kiroxas kiroxas changed the title Use Span to reduce allocations in http_client_tcp Use Span to reduce allocations in http_client_tcp::poll Apr 2, 2025
@AThousandShips AThousandShips added this to the 4.x milestone Apr 2, 2025
@kiroxas kiroxas force-pushed the pollAllocations branch 2 times, most recently from 98317fc to 5159a8a Compare April 2, 2025 15:47
@kiroxas kiroxas force-pushed the pollAllocations branch 2 times, most recently from ede3568 to 26988b1 Compare April 4, 2025 11:25
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

Code wise this looks good to me!
Unfortunately, I don't have any insight on the http client, and whether this function needs to be fast.
I'd like to see either a benchmark showing a practical improvement, or a review from networking folks.

const int index = std::size(transferEncodingLabel) - 1;
Span<char32_t> sp = Span<char32_t>(s.ptr() + index, s.length() - index);
sp = String::strip_edges_span(sp, true, true);
if (String("chunked") == sp) {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't need to allocate a String. However, my equality operator PR is still outstanding, so I think it's acceptable for the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it would be better to not have to construct the string, but we can't right now. We can update this if your PR is merged.

@Ivorforce Ivorforce requested a review from a team April 4, 2025 15:25
@akien-mga akien-mga changed the title Use Span to reduce allocations in http_client_tcp::poll Use Span to reduce allocations in HTTPClientTCP::poll Apr 4, 2025
@Repiteo Repiteo modified the milestones: 4.x, 4.5 May 14, 2025
@akien-mga akien-mga requested a review from Faless June 5, 2025 10:30
@Repiteo Repiteo modified the milestones: 4.5, 4.6 Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants