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/07/07 19:17:39 UTC
[GitHub] [geode-native] pdxcodemonkey opened a new pull request #830: GEODE-8891: Make static regex a Meyers Singleton in ThinClientRegion
pdxcodemonkey opened a new pull request #830:
URL: https://github.com/apache/geode-native/pull/830
- Latest compiler/runtime on RHEL-8 causes an order-of-initialization
bug for this static regex, and will core dump at app startup for
optimized builds.
- Changing to Meyers singleton ensures regex variable isn't initialized
until the first time we use it, long after all static initializers are
called.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #830: GEODE-8891: Make static regex a Meyers Singleton in ThinClientRegion
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #830:
URL: https://github.com/apache/geode-native/pull/830#discussion_r665671236
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -589,7 +595,7 @@ std::shared_ptr<SelectResults> ThinClientRegion::query(
}
std::string squery;
- if (std::regex_search(predicate, PREDICATE_IS_FULL_QUERY_REGEX)) {
+ if (std::regex_search(predicate, IsFullQueryRegex::instance())) {
Review comment:
I am not quite following this argument since it seems to argue for both options. The Meyers singleton won't initialize the static until first use so there is no burden to the test harness as a whole.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mreddington commented on a change in pull request #830: GEODE-8891: Make static regex a Meyers Singleton in ThinClientRegion
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #830:
URL: https://github.com/apache/geode-native/pull/830#discussion_r665669483
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -589,7 +595,7 @@ std::shared_ptr<SelectResults> ThinClientRegion::query(
}
std::string squery;
- if (std::regex_search(predicate, PREDICATE_IS_FULL_QUERY_REGEX)) {
+ if (std::regex_search(predicate, IsFullQueryRegex::instance())) {
Review comment:
Why wrap the static const in a singleton? That sounds like an even more complicated solution. You could have just made this regex a local const variable. Who cares about runtime performance when this isn't where we're slow? The class still needs to be initialized at runtime anyway, as you've found out the hard way, and instead of burdening the whole test harness with the initialization of a global static, you could isolate that initialization to the instantiation of this test when it's ran.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #830: GEODE-8891: Make static regex a Meyers Singleton in ThinClientRegion
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #830:
URL: https://github.com/apache/geode-native/pull/830#discussion_r665663993
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -49,8 +49,14 @@ namespace apache {
namespace geode {
namespace client {
-static const std::regex PREDICATE_IS_FULL_QUERY_REGEX(
- "^\\s*(?:select|import)\\b", std::regex::icase);
+class IsFullQueryRegex {
Review comment:
I guess I would have just don it within the `ThinClientRegion` class and made the method `private` since there is no need for it to be public (well maybe for tests but there are none).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #830: GEODE-8891: Make static regex a Meyers Singleton in ThinClientRegion
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #830:
URL: https://github.com/apache/geode-native/pull/830#discussion_r665762038
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -589,7 +595,7 @@ std::shared_ptr<SelectResults> ThinClientRegion::query(
}
std::string squery;
- if (std::regex_search(predicate, PREDICATE_IS_FULL_QUERY_REGEX)) {
+ if (std::regex_search(predicate, IsFullQueryRegex::instance())) {
Review comment:
Given this value doesn't have any use outside this method I could agree with making it a local const. There is technically no different between that and the proposed implementation other than encapsulation of the const logic in the additional method.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #830: GEODE-8891: Make static regex a Meyers Singleton in ThinClientRegion
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #830:
URL: https://github.com/apache/geode-native/pull/830#discussion_r665755714
##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -589,7 +595,7 @@ std::shared_ptr<SelectResults> ThinClientRegion::query(
}
std::string squery;
- if (std::regex_search(predicate, PREDICATE_IS_FULL_QUERY_REGEX)) {
+ if (std::regex_search(predicate, IsFullQueryRegex::instance())) {
Review comment:
If initializing a constant something is relatively expensive (i.e., a significant cost relative to its use), I'd prefer to initialize it only once, so I can amortize the cost of initialization.
In this case, I believe mreddington is advocating just declaring it locally inside of functions that use it, and not worry about the cost. Although calling the regex CTOR is likely a small percentage of the total test time, having to recreate a regex that's const seems silly.
If you declare the regex as a const static inside the function that uses it, then it will be initialized only once when the function is first used, thereby solving any cost concern associated with a regex CTOR. Also, C++11 guarantees that dynamic initialization of function-local statics is thread-safe, in case two threads call the function using the local regex at the same time.
So my recommendation is to simply move the original 2 lines of code inside the function that uses it.
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey merged pull request #830: GEODE-8891: Move static regex in ThinClientRegion
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #830:
URL: https://github.com/apache/geode-native/pull/830
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org