Skip to content

msg/async/ProtocolV2: pass desc as C string to write()#60106

Merged
batrick merged 1 commit intoceph:mainfrom
MaxKellermann:protov2_write_cstring
Oct 22, 2024
Merged

msg/async/ProtocolV2: pass desc as C string to write()#60106
batrick merged 1 commit intoceph:mainfrom
MaxKellermann:protov2_write_cstring

Conversation

@MaxKellermann
Copy link
Member

All callers really pass a C string literal, and declaring a std::string parameter will implicitly create two std::string instances: one on the caller's stack, and another one inside write() as parameter to the continuation lambda. This causes considerable and unnecessary overhead.

This patch reduces the code size by nearly 2 kB.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)

@MaxKellermann MaxKellermann requested a review from a team as a code owner October 3, 2024 15:26
@github-actions github-actions bot added the core label Oct 3, 2024
@MaxKellermann
Copy link
Member Author

"Identified problems
No space left on device"

Yet another bogus CI failure.

@MaxKellermann
Copy link
Member Author

@batrick, do you still believe std::string_view is better? Please confirm, then I'll change the PR (and keep the lower-overhead C string in our private fork).

All callers really pass a C string literal, and declaring a
`std::string` parameter will implicitly create two `std::string`
instances: one on the caller's stack, and another one inside write()
as parameter to the continuation lambda.  This causes considerable and
unnecessary overhead.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@batrick
Copy link
Member

batrick commented Oct 8, 2024

jenkins test api

@batrick
Copy link
Member

batrick commented Oct 8, 2024

jenkins test make check arm64

@batrick
Copy link
Member

batrick commented Oct 9, 2024

jenkins test api

1 similar comment
@batrick
Copy link
Member

batrick commented Oct 9, 2024

jenkins test api

@batrick
Copy link
Member

batrick commented Oct 19, 2024

This PR is under test in https://tracker.ceph.com/issues/68629.

batrick added a commit to batrick/ceph that referenced this pull request Oct 19, 2024
* refs/pull/60106/head:
	msg/async/ProtocolV2: pass `desc` as `std::string_view` to write()
@batrick
Copy link
Member

batrick commented Oct 22, 2024

@batrick batrick merged commit ee6523b into ceph:main Oct 22, 2024
@MaxKellermann MaxKellermann deleted the protov2_write_cstring branch October 22, 2024 05:09
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.

3 participants