You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/27 01:32:30 UTC
[GitHub] [geode-native] gaussianrecurrence opened a new pull request #776: GEODE-9078: Remove ACE_RW_Thread_Mutex mutexes
gaussianrecurrence opened a new pull request #776:
URL: https://github.com/apache/geode-native/pull/776
- Removed all ACE mutexes.
- TODO. Review mutex usage in PdxType as it is only used to read, so
either it's not necessary or we are missing write locks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence removed a comment on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence removed a comment on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830413106
> > ```
> > TODO. Review mutex usage in PdxType as it is only used to read, so
> > either it's not necessary or we are missing write locks.
> > ```
> >
> >
> > Anyone happens to know why is there a RW mutex to protect just read access? Seems quite odd to me :S
>
> I vaguely recall this. I think at the time I didn't have time to research why it only ever appears to acquire the read lock. If it only acquires a read lock then it's rather useless and should be removed. It's also possible that there should be a write lock somewhere.
What about
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830430021
I
> @gaussianrecurrence Big sorry! I think I have lead you down a habit hole. It looks like we never called out our deviations from the Google C++ Style Guide. I for one actually don't mind the `_` separator model in C++ but we had been preserving the `camelCase` model. We just adopted the trailing `_` for member variables. We have a few other deviations would should probably have documented too.
>
> https://github.com/apache/geode-native/blob/develop/CONTRIBUTING.md#style
>
> Even our examples avoid this because they are all single words.
>
> Let's see what other members of the team think on moving on fully to the Google `_` style?
>
> @pdxcodemonkey @mmartell @mreddington @echobravopapa @mivanac
I guess you mention it because of the make_lock methods right? I did follow another existing example to name the member function. However if you prefer to stick to the agreed camelCase notation, I can change it, no problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r623778380
##########
File path: cppcache/src/AdminRegion.hpp
##########
@@ -48,7 +48,7 @@ class AdminRegion : public std::enable_shared_from_this<AdminRegion> {
ThinClientBaseDM* m_distMngr;
std::string m_fullPath;
TcrConnectionManager* m_connectionMgr;
- ACE_RW_Thread_Mutex m_rwLock;
+ boost::shared_mutex mutex_;
Review comment:
I am trying to keep changes as minimal as possible. I have to confess every time I see something wrong, like this my OCD triggers and I am tempted to change it, but as I said I am trying to hold back hehe. However if you consider this is not a big change, I will update all the member variable names to match the current standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r623284694
##########
File path: cppcache/src/AdminRegion.hpp
##########
@@ -48,7 +48,7 @@ class AdminRegion : public std::enable_shared_from_this<AdminRegion> {
ThinClientBaseDM* m_distMngr;
std::string m_fullPath;
TcrConnectionManager* m_connectionMgr;
- ACE_RW_Thread_Mutex m_rwLock;
+ boost::shared_mutex mutex_;
Review comment:
Please match the existing member variable format or update them all to the current standard.
##########
File path: cppcache/src/RemoteQuery.cpp
##########
@@ -125,7 +125,7 @@ GfErrType RemoteQuery::executeNoThrow(
ThinClientBaseDM* tcdm, std::shared_ptr<CacheableVector> paramList) {
LOGFINEST("%s: executing query: %s", func, m_queryString.c_str());
- TryReadGuard guard(m_queryService->getLock(), m_queryService->invalid());
+ TryReadGuard guard(m_queryService->getMutex(), m_queryService->invalid());
Review comment:
This always felt very wrong to me. It's not just that locks across components is a bad smell but that we leave the leak the locking logic too. What if there was an `tryAcquireReadLock` type method that returned a guard object?
##########
File path: cppcache/src/DataOutputInternal.hpp
##########
@@ -23,8 +23,6 @@
#include <geode/DataOutput.hpp>
#include <geode/PoolManager.hpp> // TODO remove
-#include "CacheImpl.hpp" // TODO remove
Review comment:
Yes!
##########
File path: cppcache/src/ReadWriteLock.cpp
##########
@@ -23,28 +23,28 @@ namespace apache {
namespace geode {
namespace client {
-TryReadGuard::TryReadGuard(ACE_RW_Thread_Mutex& lock,
- const volatile bool& exitCondition)
- : lock_(lock), isAcquired_(false) {
+TryReadGuard::TryReadGuard(boost::shared_mutex& mutex,
+ const volatile bool& exit_cond)
+ : mutex_{mutex}, locked_{false} {
do {
- if (lock_.tryacquire_read() != -1) {
- isAcquired_ = true;
+ if (mutex_.try_lock_shared()) {
+ locked_ = true;
break;
}
std::this_thread::yield();
- } while (!exitCondition);
+ } while (!exit_cond);
}
-TryWriteGuard::TryWriteGuard(ACE_RW_Thread_Mutex& lock,
- const volatile bool& exitCondition)
- : lock_(lock), isAcquired_(false) {
+TryWriteGuard::TryWriteGuard(boost::shared_mutex& mutex,
+ const volatile bool& exit_cond)
+ : mutex_{mutex}, locked_{false} {
Review comment:
What do you think about replacing this hot spin try lock with a condition variable that is signaled. Almost all these are around some shutdown flag, which doesn't change frequently so if the lock is contended we are just eating CPU for nothing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey merged pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #776:
URL: https://github.com/apache/geode-native/pull/776
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-842999229
Is there anything else here to do? Maybe @pdxcodemonkey, you want to review it? Or can it be merged already? Thanks! :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830434263
> @gaussianrecurrence The sub group of people working on geode-native has historically been a VERY small group and we did a very poor job communicating outward and recording some of these variations. I wonder if for the sake of not holding up this PR if you should just blackout that variable renaming commit, sorry, and we start an open discussion on the dev@geode list on the C++ style guide. We should discuss deviations we want and how far we should go to try to refactor code to fit it. Then we should record the consensus in the CONTRIBUTING.md.
Yes, I also think that would be the fastest approach for this PR. Will create a new PR with that change and solving Windows compilation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence edited a comment on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence edited a comment on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830434263
> @gaussianrecurrence The sub group of people working on geode-native has historically been a VERY small group and we did a very poor job communicating outward and recording some of these variations. I wonder if for the sake of not holding up this PR if you should just blackout that variable renaming commit, sorry, and we start an open discussion on the dev@geode list on the C++ style guide. We should discuss deviations we want and how far we should go to try to refactor code to fit it. Then we should record the consensus in the CONTRIBUTING.md.
Yes, I also think that would be the fastest approach for this PR. Will create a new revision with that change and solving Windows compilation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830433225
@gaussianrecurrence The sub group of people working on geode-native has historically been a VERY small group and we did a very poor job communicating outward and recording some of these variations. I wonder if for the sake of not holding up this PR if you should just blackout that variable renaming commit, sorry, and we start an open discussion on the dev@geode list on the C++ style guide. We should discuss deviations we want and how far we should go to try to refactor code to fit it. Then we should record the consensus in the CONTRIBUTING.md.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-811333206
> @gaussianrecurrence Please rebase onto `develop` and we will review. We merged a couple of old outstanding PRs this morning, one of which was _huge_, so there are conflicts to resolve.
Will do. However note that there is one thing to look into:
```
TODO. Review mutex usage in PdxType as it is only used to read, so
either it's not necessary or we are missing write locks.
```
Anyone happens to know why is there a RW mutex to protect just read access? Seems quite odd to me :S
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r631224771
##########
File path: cppcache/src/RemoteQuery.cpp
##########
@@ -125,11 +127,11 @@ GfErrType RemoteQuery::executeNoThrow(
ThinClientBaseDM* tcdm, std::shared_ptr<CacheableVector> paramList) {
LOGFINEST("%s: executing query: %s", func, m_queryString.c_str());
- TryReadGuard guard(m_queryService->getLock(), m_queryService->invalid());
-
+ boost::shared_lock<boost::shared_mutex> guard{m_queryService->getMutex()};
Review comment:
> Were you going to change these patterns to return a `shared_lock` rather than the `shared_mutex`?
I tried to change it, but I found a really strange issue in the .Net part:
- An exception with message "The string binding is invalid" was being thrown due to the fact that
the Apache.Geode DLL has mixed code and compiles with CLR enabled.
That's something dis-recommended by Boost because under certain
conditions could cause problems with boost static variables.
In this case the issue was related to one of the boost::exception
static variables.
This issue was solved by removing header inclusion introduced in
revision 3.
- Due to above issue, exposing the lock rather than the mutex is not
possible for now, until a further refactor is made to .Net code, so
mutex/lock exposition from revision 3 has been reverted.
So I had to revert that particular change. I am creating an issue so we can tackle this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-829524591
> ```
> TODO. Review mutex usage in PdxType as it is only used to read, so
> either it's not necessary or we are missing write locks.
> ```
>
> Anyone happens to know why is there a RW mutex to protect just read access? Seems quite odd to me :S
I vaguely recall this. I think at the time I didn't have time to research why it only ever appears to acquire the read lock. If it only acquires a read lock then it's rather useless and should be removed. It's also possible that there should be a write lock somewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence removed a comment on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence removed a comment on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-811313472
> @gaussianrecurrence Please rebase onto `develop` and we will review. We merged a couple of old outstanding PRs this morning, one of which was _huge_, so there are conflicts to resolve.
Will do. However note that there is one thing to look into:
```
TODO. Review mutex usage in PdxType as it is only used to read, so
either it's not necessary or we are missing write locks.
```
Anyone happens to know why is there a RW mutex to protect just read access? Seems quite odd to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r623806969
##########
File path: cppcache/src/RemoteQuery.cpp
##########
@@ -125,7 +125,7 @@ GfErrType RemoteQuery::executeNoThrow(
ThinClientBaseDM* tcdm, std::shared_ptr<CacheableVector> paramList) {
LOGFINEST("%s: executing query: %s", func, m_queryString.c_str());
- TryReadGuard guard(m_queryService->getLock(), m_queryService->invalid());
+ TryReadGuard guard(m_queryService->getMutex(), m_queryService->invalid());
Review comment:
Totally agree. Will do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r631220291
##########
File path: cppcache/src/RemoteQuery.cpp
##########
@@ -125,11 +127,11 @@ GfErrType RemoteQuery::executeNoThrow(
ThinClientBaseDM* tcdm, std::shared_ptr<CacheableVector> paramList) {
LOGFINEST("%s: executing query: %s", func, m_queryString.c_str());
- TryReadGuard guard(m_queryService->getLock(), m_queryService->invalid());
-
+ boost::shared_lock<boost::shared_mutex> guard{m_queryService->getMutex()};
Review comment:
Were you going to change these patterns to return a `shared_lock` rather than the `shared_mutex`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830430954
> I guess you mention it because of the make_lock methods right? I did follow another existing example to name the member function. However if you prefer to stick to the agreed camelCase notation, I can change it, no problem.
No, camelCase on method names are in the Style Guide. I mean the member and local variable names.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r623969622
##########
File path: cppcache/src/AdminRegion.hpp
##########
@@ -48,7 +48,7 @@ class AdminRegion : public std::enable_shared_from_this<AdminRegion> {
ThinClientBaseDM* m_distMngr;
std::string m_fullPath;
TcrConnectionManager* m_connectionMgr;
- ACE_RW_Thread_Mutex m_rwLock;
+ boost::shared_mutex mutex_;
Review comment:
I have the exact same problem. If the changes are small I just do them. If they are big-sh then I do them on a separate commit so the reviewers can focus on real change first and then the reformats/refactors separately. If the changes are really big I keep the same style and then open a new ticket to do a mass cleanup of the file(s). It's a judgement call.
For this one I would probably just keep the old style for consistency, which really hurts me but less than having a mix of styles.
Your call though really...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r623786676
##########
File path: cppcache/src/ReadWriteLock.cpp
##########
@@ -23,28 +23,28 @@ namespace apache {
namespace geode {
namespace client {
-TryReadGuard::TryReadGuard(ACE_RW_Thread_Mutex& lock,
- const volatile bool& exitCondition)
- : lock_(lock), isAcquired_(false) {
+TryReadGuard::TryReadGuard(boost::shared_mutex& mutex,
+ const volatile bool& exit_cond)
+ : mutex_{mutex}, locked_{false} {
do {
- if (lock_.tryacquire_read() != -1) {
- isAcquired_ = true;
+ if (mutex_.try_lock_shared()) {
+ locked_ = true;
break;
}
std::this_thread::yield();
- } while (!exitCondition);
+ } while (!exit_cond);
}
-TryWriteGuard::TryWriteGuard(ACE_RW_Thread_Mutex& lock,
- const volatile bool& exitCondition)
- : lock_(lock), isAcquired_(false) {
+TryWriteGuard::TryWriteGuard(boost::shared_mutex& mutex,
+ const volatile bool& exit_cond)
+ : mutex_{mutex}, locked_{false} {
Review comment:
You definetly have a point. This will require quite some changes, but I'll make it work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r623970799
##########
File path: cppcache/src/ReadWriteLock.cpp
##########
@@ -23,28 +23,28 @@ namespace apache {
namespace geode {
namespace client {
-TryReadGuard::TryReadGuard(ACE_RW_Thread_Mutex& lock,
- const volatile bool& exitCondition)
- : lock_(lock), isAcquired_(false) {
+TryReadGuard::TryReadGuard(boost::shared_mutex& mutex,
+ const volatile bool& exit_cond)
+ : mutex_{mutex}, locked_{false} {
do {
- if (lock_.tryacquire_read() != -1) {
- isAcquired_ = true;
+ if (mutex_.try_lock_shared()) {
+ locked_ = true;
break;
}
std::this_thread::yield();
- } while (!exitCondition);
+ } while (!exit_cond);
}
-TryWriteGuard::TryWriteGuard(ACE_RW_Thread_Mutex& lock,
- const volatile bool& exitCondition)
- : lock_(lock), isAcquired_(false) {
+TryWriteGuard::TryWriteGuard(boost::shared_mutex& mutex,
+ const volatile bool& exit_cond)
+ : mutex_{mutex}, locked_{false} {
Review comment:
We don't necessarily have to do it immediately. You could keep this in your back pocket as a future cleanup of this helper. I have wanted to fix this one for a while but just keep punting it myself. Your steps to make it boost based make it that much closer to reality though. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence closed pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence closed pull request #776:
URL: https://github.com/apache/geode-native/pull/776
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-811178222
@gaussianrecurrence Please rebase onto `develop` and we will review. We merged a couple of old outstanding PRs this morning, one of which was _huge_, so there are conflicts to resolve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830423008
@gaussianrecurrence Big sorry! I think I have lead you down a habit hole. It looks like we never called out our deviations from the Google C++ Style Guide. I for one actually don't mind the `_` separator model in C++ but we had been preserving the `camelCase` model. We just adopted the trailing `_` for member variables. We have a few other deviations would should probably have documented too.
https://github.com/apache/geode-native/blob/develop/CONTRIBUTING.md#style
Even our examples avoid this because they are all single words.
Let's see what other members of the team think on moving on fully to the Google `_` style?
@pdxcodemonkey @mmartell @mreddington @echobravopapa @mivanac
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830413106
> > ```
> > TODO. Review mutex usage in PdxType as it is only used to read, so
> > either it's not necessary or we are missing write locks.
> > ```
> >
> >
> > Anyone happens to know why is there a RW mutex to protect just read access? Seems quite odd to me :S
>
> I vaguely recall this. I think at the time I didn't have time to research why it only ever appears to acquire the read lock. If it only acquires a read lock then it's rather useless and should be removed. It's also possible that there should be a write lock somewhere.
What about
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r631226162
##########
File path: cppcache/src/RemoteQuery.cpp
##########
@@ -125,11 +127,11 @@ GfErrType RemoteQuery::executeNoThrow(
ThinClientBaseDM* tcdm, std::shared_ptr<CacheableVector> paramList) {
LOGFINEST("%s: executing query: %s", func, m_queryString.c_str());
- TryReadGuard guard(m_queryService->getLock(), m_queryService->invalid());
-
+ boost::shared_lock<boost::shared_mutex> guard{m_queryService->getMutex()};
Review comment:
.NET 😢 , got it, say no more!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-830414699
Sorry for the spam. I closed the PR by mistake. Luckily I was able to re-open it.
Regarding changes requested by you @pivotal-jbarrett, I finally made all of them as suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-841298887
Regarding the pending topic we had related to having only read locks in PdxType, I went through the possible cases in which PdxType state is written and where is read I came to the following conclusion (Sorry for the long read):
## Cases in which PdxType state is accessed
- **toData**
- **(fixed|variable)LengthFieldPosition**
- **get(RemoteToLocal|LocalToRemote)Map**. This it uses lazy initialization, that's why is considered here.
- **isContains**
- **clone**
- **is(Local|Remote)TypeContains**
- **mergeVersion**
- **operator==**
If PdxType requires exlusive access for those cases where its state is mutated, then in all of the above cases, a shared access lock will be required.
## Cases in which PdxType state is mutated
- **fromData**. This is only used while requesting a PdxType to the server, so no other thread will access this type. Hence, this function does not require exclusive access.
- **add(Fixed|Variable)LengthTypeField** are only used while creating the PdxType, so it won't require exclusive access.
- **InitializeType** is called always after a PdxType has been created or fetched from the server, so it won't require exclusive access.
- **init(LocalToRemote|RemoteToLocal)** is called in get(LocalToRemote|RemoteToLocal)Map and InitializeType. Given InitializeType does not require exclusive access, whether or not init(LocalToRemote|RemoteToLocal) requires exclusive access depends on get(LocalToRemote|RemoteToLocal)Map.
- **get(RemoteToLocal|LocalToRemote)Map** uses lazy initialization, meaning that if init(LocalToRemote|RemoteToLocal) was previously called, it won't mutate the state, and it's used by:
- **PdxRemoteWriter::PdxRemoteWriter**
- **PdxRemoteWriter::writeUnreadFields**
- **PdxLocalReader::PdxLocalReader**
- **PdxLocalReader::getPreservedData**
- **PdxRemoteReader::PdxRemoteReader** (by inheritance)
- **PdxReaderWithTypeCollector::PdxReaderWithTypeCollector** (by inheritance)
### **get(RemoteToLocal|LocalToRemote)Map** follow up
So, regarding **PdxRemoteWriter**, the PdxType is either initialized to:
- A merged PdxType which is initialized in all cases.
- A local PdxType which is initialized in all cases
Regarding **PdxLocalReader**, **PdxRemoteReader**, **PdxReaderWithTypeCollector**, it uses a PdxType instance which might be fetched from the PdxTypeRegistry, so the exclusive access depends on whether all the PdxTypes added to the PdxTypeRegistry are initialized.
All the calls to PdxTypeRegistry::addPdxType are in PdxHelper and in all cases PdxTypes are initialized.
Consequently, I can conclude that **get(RemoteToLocal|LocalToRemote)Map** does not require exlusive access.
With all of the above, I'd say there is no need to use locks in PdxTypes, so I will remove the mutex and shared lock from that class.
I guess probably those locks were left in the code by mistake after some refactor. I sadly could not trace the exact commit, as the change seems to have been done before open sourcing the native client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #776: GEODE-9078: Remove ACE mutexes
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #776:
URL: https://github.com/apache/geode-native/pull/776#issuecomment-811313472
> @gaussianrecurrence Please rebase onto `develop` and we will review. We merged a couple of old outstanding PRs this morning, one of which was _huge_, so there are conflicts to resolve.
Will do. However note that there is one thing to look into:
```
TODO. Review mutex usage in PdxType as it is only used to read, so
either it's not necessary or we are missing write locks.
```
Anyone happens to know why is there a RW mutex to protect just read access? Seems quite odd to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org