[
https://issues.apache.org/jira/browse/GEODE-8213?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17125993#comment-17125993
]
ASF GitHub Bot commented on GEODE-8213:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #611:
URL: https://github.com/apache/geode-native/pull/611#discussion_r435327033
##########
File path: cppcache/src/SerializationRegistry.hpp
##########
@@ -73,16 +72,14 @@ using internal::DataSerializableInternal;
using internal::DataSerializablePrimitive;
class TheTypeMap {
- std::unordered_map<internal::DSCode, TypeFactoryMethod>
+ std::shared_ptr<std::unordered_map<internal::DSCode, TypeFactoryMethod>>
Review comment:
I am pretty sure you can't perform `std::atomic` operations on a
`std::shared_ptr` and expect to get the lifecycle management of
`std::shared_ptr`. If you use `std::atomic` to swap out the contents behind
this value nothing will update the ref counter and release the memory of the
pointed to heap.
##########
File path: cppcache/src/SerializationRegistry.cpp
##########
@@ -355,45 +363,39 @@ std::shared_ptr<Serializable>
SerializationRegistry::GetEnum(
}
void TheTypeMap::clear() {
- std::lock_guard<util::concurrent::spinlock_mutex> guard(
- m_dataSerializableMapLock);
- m_dataSerializableMap.clear();
+ auto tempDataSerializableMap =
+ std::make_shared<std::unordered_map<int32_t, TypeFactoryMethod>>();
+ std::atomic_store(&m_dataSerializableMap, tempDataSerializableMap);
- std::lock_guard<util::concurrent::spinlock_mutex> guard2(
- m_dataSerializableFixedIdMapLock);
- m_dataSerializableFixedIdMap.clear();
+ auto tempDataSerializableFixedId = std::make_shared<
+ std::unordered_map<internal::DSFid, TypeFactoryMethod>>();
+ std::atomic_store(&m_dataSerializableFixedIdMap,
tempDataSerializableFixedId);
- std::lock_guard<util::concurrent::spinlock_mutex> guard3(
- m_pdxSerializableMapLock);
- m_pdxSerializableMap.clear();
+ auto tempPdxSerializableMap =
+ std::make_shared<std::unordered_map<std::string,
TypeFactoryMethodPdx>>();
+ std::atomic_store(&m_pdxSerializableMap, tempPdxSerializableMap);
}
void TheTypeMap::findDataSerializable(int32_t id,
TypeFactoryMethod& func) const {
- std::lock_guard<util::concurrent::spinlock_mutex> guard(
- m_dataSerializableMapLock);
- const auto& found = m_dataSerializableMap.find(id);
- if (found != m_dataSerializableMap.end()) {
+ const auto& found = m_dataSerializableMap->find(id);
+ if (found != m_dataSerializableMap->end()) {
Review comment:
This is not thread safe. Between calls to `find` and `end` the map
currently pointed to by `m_dataSerializableMap` can change.
```C++
auto dataSerializableMap = std::atomic_load(m_dataSerializableMap);
const auto& found = dataSerializableMap->find(id);
if (found != dataSerializableMap->end()) {
...
```
There are more of these time of access behaviors in this file.
##########
File path: cppcache/src/SerializationRegistry.cpp
##########
@@ -409,44 +411,56 @@ void TheTypeMap::bindDataSerializable(TypeFactoryMethod
func, int32_t id) {
"TheTypeMap::bind: Serialization type not implemented.");
}
- std::lock_guard<util::concurrent::spinlock_mutex> guard(
- m_dataSerializableMapLock);
- const auto& result = m_dataSerializableMap.emplace(id, func);
+ auto tempDataSerializableMap =
+ std::make_shared<std::unordered_map<int32_t, TypeFactoryMethod>>(
+ *m_dataSerializableMap);
+ const auto& result = tempDataSerializableMap->emplace(id, func);
if (!result.second) {
LOGERROR("A class with ID %d is already registered.", id);
throw IllegalStateException("A class with given ID is already
registered.");
}
+ std::atomic_store(&m_dataSerializableMap, tempDataSerializableMap);
Review comment:
There is a race condition here that we need to consider. If two threads
attempt to update this map they will believe to have successfully done so but
only one will truly win. For example, type 1 and 2 are added concurrently, the
map with either contain type 1 or type 2 but not both. The thread that tried to
add the type that didn't get into the map will not error.
This type of copy on write map behavior needs a compare and swap storage.
You fetch the current map pointer, clone and add your contents to the map, then
if and only if the current map pointer matches the one you fetched previously
to you replace it with your new map, otherwise you try again.
```C++
while(true) {
const auto currentDataSerializableMap = std::atomic_load(m_
tempDataSerializableMap);
auto newDataSerializableMap = clone(currentDataSerializableMap);
const auto& result = newDataSerializableMap->emplace(id, func);
if (!std:: compare_exchange_strong(&m_dataSerializableMap,
currentDataSerializableMap, newDataSerializableMap)) continue;
if (!result.second) {
...
}
break;
}
```
Similar goes for any removal or updating operations.
----------------------------------------------------------------
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:
us...@infra.apache.org
> C++ native client performance bottleneck in access to serialization registry
> ----------------------------------------------------------------------------
>
> Key: GEODE-8213
> URL: https://issues.apache.org/jira/browse/GEODE-8213
> Project: Geode
> Issue Type: Improvement
> Components: native client
> Reporter: Alberto Gomez
> Assignee: Alberto Gomez
> Priority: Major
> Fix For: 1.14.0
>
>
> It's been observed that when the number of threads used in a Geode client
> using PdxSerialization is greater than 8, there is an important drop in
> performance.
> Analysing the client process behavior with perf, it has been observed a very
> high CPU consumption by a spinlock
> (apache::geode::util::concurrent::spinlock_mutex::lock) used when accessing
> the serialization registry .
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
