pivotal-jbarrett commented on a change in pull request #918:
URL: https://github.com/apache/geode-native/pull/918#discussion_r800946679
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -143,11 +148,26 @@ void DistributedSystemImpl::unregisterCliCallback(int
appdomainId) {
}
}
+std::string DistributedSystemImpl::getThreadName() {
Review comment:
You can use `const std::string&` once you use the `thread_local`
implementation.
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -38,6 +38,10 @@
#include "util/Log.hpp"
#include "version.h"
+namespace {
+std::map<std::thread::id, std::string> g_threadNames;
Review comment:
Replace with `static thread_local` member on `DistributedSystemImpl`:
Header:
```c++
class DistributedSystemImpl {
...
private:
static thread_local std::string threadName_;
}
```
Implementation:
```c++
std::string initThreadName() {
std::stringstream ss;
ss << std::this_thread::get_id();
return ss.str();
}
thread_local std::string DistributedSystemImpl::threadName_ =
initThreadName();
const std::string& DistributedSystemImpl::getThreadName() {
return threadName_;
}
void DistributedSystemImpl::setThreadName(const std::string& threadName) {
std::stringstream ss;
ss << std::this_thread::get_id() << " (" << threadName << ")";
threadName_ = ss.str();
}
```
I question the usefulness of putting the id in the threadName since when
logging we include both `std::thread::id` and this thread name already. I
suggest removing it which also simplifies the implementation.
Default thread name is `""`, empty string.
```c++
thread_local std::string DistributedSystemImpl::threadName_;
const std::string& DistributedSystemImpl::getThreadName() {
return threadName_;
}
void DistributedSystemImpl::setThreadName(std::string threadName) {
threadName_ = std::move(threadName);
}
```
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -143,11 +148,26 @@ void DistributedSystemImpl::unregisterCliCallback(int
appdomainId) {
}
}
+std::string DistributedSystemImpl::getThreadName() {
+ std::thread::id id = std::this_thread::get_id();
+ std::stringstream ss;
+ ss << id;
+ std::string threadName;
+ if (g_threadNames[id] != "") {
Review comment:
This is not a thread safe access to a non-thread safe data structure.
There is actually no reason to use a map with thread ID as key since that is
pretty much what `thread_local` is.
##########
File path: cppcache/src/DistributedSystemImpl.hpp
##########
@@ -48,6 +49,7 @@ using CliCallbackMethod = std::function<void(Cache&)>;
class DistributedSystemImpl {
public:
static void setThreadName(const std::string& threadName);
Review comment:
Maybe none of this should be on `DistributedSystemImpl`. It has nothing
to do with the Distributed System. Perhaps, since this is only used for
logging, it should be in the logging classes somewhere. It could also be its
own class/struct or just some global name spaced values and functions.
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -143,11 +148,26 @@ void DistributedSystemImpl::unregisterCliCallback(int
appdomainId) {
}
}
+std::string DistributedSystemImpl::getThreadName() {
+ std::thread::id id = std::this_thread::get_id();
+ std::stringstream ss;
+ ss << id;
+ std::string threadName;
+ if (g_threadNames[id] != "") {
+ threadName = ss.str() + " (" + g_threadNames[id] + ")";
+ } else {
+ threadName = ss.str();
+ }
+ return threadName;
Review comment:
Augmenting the thread name on each all is expensive for an output that
will effectively be constant. If the threadName needs to be augmented, though I
argue it doesn't, then it should be done on the less frequently executed
`setThreadName()` call.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
