[21612] Data sharing performance improvement #5209

paxifaer · 2024-09-07T10:25:32Z

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

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.

…ch (eProsima#5071) * Refs #20181: Add BB test Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #20181: Add Fix Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #20181: linter Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #20181. Pass in secure_endpoints as lambda capture. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #20181. New approach. Automatically sending DATA(p) when receiving a DATA(p) could lead to an infinite ping-pong between the two participants. This resulted in some cases in the transport threads eating all CPU resources. The new approach matches the discovered participant to the builtin non-secure PDP writer, so it will receive the DATA(p) of the local participant in the next periodic announcement. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #20181. Unmatch non-secure before matching secure. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: paxifaer <807128216@qq.com>

Signed-off-by: paxifaer <807128216@qq.com>

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Signed-off-by: paxifaer <807128216@qq.com>

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> Signed-off-by: paxifaer <807128216@qq.com>

* Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl) Signed-off-by: Matthias Schneider <ma30002000@yahoo.de> * Use steady_clock instead for system_clock for calculating timeouts Signed-off-by: Matthias Schneider <ma30002000@yahoo.de> * Use correct clock's duration for duration_cast Signed-off-by: Matthias Schneider <ma30002000@yahoo.de> * Use Time_t::now() Signed-off-by: Matthias Schneider <ma30002000@yahoo.de> * Fix build. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21314. Refactor on DataWriterImpl. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21314. Refactor on DataReaderImpl. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21314. Refactor on StatefulWriter. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21314. Protect current_time_since_unix_epoch against clock adjustments. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Revert "Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl)" This reverts commit d69eb91. --------- Signed-off-by: Matthias Schneider <ma30002000@yahoo.de> Signed-off-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: paxifaer <807128216@qq.com>

…d complete TypeObject (eProsima#5181) * Refs #21509. Fix Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #21322. Fixes for ROS2 Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #21322. Fix unit tests Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> --------- Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> Signed-off-by: paxifaer <807128216@qq.com>

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> Signed-off-by: paxifaer <807128216@qq.com>

…e 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. Signed-off-by: paxifaer <807128216@qq.com>

JesusPoderoso · 2024-09-09T14:34:48Z

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

paxifaer · 2024-09-13T01:44:21Z

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

Anybody review?

JesusPoderoso · 2024-09-13T05:45:50Z

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.

paxifaer · 2024-09-13T06:41:45Z

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.

MiguelCompany · 2024-09-13T06:45:55Z

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

paxifaer · 2024-09-19T02:09:49Z

@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

MiguelCompany · 2024-10-02T13:28:53Z

@Mergifyio rebase

…e 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. Signed-off-by: paxifaer <807128216@qq.com>

mergify · 2024-10-02T13:29:13Z

rebase

✅ Branch has been successfully rebased

MiguelCompany · 2024-10-02T13:35:15Z

src/cpp/rtps/DataSharing/DataSharingListener.cpp

+    // Using Acquire-Release semantics can avoid some blocking problems in traditional locking, such as deadlock and priority inversion. 
+    // This usually means higher concurrency performance because threads don't have to wait for locks to be released.
+    // Through Acquire-Release, you can achieve finer-grained control over specific variables or data structures without having to lock the entire resource or object, which can reduce contention and improve concurrency.
+    writer_pools_changed_.store(false,std::memory_order_release);

    // Loop on the writers looking for data not read yet
    for (auto it = writer_pools_.begin(); it != writer_pools_.end(); ++it)


I am a bit worried about the validity of this iterator changing during the next few lines (until we get to std::shared_ptr<ReaderPool> pool = it->pool;)

If there was a problem with effectiveness here, it would have been exposed before. I'll keep an eye on this later, and if there's a problem I'll fix it

The places where writer_pools_ is modified are protected by taking mutex_.
Your PR is removing the guard on the mutex_ during the traversal of writer_pools_ here.

The places where writer_pools_ is modified are protected by taking mutex_. Your PR is removing the guard on the mutex_ during the traversal of writer_pools_ here.

Don't worry about this, I use the lock_free mechanism here. It is thread safe.

writer_pools_ is a std::vector<WriterInfo> which is not thread-safe. If DataSharingListener::remove_datasharing_writer() is called on one thread while another thread is inside DataSharingListener::process_new_data(), an invalidated iterator on the vector could be dereferenced

you are right. But I tested it many times under multi-thread conditions and there was no problem.

I re-locked

MiguelCompany · 2024-10-02T13:35:47Z

src/cpp/rtps/DataSharing/DataSharingListener.cpp

+    notification_->notification_->new_data.store(false,std::memory_order_release);
+    // Using Acquire-Release semantics can avoid some blocking problems in traditional locking, such as deadlock and priority inversion. 
+    // This usually means higher concurrency performance because threads don't have to wait for locks to be released.
+    // Through Acquire-Release, you can achieve finer-grained control over specific variables or data structures without having to lock the entire resource or object, which can reduce contention and improve concurrency.
+    writer_pools_changed_.store(false,std::memory_order_release);


Fix uncrustify here

MiguelCompany · 2024-10-02T13:36:26Z

@paxifaer Please check my review here

Signed-off-by: paxifaer <807128216@qq.com>

…nto data_sharing Signed-off-by: paxifaer <807128216@qq.com>

Signed-off-by: paxifaer <807128216@qq.com>

paxifaer · 2024-10-03T06:47:31Z

@paxifaer Please check my review here

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

paxifaer · 2024-10-03T06:48:20Z

@paxifaer Please check my review here

Have your uncrustify configuration files changed?

…y in test. Signed-off-by: paxifaer <807128216@qq.com>

Signed-off-by: paxifaer <807128216@qq.com>

paxifaer closed this Sep 7, 2024

Mario-DL and others added 26 commits September 7, 2024 10:33

Fix the problem that gcc-9 cannot compile

e02a52d

Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.cpp

9c8bb04

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.cpp

eeb1381

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.cpp

d12a810

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

d2eacdb

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.cpp

b446cd4

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

242456a

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

3f179ac

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

291fa89

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

3dfdb20

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.cpp

75b7480

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.cpp

549d97a

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

91ed3a6

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.cpp

1e6d9aa

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.cpp

2ec01b6

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

ca455e2

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

f9153ef

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

8122e05

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Update src/cpp/fastdds/xtypes/dynamic_types/AnnotationDescriptorImpl.hpp

64e4230

Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Signed-off-by: paxifaer <807128216@qq.com>

Revert missing noexcept suggested by JesusPoderoso.

7a19061

Signed-off-by: paxifaer <807128216@qq.com>

Set string arguments as const references (eProsima#5179)

30328eb

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> Signed-off-by: paxifaer <807128216@qq.com>

Use eProsima-CI action to install Qt (eProsima#5186)

8063e60

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> Signed-off-by: paxifaer <807128216@qq.com>

paxifaer reopened this Sep 7, 2024

paxifaer force-pushed the data_sharing branch from a62506d to a2098d6 Compare September 7, 2024 10:40

JesusPoderoso added this to the v3.0.2 milestone Sep 9, 2024

JesusPoderoso changed the title ~~Data sharing~~ [21612] Data sharing performance improvement Sep 9, 2024

JesusPoderoso added the needs-review PR that is ready to be reviewed label Sep 10, 2024

paxifaer and others added 2 commits September 19, 2024 01:46

Merge branch 'master' into data_sharing

b62bc1b

Merge branch 'eProsima:master' into data_sharing

6de8155

MiguelCompany force-pushed the data_sharing branch from 6de8155 to 584cf12 Compare September 19, 2024 05:57

MiguelCompany modified the milestones: v3.0.2, v3.1.0 Oct 2, 2024

MiguelCompany force-pushed the data_sharing branch from 584cf12 to 34ba4c6 Compare October 2, 2024 13:29

MiguelCompany self-requested a review October 2, 2024 13:29

MiguelCompany requested changes Oct 2, 2024

View reviewed changes

paxifaer added 4 commits October 3, 2024 06:19

Fix uncrustify

9eaae61

Signed-off-by: paxifaer <807128216@qq.com>

Merge branch 'data_sharing' of https://github.com/paxifaer/Fast-DDS i…

860d8dd

…nto data_sharing Signed-off-by: paxifaer <807128216@qq.com>

fix uncrustify 2

1e95b84

Signed-off-by: paxifaer <807128216@qq.com>

fix duplicate code

0bc9d2e

Signed-off-by: paxifaer <807128216@qq.com>

rsanchez15 modified the milestones: v3.1.0, v3.1.1 Oct 3, 2024

paxifaer added 2 commits October 3, 2024 11:23

For thread safety, add locks. Mark the usage restrictions of zero cop…

4ce305e

…y in test. Signed-off-by: paxifaer <807128216@qq.com>

Mark the usage restrictions of zero copy in test.

fa752fa

Signed-off-by: paxifaer <807128216@qq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[21612] Data sharing performance improvement #5209

[21612] Data sharing performance improvement #5209

paxifaer commented Sep 7, 2024 •

edited by JesusPoderoso

Loading

JesusPoderoso commented Sep 9, 2024

paxifaer commented Sep 13, 2024

JesusPoderoso commented Sep 13, 2024

paxifaer commented Sep 13, 2024

MiguelCompany commented Sep 13, 2024

paxifaer commented Sep 19, 2024

MiguelCompany commented Oct 2, 2024

mergify bot commented Oct 2, 2024

MiguelCompany Oct 2, 2024

paxifaer Oct 3, 2024

MiguelCompany Oct 3, 2024

paxifaer Oct 3, 2024

MiguelCompany Oct 3, 2024

paxifaer Oct 3, 2024

paxifaer Oct 3, 2024

MiguelCompany Oct 2, 2024

MiguelCompany commented Oct 2, 2024

paxifaer commented Oct 3, 2024

paxifaer commented Oct 3, 2024

[21612] Data sharing performance improvement #5209

Are you sure you want to change the base?

[21612] Data sharing performance improvement #5209

Conversation

paxifaer commented Sep 7, 2024 • edited by JesusPoderoso Loading

Description

Contributor Checklist

Reviewer Checklist

JesusPoderoso commented Sep 9, 2024

paxifaer commented Sep 13, 2024

JesusPoderoso commented Sep 13, 2024

paxifaer commented Sep 13, 2024

MiguelCompany commented Sep 13, 2024

paxifaer commented Sep 19, 2024

MiguelCompany commented Oct 2, 2024

mergify bot commented Oct 2, 2024

✅ Branch has been successfully rebased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCompany commented Oct 2, 2024

paxifaer commented Oct 3, 2024

paxifaer commented Oct 3, 2024

paxifaer commented Sep 7, 2024 •

edited by JesusPoderoso

Loading