Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature add atomics #592

Merged
merged 19 commits into from
Apr 11, 2019
Merged

Feature add atomics #592

merged 19 commits into from
Apr 11, 2019

Conversation

timmah
Copy link
Contributor

@timmah timmah commented Apr 3, 2019

Background

  • Add atomic support for floating point types. This will be obsoleted when Draco adopts C++20.

Purpose of Pull Request

  • add support for atomic fetch-and-add for floating point types
  • it works, can't say it's great performance

Description of changes

  • Add ds++/atomics.hh (must...not...call it house_atomics.hh)
  • Add multithreaded unit test in ds++/test/tstatomics.cc that attempts to demonstrate correct use of atomics
  • Requires C++11 support

Status

src/ds++/DracoStrings.hh Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #592 into develop will increase coverage by <.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           develop    #592     +/-   ##
=========================================
+ Coverage     93.7%   93.7%   +<.1%     
=========================================
  Files          377     381      +4     
  Lines        17665   17805    +140     
=========================================
+ Hits         16554   16687    +133     
- Misses        1111    1118      +7

@timmah timmah changed the title WIP: Feature add atomics Feature add atomics Apr 8, 2019
@timmah timmah changed the title Feature add atomics WIP: Feature add atomics Apr 8, 2019
@timmah timmah changed the title WIP: Feature add atomics Feature add atomics Apr 8, 2019
Copy link
Collaborator

@KineticTheory KineticTheory left a comment

Choose a reason for hiding this comment

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

@timmah This looks really good. I have a couple of nit-picky changes that I want to see added before it is merged.

@atom @alexrlongne Can either of you review this PR?

src/ds++/atomics.hh Show resolved Hide resolved
src/ds++/atomics.hh Outdated Show resolved Hide resolved
src/ds++/atomics.hh Outdated Show resolved Hide resolved
src/ds++/atomics.hh Show resolved Hide resolved
src/ds++/atomics.hh Outdated Show resolved Hide resolved
src/ds++/test/tstatomics.cc Outdated Show resolved Hide resolved
src/ds++/test/tstatomics.cc Show resolved Hide resolved
@alexrlongne
Copy link
Contributor

@KineticTheory I agree with your suggestions, otherwise this looks good to me! I think it's great that the standard has this level of control for atomics. I don't quite understand why they don't provide these kinds of basic operations on atomic types.

@timmah
Copy link
Contributor Author

timmah commented Apr 9, 2019

I want to add a test of fetch_sub, maybe hold off before merging.

OK--added a test of fetch_sub (which revealed a mistake--woot)

@timmah timmah changed the title Feature add atomics WIP: Feature add atomics Apr 9, 2019
@timmah timmah changed the title WIP: Feature add atomics Feature add atomics Apr 9, 2019
@attom
Copy link
Contributor

attom commented Apr 10, 2019

This looks reasonable to me. I assume there's no problem using compare_exchange_weak instead of strong?

@KineticTheory KineticTheory merged commit b279b1a into lanl:develop Apr 11, 2019
@KineticTheory KineticTheory mentioned this pull request Jun 17, 2019
6 tasks
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.

4 participants