iox-#2177 Update SPSC SoFi

[albtam]

• add comments to clarify concurrent behavior
• fix memory orders

Pre-Review Checklist for the PR Author

1. Add a second reviewer for complex new features or larger refactorings
2. Code follows the coding style of CONTRIBUTING.md
3. Tests follow the best practice for testing
4. Changelog updated in the unreleased section including API breaking changes
5. Branch follows the naming format (iox-123-this-is-a-branch)
6. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
7. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
8. Relevant issues are linked
9. Add sensible notes for the reviewer
10. All checks have passed (except task-list-completed)
11. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
12. Assign PR to reviewer

Notes for Reviewer

I updated some memory orders and updated the comments to make sure future contributors understand the changes

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Closes Update SPSCSofi #2177
• Closes Bug in lock-free sofi #2160
• Closes Decipher weird comment in SoFi #1695

[albtam]

From #2178 (comment)

@elBoberido:

I could make your example pass miri by making both compare and swap loops use either compare_exchange or compare_exchange_weak.
But when increasing the number of pushes a lot it still detects (false positive?) races. The tests also need to be done in a different way, e.g. the cancelation points of the loop become more complex due to the overflow behavior.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.21%. Comparing base (a285e98) to head (8e0af98).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2214      +/-   ##
==========================================
- Coverage   78.22%   78.21%   -0.01%     
==========================================
  Files         433      433              
  Lines       16041    16033       -8     
  Branches     2301     2299       -2     
==========================================
- Hits        12548    12541       -7     
  Misses       2647     2647              
+ Partials      846      845       -1     

Flag	Coverage Δ
unittests	78.04% <100.00%> (-0.01%)	⬇️
unittests_timing	15.06% <46.15%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...concurrent/buffer/include/iox/detail/spsc_sofi.hpp	100.00% <ø> (ø)
...concurrent/buffer/include/iox/detail/spsc_sofi.inl	91.83% <100.00%> (+2.36%)	⬆️

... and 1 file with indirect coverage changes

[elBoberido]

Sorry for letting you wait so long for the review. I needed a fresh mind to review lock-free code :)

[elBoberido]

@albtam just to be sure that you are not waiting on feedback. I'll wait until the CI (at least the github actions) are green.

[albtam]

@albtam just to be sure that you are not waiting on feedback. I'll wait until the CI (at least the github actions) are green.

@elBoberido Back to you

[albtam]

@elBoberido Thanks. I addressed most of your comments. There are still some points to clarify

[elBoberido]

@albtam you can undo some of my suggestions 😅

[elBoberido]

This somehow becomes a never ending story but with the revert of the relaxed ordering in the places where acq_rel was required, we might be able to relax some of the acquire orderings. Unfortunately this is something which is hard to see when there are too many wrong orderings but once the ordering is at least as strong as required, we can identify the locations which definitely need the strong ordering and can try to relax some orderings which act as sync points when we identify new sync points or change the algorithm a bit.

[albtam]

I pushed some of your proposals. There is still something unclear. Lock-free is hard :P

[elBoberido]

Yes, lock-free is hard 😅

Btw, can you please adjust your commit messages with the issue number.

Before this is merged, it would be good to run test_stress_spsc_sofi. Ideally each of the three test for a few hours. The stress time can only be adjusted in the source code.

[albtam]

@elBoberido I implemented your last recommendations.

[elBoberido]

@albtam sorry, this totally slipped my attention. The changes look good. Did you have a chance to run the stress tests?
It would also be great if you could run the Miri tests with the current memiry orders.

[elBoberido]

@albtam did you have the chance to run the stress tests on an aarch64 CPU?

[albtam]

@elBoberido Sorry for the delay. I ran the stress tests (on x64 CPU). This test failed SpscSofiStress.SimultaneouslyPushAndPopOnEmptySoFi (both on master and my branch).

I don't have an aarch64 CPU, would it be possible for you to run the tests?

[orecham]

@elBoberido Sorry for the delay. I ran the stress tests (on x64 CPU). This test failed SpscSofiStress.SimultaneouslyPushAndPopOnEmptySoFi (both on master and my branch).

I don't have an aarch64 CPU, would it be possible for you to run the tests?

@albtam I can run it for you on my mac

[elBoberido]

If it runs without error on the mac, then this can be merged. But before it needs to be rebased. There is now an abstraction for the atomics which leads to a merge conflict. It should be easily solved, though.

@albtam can you post the output of the stress tests. I've run main and your branch and didn't encounter an error.

[albtam]

@elBoberido

Thanks

I've run main and your branch and didn't encounter an error.

I reran them and everything's fine.

I fixed the conflicts. Back to you

[elBoberido]

@albtam thanks for your contribution :)