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/11/13 05:41:31 UTC
[GitHub] thrift pull request #1129: THRIFT-3038 clean up remaining volatile issues; r...
GitHub user jeking3 opened a pull request:
https://github.com/apache/thrift/pull/1129
THRIFT-3038 clean up remaining volatile issues; remove unused NoStarveReadWriteMutex class
This takes care of items 1, 3, and 6 in the PR 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/1129.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 #1129
----
commit 356be9a548a7fcd6bc8da9473b822c87bec71315
Author: James E. King, III <ji...@simplivity.com>
Date: 2016-11-13T04:55:50Z
THRIFT-3038 clean up remaining volatile issues; remove unused NoStarveReadWriteMutex class
----
---
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 #1129: THRIFT-3038 clean up remaining volatile issues
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1129
By moving the issue with NoStargeReadWriteMutex to THRIFT-3972 this became much simpler and it ready for approval.
---
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 #1129: THRIFT-3038 clean up remaining volatile issues; r...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1129#discussion_r87711314
--- Diff: lib/cpp/src/thrift/concurrency/Mutex.cpp ---
@@ -37,11 +37,10 @@ namespace concurrency {
#ifndef THRIFT_NO_CONTENTION_PROFILING
+static sig_atomic_t mutexProfilingCounter = 0;
static sig_atomic_t mutexProfilingSampleRate = 0;
--- End diff --
According to the comment in the implementation file, a race on this variable is acceptable, so I will change the type as you suggested and re-submit.
---
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 #1129: THRIFT-3038 clean up remaining volatile issues; r...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1129#discussion_r87709373
--- Diff: lib/cpp/src/thrift/concurrency/Mutex.h ---
@@ -101,25 +101,6 @@ class ReadWriteMutex {
boost::shared_ptr<impl> impl_;
};
-/**
- * A ReadWriteMutex that guarantees writers will not be starved by readers:
- * When a writer attempts to acquire the mutex, all new readers will be
- * blocked from acquiring the mutex until the writer has acquired and
- * released it. In some operating systems, this may already be guaranteed
- * by a regular ReadWriteMutex.
- */
-class NoStarveReadWriteMutex : public ReadWriteMutex {
-public:
- NoStarveReadWriteMutex();
-
- virtual void acquireRead() const;
- virtual void acquireWrite() const;
-
-private:
- Mutex mutex_;
- mutable volatile bool writerWaiting_;
-};
--- End diff --
It's in a "public" header (kind of).
Are you sure you delete this without deprecation warning etc ?
In this file, ReadWriteMutex and RWGuard are not used inside Thrift either.
How about removing or deprecating all the three classes in a separate issue,
since these are somewhat more involved compared to private volatile removal ?
---
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 #1129: THRIFT-3038 clean up remaining volatile issues
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1129
---
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 #1129: THRIFT-3038 clean up remaining volatile issues; r...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1129#discussion_r87707817
--- Diff: lib/cpp/src/thrift/concurrency/Mutex.cpp ---
@@ -37,11 +37,10 @@ namespace concurrency {
#ifndef THRIFT_NO_CONTENTION_PROFILING
+static sig_atomic_t mutexProfilingCounter = 0;
static sig_atomic_t mutexProfilingSampleRate = 0;
--- End diff --
Are we dealing with signal here at all ?
Otherwise, changing these to int/int32_t would be a good idea.
mutexProfilingCounter can arguably be atomic if we want to avoid data race (technically UB) in exchange for some performance penalty.
---
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.
---