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

Remove workaround for lack of std::atomic_init #996

Closed
wants to merge 1 commit into from

Conversation

JoeLoser
Copy link
Contributor

@JoeLoser JoeLoser commented Jan 12, 2019

Summary:

  • Since GCC 5 and later has std::atomic_init, remove the workaround
    present in Tearable.h to default initialize atomic variables.
  • Default initialization of atomics do not work as you would expect. See
    http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0883r0.pdf
    for the explanation why.
  • To get around the default initialization issue, we just call
    std::atomic_init for each element in the array of atomics.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yfeldblum has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yfeldblum
Copy link
Contributor

From internal review, pedantically, this leaves the atomic in uninitialized state.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0883r0.pdf

So Tearable would need either this intermediate type or to call std::atomic_init in a loop.

@JoeLoser
Copy link
Contributor Author

JoeLoser commented Jan 17, 2019

Thanks for the paper link; I was not aware of this gotcha. It's a bit unfortunate the default initialization for atomics is broken IMO. I like Herb's quote of

I have never seen the current behavior of deliberately failing to initialize an atomic (to the obvious default value of T{}) to be anything but a source of user surprise and a bug farm.

In any case, I decided to switch to using an array of atomics and explicitly call std::atomic_init for each element. I think this new behavior is a bit more explicit in our intent to default initialize the elements rather than through the intermediate type. What do you think?

folly/synchronization/Tearable.h Outdated Show resolved Hide resolved
folly/synchronization/Tearable.h Show resolved Hide resolved
Summary:
- Since GCC 5 and later has `std::atomic_init`, remove the workaround
  present in `Tearable.h` to default initialize atomic variables.
- Default initialization of atomics do not work as you would expect. See
  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0883r0.pdf
  for the explanation why.
- To get around the default initialization issue, we just call
  `std::atomic_init` for each element in the array of atomics.
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yfeldblum has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@JoeLoser JoeLoser deleted the atomic_init branch February 21, 2019 04:14
sandraiser pushed a commit to sandraiser/folly that referenced this pull request Mar 4, 2019
Summary:
- Since GCC 5 and later has `std::atomic_init`, remove the workaround
  present in `Tearable.h` to default initialize atomic variables.
- Default initialization of atomics do not work as you would expect. See
  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0883r0.pdf
  for the explanation why.
- To get around the default initialization issue, we just call
  `std::atomic_init` for each element in the array of atomics.
Pull Request resolved: facebook#996

Reviewed By: LeeHowes

Differential Revision: D13648263

Pulled By: yfeldblum

fbshipit-source-id: 6f3c84089f9158bc5c0ad5efac13d49ef69f1770
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.

3 participants