Fast-DDS icon indicating copy to clipboard operation
Fast-DDS copied to clipboard

[21612] Data sharing performance improvement

Open paxifaer opened this issue 1 year ago • 11 comments

Description

Compared with the traditional separate locking method, Acquire-Release read-write consistency provides a better solution in terms of performance, readability, scalability and memory consistency. When using ordinary locking mechanisms, it is often necessary to introduce global locks to protect shared resources, but this is inefficient. Acquire-Release allows multiple threads to safely operate shared data without complete mutual exclusion.

Contributor Checklist

  • [ ] Commit messages follow the project guidelines.
  • [ ] The code follows the style guidelines of this project.
  • [ ] Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • [ ] Any new/modified methods have been properly documented using Doxygen.
  • [ ] Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • [ ] Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • [ ] Changes are API compatible.
  • [ ] New feature has been added to the versions.md file (if applicable).
  • [ ] New feature has been documented/Current behavior is correctly described in the documentation.
  • [ ] Applicable backports have been included in the description.

Reviewer Checklist

  • [ ] The PR has a milestone assigned.
  • [ ] The title and description correctly express the PR's purpose.
  • [ ] Check contributor checklist is correct.
  • [ ] If this is a critical bug fix, backports to the critical-only supported branches have been requested.
  • [ ] Check CI results: changes do not issue any warning.
  • [ ] Check CI results: failing tests are unrelated with the changes.

paxifaer avatar Sep 07 '24 10:09 paxifaer

Hi @paxifaer, thanks for our contribution. The eProsima team will review the PR in the following days.

JesusPoderoso avatar Sep 09 '24 14:09 JesusPoderoso

Hi @paxifaer, thanks for our contribution. The eProsima team will review the PR in the following days.

Anybody review?

paxifaer avatar Sep 13 '24 01:09 paxifaer

Hi @paxifaer

Anybody review?

Please, be patient. We have a considerable workload caused by the v3 release. In the following weeks, we will review your PR.

JesusPoderoso avatar Sep 13 '24 05:09 JesusPoderoso

Hi @paxifaer

Anybody review?

Please, be patient. We have a considerable workload caused by the v3 release. In the following weeks, we will review your PR.

hhh,ok,I just worry about that you forget it.I will be patient.Go head.

paxifaer avatar Sep 13 '24 06:09 paxifaer

@paxifaer In the meantime, could you please rebase this on top of master, leaving only the relevant changes (i.e. the last commit)?

MiguelCompany avatar Sep 13 '24 06:09 MiguelCompany

@paxifaer In the meantime, could you please rebase this on top of master, leaving only the relevant changes (i.e. the last commit)?

Is it okay for me to do this? I have merged the latest code

paxifaer avatar Sep 19 '24 02:09 paxifaer

@mergifyio rebase

MiguelCompany avatar Oct 02 '24 13:10 MiguelCompany

rebase

✅ Branch has been successfully rebased

mergify[bot] avatar Oct 02 '24 13:10 mergify[bot]

@paxifaer Please check my review here

MiguelCompany avatar Oct 02 '24 13:10 MiguelCompany

@paxifaer Please check my review here

I adapted the code the way you did. Why does it still go wrong?

paxifaer avatar Oct 03 '24 06:10 paxifaer

@paxifaer Please check my review here

Have your uncrustify configuration files changed?

paxifaer avatar Oct 03 '24 06:10 paxifaer

I have fixed my mistake about thread safe.

paxifaer avatar Oct 30 '24 03:10 paxifaer

I have fixed my mistake about thread safe.

So now the code is almost the same as it was before, and since writer_pools_changed_ is always written with the mutex taken, it makes sense to leave the accesses with std::memory_order_relaxed.

That would leave the code as it is, and then this PR is not required.

I am thus closing the PR. You can open a different one for the Container Tips on the performance test README files.

MiguelCompany avatar Oct 30 '24 11:10 MiguelCompany