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 2020/06/23 08:10:05 UTC
[GitHub] [geode-native] albertogpz opened a new pull request #621: GEODE-8213: Switch SerializationRegistry spinlock to mutex
albertogpz opened a new pull request #621:
URL: https://github.com/apache/geode-native/pull/621
----------------------------------------------------------------
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 #621: GEODE-8213: Switch SerializationRegistry spinlock to mutex
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #621:
URL: https://github.com/apache/geode-native/pull/621#discussion_r445106638
##########
File path: cppcache/src/SerializationRegistry.hpp
##########
@@ -79,10 +80,10 @@ class TheTypeMap {
std::unordered_map<internal::DSFid, TypeFactoryMethod>
m_dataSerializableFixedIdMap;
std::unordered_map<std::string, TypeFactoryMethodPdx> m_pdxSerializableMap;
- mutable util::concurrent::spinlock_mutex m_dataSerializablePrimitiveMapLock;
- mutable util::concurrent::spinlock_mutex m_dataSerializableMapLock;
- mutable util::concurrent::spinlock_mutex m_dataSerializableFixedIdMapLock;
- mutable util::concurrent::spinlock_mutex m_pdxSerializableMapLock;
+ mutable std::mutex m_dataSerializablePrimitiveMapLock;
Review comment:
Oh I see you made them anyway. Cool!
----------------------------------------------------------------
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] mkevo merged pull request #621: GEODE-8213: Switch SerializationRegistry spinlock to mutex
Posted by GitBox <gi...@apache.org>.
mkevo merged pull request #621:
URL: https://github.com/apache/geode-native/pull/621
----------------------------------------------------------------
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 #621: GEODE-8213: Switch SerializationRegistry spinlock to mutex
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #621:
URL: https://github.com/apache/geode-native/pull/621#discussion_r445104329
##########
File path: cppcache/src/SerializationRegistry.hpp
##########
@@ -79,10 +80,10 @@ class TheTypeMap {
std::unordered_map<internal::DSFid, TypeFactoryMethod>
m_dataSerializableFixedIdMap;
std::unordered_map<std::string, TypeFactoryMethodPdx> m_pdxSerializableMap;
- mutable util::concurrent::spinlock_mutex m_dataSerializablePrimitiveMapLock;
- mutable util::concurrent::spinlock_mutex m_dataSerializableMapLock;
- mutable util::concurrent::spinlock_mutex m_dataSerializableFixedIdMapLock;
- mutable util::concurrent::spinlock_mutex m_pdxSerializableMapLock;
+ mutable std::mutex m_dataSerializablePrimitiveMapLock;
Review comment:
Haha. It looks like this guide changed after we adopted it. Camel case was acceptable at the time. We should probably address this. So I guess do worry about the variables right now. Thanks for spotting 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] albertogpz commented on a change in pull request #621: GEODE-8213: Switch SerializationRegistry spinlock to mutex
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #621:
URL: https://github.com/apache/geode-native/pull/621#discussion_r444704517
##########
File path: cppcache/src/SerializationRegistry.hpp
##########
@@ -79,10 +80,10 @@ class TheTypeMap {
std::unordered_map<internal::DSFid, TypeFactoryMethod>
m_dataSerializableFixedIdMap;
std::unordered_map<std::string, TypeFactoryMethodPdx> m_pdxSerializableMap;
- mutable util::concurrent::spinlock_mutex m_dataSerializablePrimitiveMapLock;
- mutable util::concurrent::spinlock_mutex m_dataSerializableMapLock;
- mutable util::concurrent::spinlock_mutex m_dataSerializableFixedIdMapLock;
- mutable util::concurrent::spinlock_mutex m_pdxSerializableMapLock;
+ mutable std::mutex m_dataSerializablePrimitiveMapLock;
Review comment:
I made the changes according to your proposal.
Nevertheles according to the CONTRIBUTING.md file we should be following Google's C++ style guide. According to it, the member variables should be named like ```data_serializable_map_mutex_```.
Is there any other style guide I am not aware of?
----------------------------------------------------------------
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] albertogpz edited a comment on pull request #621: GEODE-8213: Switch SerializationRegistry spinlock to mutex
Posted by GitBox <gi...@apache.org>.
albertogpz edited a comment on pull request #621:
URL: https://github.com/apache/geode-native/pull/621#issuecomment-647987941
The shared_lock provided by boost offered a much worse performance than the spinlock or the mutex (see https://github.com/apache/geode-native/pull/611).
There is a tiny gain according to the benchmark performance test in most cases using a mutex:
```
Benchmark Time CPU Time Old Time New CPU Old CPU New
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:1 -0.0997 -0.0998 193 174 193 174
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:2 -0.1107 -0.1107 100 89 199 177
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:4 -0.0666 -0.0667 51 48 206 192
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:8 -0.0151 -0.0160 32 32 259 255
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:16 +0.0258 +0.0430 24 25 300 313
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:32 +0.0843 +0.0349 22 24 309 320
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:64 +0.0283 +0.0359 22 23 312 323
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:96 -0.1669 +0.0336 25 21 315 326
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:1 -0.0925 -0.0925 190 173 190 173
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:2 -0.0528 -0.0529 98 93 197 186
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:4 -0.0803 -0.0803 53 49 213 195
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:8 -0.0801 -0.0802 35 32 279 256
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:16 -0.0189 -0.0052 26 25 318 316
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:32 -0.0000 +0.0145 25 25 325 330
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:64 -0.0363 +0.0179 24 24 327 332
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:96 -0.0465 +0.0151 24 23 328 333
SerializationRegistryBM_findDataSerializable/real_time/threads:1 -0.0557 -0.0557 193 183 193 183
SerializationRegistryBM_findDataSerializable/real_time/threads:2 -0.0190 -0.0190 98 97 197 193
SerializationRegistryBM_findDataSerializable/real_time/threads:4 -0.0716 -0.0722 56 52 222 206
SerializationRegistryBM_findDataSerializable/real_time/threads:8 -0.1733 -0.1726 37 30 295 244
SerializationRegistryBM_findDataSerializable/real_time/threads:16 -0.0343 -0.0065 26 25 322 319
SerializationRegistryBM_findDataSerializable/real_time/threads:32 +0.0158 +0.0069 25 25 327 329
SerializationRegistryBM_findDataSerializable/real_time/threads:64 -0.0160 +0.0143 25 25 326 331
SerializationRegistryBM_findDataSerializable/real_time/threads:96 +0.1475 +0.0089 22 25 329 332
SerializationRegistryBM_findPdxSerializable/real_time/threads:1 -0.0616 -0.0616 198 186 198 186
SerializationRegistryBM_findPdxSerializable/real_time/threads:2 -0.0794 -0.0795 101 93 202 186
SerializationRegistryBM_findPdxSerializable/real_time/threads:4 -0.0946 -0.0964 57 52 228 206
SerializationRegistryBM_findPdxSerializable/real_time/threads:8 -0.0001 -0.0016 38 38 303 302
SerializationRegistryBM_findPdxSerializable/real_time/threads:16 -0.0356 -0.0215 30 29 375 367
SerializationRegistryBM_findPdxSerializable/real_time/threads:32 -0.0635 -0.0182 29 27 383 376
SerializationRegistryBM_findPdxSerializable/real_time/threads:64 +0.0668 -0.0076 27 28 384 381
SerializationRegistryBM_findPdxSerializable/real_time/threads:96 +0.0098 -0.0051 28 28 386 384
```
----------------------------------------------------------------
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 #621: GEODE-8213: Switch SerializationRegistry spinlock to mutex
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #621:
URL: https://github.com/apache/geode-native/pull/621#discussion_r444346251
##########
File path: cppcache/src/SerializationRegistry.hpp
##########
@@ -23,6 +23,7 @@
#include <functional>
#include <iostream>
#include <memory>
+#include <mutex>
Review comment:
You should be able to remove `#include "util/concurrent/spinlock_mutex.hpp"`
##########
File path: cppcache/src/SerializationRegistry.hpp
##########
@@ -79,10 +80,10 @@ class TheTypeMap {
std::unordered_map<internal::DSFid, TypeFactoryMethod>
m_dataSerializableFixedIdMap;
std::unordered_map<std::string, TypeFactoryMethodPdx> m_pdxSerializableMap;
- mutable util::concurrent::spinlock_mutex m_dataSerializablePrimitiveMapLock;
- mutable util::concurrent::spinlock_mutex m_dataSerializableMapLock;
- mutable util::concurrent::spinlock_mutex m_dataSerializableFixedIdMapLock;
- mutable util::concurrent::spinlock_mutex m_pdxSerializableMapLock;
+ mutable std::mutex m_dataSerializablePrimitiveMapLock;
Review comment:
Given that you are touching 90% of the member variables in this class perhaps now is a good time to make them style guide compliant and more correctly named, like `dataSerializableMapMutex_`.
----------------------------------------------------------------
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] albertogpz commented on pull request #621: GEODE-8213: Switch SerializationRegistry spinlock to mutex
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #621:
URL: https://github.com/apache/geode-native/pull/621#issuecomment-647987941
The shared_lock provided by boost offered a much worse performance than the spinlock or the mutex.
There is a tiny gain according to the benchmark performance test in most cases using a mutex:
```
Benchmark Time CPU Time Old Time New CPU Old CPU New
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:1 -0.0997 -0.0998 193 174 193 174
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:2 -0.1107 -0.1107 100 89 199 177
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:4 -0.0666 -0.0667 51 48 206 192
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:8 -0.0151 -0.0160 32 32 259 255
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:16 +0.0258 +0.0430 24 25 300 313
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:32 +0.0843 +0.0349 22 24 309 320
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:64 +0.0283 +0.0359 22 23 312 323
SerializationRegistryBM_findDataSerializablePrimitive/real_time/threads:96 -0.1669 +0.0336 25 21 315 326
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:1 -0.0925 -0.0925 190 173 190 173
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:2 -0.0528 -0.0529 98 93 197 186
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:4 -0.0803 -0.0803 53 49 213 195
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:8 -0.0801 -0.0802 35 32 279 256
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:16 -0.0189 -0.0052 26 25 318 316
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:32 -0.0000 +0.0145 25 25 325 330
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:64 -0.0363 +0.0179 24 24 327 332
SerializationRegistryBM_findDataSerializableFixedId/real_time/threads:96 -0.0465 +0.0151 24 23 328 333
SerializationRegistryBM_findDataSerializable/real_time/threads:1 -0.0557 -0.0557 193 183 193 183
SerializationRegistryBM_findDataSerializable/real_time/threads:2 -0.0190 -0.0190 98 97 197 193
SerializationRegistryBM_findDataSerializable/real_time/threads:4 -0.0716 -0.0722 56 52 222 206
SerializationRegistryBM_findDataSerializable/real_time/threads:8 -0.1733 -0.1726 37 30 295 244
SerializationRegistryBM_findDataSerializable/real_time/threads:16 -0.0343 -0.0065 26 25 322 319
SerializationRegistryBM_findDataSerializable/real_time/threads:32 +0.0158 +0.0069 25 25 327 329
SerializationRegistryBM_findDataSerializable/real_time/threads:64 -0.0160 +0.0143 25 25 326 331
SerializationRegistryBM_findDataSerializable/real_time/threads:96 +0.1475 +0.0089 22 25 329 332
SerializationRegistryBM_findPdxSerializable/real_time/threads:1 -0.0616 -0.0616 198 186 198 186
SerializationRegistryBM_findPdxSerializable/real_time/threads:2 -0.0794 -0.0795 101 93 202 186
SerializationRegistryBM_findPdxSerializable/real_time/threads:4 -0.0946 -0.0964 57 52 228 206
SerializationRegistryBM_findPdxSerializable/real_time/threads:8 -0.0001 -0.0016 38 38 303 302
SerializationRegistryBM_findPdxSerializable/real_time/threads:16 -0.0356 -0.0215 30 29 375 367
SerializationRegistryBM_findPdxSerializable/real_time/threads:32 -0.0635 -0.0182 29 27 383 376
SerializationRegistryBM_findPdxSerializable/real_time/threads:64 +0.0668 -0.0076 27 28 384 381
SerializationRegistryBM_findPdxSerializable/real_time/threads:96 +0.0098 -0.0051 28 28 386 384
```
----------------------------------------------------------------
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