You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jeking3 <gi...@git.apache.org> on 2016/04/06 17:18:56 UTC
[GitHub] thrift pull request: THRIFT-3038: fix up some volatiles in cpp
GitHub user jeking3 opened a pull request:
https://github.com/apache/thrift/pull/981
THRIFT-3038: fix up some volatiles in cpp
I addressed the concerns in TFileTransport (forceFlush_) and in TThreadPoolServer, so items #2, #4, #5 from the original issue description.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift defect/THRIFT-3038
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/981.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #981
----
commit cb4849abc4156d64532989344a4b2c2fa8be8148
Author: Jim King <ji...@simplivity.com>
Date: 2016-04-05T17:00:24Z
THRIFT-3038: fix a couple races and removed volatile per analysis
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the issue:
https://github.com/apache/thrift/pull/981
Seeing the [commit log](https://github.com/apache/thrift/commit/05b19b57cf2e224ab49e5cf983677361acb18311#diff-487498268b6e5a491d25456332fd44a1R107), it looks like more about not to use `acquire` for `store` and `release` for `load` (it's swapped).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on the issue:
https://github.com/apache/thrift/pull/981
Unless you have extremely compelling profiling results, the only atomic memory order you should be using is memory_order_seq_cst. Even better, you get seq_cst for free as the default parameter of load and store.
memory_order_consume is almost entirely broken in C++11 and 14. Basically all compilers promote it to an acquire instead of a consume.
memory_order_relaxed is extremely difficult to reason about, and should be avoided.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #981: THRIFT-3038: fix up some volatiles in cpp
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/981
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/981
Are you suggesting I get rid of the load and store calls and instead just use the provided "operator=" from the atomic? I'm okay with that... it is much more readable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/981
I am okay letting things default to something else here. I followed the boost examples and it passed the build with those settings.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on the issue:
https://github.com/apache/thrift/pull/981
The default op=, and even the default conversion operators from atomic<T> to T are fine.
@nsuke I would prefer we keep the access to TThreadPoolServer timeout variables as seq_cst. The setter operations shouldn't ever be in performance critical code, and the 'get' side of things is happening in an already slow area. On x86, the getters will still turn into regular 'mov' operations. On ARM, there will be a couple of memory barriers generated, but those shouldn't be a huge deal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #981: THRIFT-3038: fix up some volatiles in cpp
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/981#discussion_r71079188
--- Diff: lib/cpp/src/thrift/transport/TFileTransport.h ---
@@ -352,7 +352,7 @@ class TFileTransport : public TFileReaderTransport, public TFileWriterTransport
// To keep track of whether the buffer has been flushed
Monitor flushed_;
- volatile bool forceFlush_;
+ bool forceFlush_;
--- End diff --
We're doing `while (forceFlush_) {`, so we need atomic here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the issue:
https://github.com/apache/thrift/pull/981
@jeking3 the atomics for TThreadPoolServer fields are for 64-bit field read-write on 32bit-x86 and other platforms ? If so, we only need relaxed.
That said, as @ben-craig suggested, leaviing it as default strict order (+without `load` and `store`) would even be better for readability.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/981
Quite interesting, the job that failed is related to the code changes but the error message comes from a version of boost before 1.53. It looks like that build job isn't making sure boost is at the minimum required version.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on the issue:
https://github.com/apache/thrift/pull/981
Your latest revision look fine, I was mainly trying to address nsuke's post.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #981: THRIFT-3038: fix up some volatiles in cpp
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/981#discussion_r71095982
--- Diff: lib/cpp/src/thrift/transport/TFileTransport.h ---
@@ -352,7 +352,7 @@ class TFileTransport : public TFileReaderTransport, public TFileWriterTransport
// To keep track of whether the buffer has been flushed
Monitor flushed_;
- volatile bool forceFlush_;
+ bool forceFlush_;
--- End diff --
Thanks for catching that - I pushed an update changing this to a boost::atomic<bool> type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/981
@ben-craig are you saying additional code changes are needed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/981
The latest build failure was environmental and passed all other tests:
```
make[3]: Leaving directory `/thrift/src/lib/lua'
make[3]: write error
make[2]: *** [check-recursive] Error 1
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---