Skip to content

Misc optimizations#60175

Merged
MaxKellermann merged 2 commits intoceph:mainfrom
MaxKellermann:misc_optimizations
Feb 12, 2025
Merged

Misc optimizations#60175
MaxKellermann merged 2 commits intoceph:mainfrom
MaxKellermann:misc_optimizations

Conversation

@MaxKellermann
Copy link
Member

@MaxKellermann MaxKellermann commented Oct 7, 2024

Various micro-optimizations.

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 7, 2024 21:17
batrick
batrick requested changes Oct 8, 2024
View reviewed changes
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.

I have no objections to these changes except that please split these into multiple PRs by component. It's much easier to QA/review that way.

@MaxKellermann
Copy link
Member Author

MaxKellermann commented Oct 8, 2024

Will do. One PR per top-level directory?

Can you see why that one unit test fails? I tried building the unit tests, but I get strange build failures - I will eventually fix them, but since I've seen lots of bogus CI failures, I thought maybe this one may be bogus as well...

@ronen-fr
Copy link
Contributor

ronen-fr commented Oct 8, 2024

A minor note: the erase() fixes seem to be more than optimizations. Might be worth mentioning in the title and labels as fixes (unless I'm missing something)

@MaxKellermann
Copy link
Member Author

MaxKellermann commented Oct 8, 2024

A minor note: the erase() fixes seem to be more than optimizations. Might be worth mentioning in the title and labels as fixes (unless I'm missing something)

The old code was not wrong; previously, the iterators were incremented manually before calling erase(). That is guaranteed to work, but it's just not good style. This doesn't fix an actual bug, and the program behavior doesn't change. The code just gets simpler and better (for some subjective meaning of "better").

@ronen-fr
Copy link
Contributor

ronen-fr commented Oct 8, 2024

The old code was not wrong; previously, the iterators were incremented manually before calling erase()

Hmm.. The line I saw said
observers.erase(o++);

So if 'observers' is a vector - that's a bug. Indeed - it is not a vector, so I stand corrected.

This was referenced Oct 9, 2024
Merged
Merged
Merged
Merged
@MaxKellermann
Copy link
Member Author

MaxKellermann commented Oct 9, 2024

I created 4 new PRs with almost all of the patches here. When these are merged, I'll rebase this PR and then we see what still remains here.

@github-actions
Copy link

github-actions bot commented Oct 22, 2024

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@MaxKellermann
Copy link
Member Author

MaxKellermann commented Oct 22, 2024

Rebased and dropped the patches that are already in the other two remaining open PRs.

@MaxKellermann
various: disable boost::intrusive::constant_time_size
3ee1ce7 
By default, the Boost intrusive containers enable the
`constant_time_size` option which adds overhead to each modification
for tracking the size in a field.  We don't need that.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann
client/barrier: use empty() instead of size()==0
f261347 
For some containers, `empty()` is faster than `size()`.

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

MaxKellermann commented Nov 14, 2024

@batrick is this trimmed-down PR acceptable or do you want me to split it further?

@MaxKellermann
Copy link
Member Author

MaxKellermann commented Dec 7, 2024

@batrick?

1 similar comment
@MaxKellermann
Copy link
Member Author

MaxKellermann commented Jan 4, 2025

@batrick?

@batrick
Copy link
Member

batrick commented Jan 6, 2025

retest this please

@ronen-fr
Copy link
Contributor

ronen-fr commented Feb 2, 2025

QA run approved (see https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues69648)

@yuriw
Copy link
Contributor

yuriw commented Feb 3, 2025

jenkins test docs

@yuriw
Copy link
Contributor

yuriw commented Feb 3, 2025

@MaxKellermann it's ready for merge when all checks passed
ref: https://tracker.ceph.com/issues/69648

@ljflores ljflores removed the needs-qa label Feb 10, 2025
@MaxKellermann
Copy link
Member Author

MaxKellermann commented Feb 12, 2025

All CI tests have succeeded, only "ceph-pr-docs" which has a bogus failure. Thanks @yuriw and @ljflores.
I think that means it can finally be merged - and I'll try my new merge privileges that were granted to me this week for the first time.

@MaxKellermann MaxKellermann merged commit 1b47950 into ceph:main Feb 12, 2025
@MaxKellermann MaxKellermann deleted the misc_optimizations branch February 12, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@batrick batrick batrick approved these changes

Assignees

No one assigned

Labels

build/ops cephfs Ceph File System common core ready-to-merge TESTED

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MaxKellermann @ronen-fr @batrick @yuriw @ljflores