Skip to content

common, mds: combine several encode() calls#60252

Merged
batrick merged 2 commits intoceph:mainfrom
MaxKellermann:denc_encode_tuple
Feb 6, 2025
Merged

common, mds: combine several encode() calls#60252
batrick merged 2 commits intoceph:mainfrom
MaxKellermann:denc_encode_tuple

Conversation

@MaxKellermann
Copy link
Member

The denc library allows appending multiple values in a single buffer::list call by wrapping them in a std::tuple. This reduces overhead because the buffer bounds checks have to be performed only once.

This PR optimizes a few hot code paths that were visible in the profiler.

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)

@github-actions github-actions bot added cephfs Ceph File System common labels Oct 10, 2024
@MaxKellermann MaxKellermann force-pushed the denc_encode_tuple branch 5 times, most recently from 94e8e7e to 66ab50e Compare October 14, 2024 14:44
@chrisphoffman chrisphoffman requested a review from a team October 17, 2024 15:28
@dparmar18 dparmar18 self-assigned this Nov 19, 2024
@dparmar18
Copy link
Contributor

jenkins retest this please

The `denc` library allows appending multiple values in a single
`buffer::list` call by wrapping them in a `std::tuple`.  This reduces
overhead because the buffer bounds checks have to be performed only
once.

The `file_layout_t` type turned up in the profiler, so it was an
obvious choice to optimize now.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
The `denc` library allows appending multiple values in a single
`buffer::list` call by wrapping them in a `std::tuple`.  This reduces
overhead because the buffer bounds checks have to be performed only
once.

This patch optimizes several types that turned up in the profiler.

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

It's been nearly a month.

@dparmar18
Copy link
Contributor

Hey @MaxKellermann sorry for the delay, just a quick question - Shouldn't entries like

encode(any_i->change_attr, bl);
also be included? This is fixed-size and I guess might benefit from the optimisation?

@MaxKellermann
Copy link
Member Author

@dparmar18, why not - this PR does not have 100% coverage of all possible optimizations. If you look close enough, you will be able to find hundreds of other calls that can be optimized with this trick. Once this PR is merged, you could post another PR with these findings.

@dparmar18
Copy link
Contributor

@dparmar18, why not - this PR does not have 100% coverage of all possible optimizations. If you look close enough, you will be able to find hundreds of other calls that can be optimized with this trick. Once this PR is merged, you could post another PR with these findings.

alright

@MaxKellermann
Copy link
Member Author

It's been another 3 weeks. And more than 3 months since I submitted this.

@dparmar18
Copy link
Contributor

It's been another 3 weeks. And more than 3 months since I submitted this.

hey @MaxKellermann apologies for the delay. There are tons of patches pending to be reviewed and QAed. We should pick this up soon, ping @batrick

@dparmar18 dparmar18 requested a review from batrick January 28, 2025 19:04
@dparmar18
Copy link
Contributor

jenkins test make check arm64

Copy link
Member

@batrick batrick left a comment

Choose a reason for hiding this comment

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

Apologies for the delay!!

@batrick
Copy link
Member

batrick commented Feb 5, 2025

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

@batrick
Copy link
Member

batrick commented Feb 6, 2025

@batrick batrick merged commit 7e6ff07 into ceph:main Feb 6, 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.

3 participants