ADPPRG-126821: Reduce contention in access to pdx registry #6
Conversation
- Whenever execution server functions with isHA=false, hasResult=true and optimizeForWrite=true, if the metadata is incomplete a request of type EXECUTE_REGION_FUNCTION_SINGLE_HOP is sent with an invalid bucket set partition. This causes an InternalFunctionInvocationTargetException exception - In order to solve this issue, now, whenever calling groupByServerToBuckets, if there isn't a valid location for one of the buckets, it returns nullptr, triggering the fallback mechanism which would be sending EXECUTE_REGION_FUNCTION to one of the nodes instead.
- Moved PUT actions from LocalRegion into its own modules. - Added PutIfAbsent action. - Added the necessary scaffolding for put requests to return a value. - Moved TcrMessagePut implementation into its own file. Now this message type accepts an operation as input. - Moved TcrMessageDestroy implementation into its own file. Now this message accepts an operation as input. - Also, updated TcrMessageDestroy operation format so it uses a byte part instead of an object. - Added putIfAbsent method to the Region interface and its implementations both for LocalRegion and ThinClientRegion. - Modified LocalRegion and ThinClientRegion to comply with the above code changes. - Modified PUT's old value parsing to handle null pointer. - Also replaced Cacheable type by Serializable in some places in order to take advantage of fast-forward definition.
| const auto& found = dataSerializableFixedIdMap_.find(dsfid); | ||
| if (found != dataSerializableFixedIdMap_.end()) { | ||
| func = found->second; | ||
| switch (dsfid) { |
There was a problem hiding this comment.
This switch case should be in SerializationRegistry::deserializeDataSerializableFixedId and in default we should call to TheTypeMap::findDataSerializableFixedId. IMHO, TheTypeMap should be strictly focused for user-defined types, not internally library pre-defined types
There was a problem hiding this comment.
Good point. I agree.
| func = found->second; | ||
| // Geode builtins will be inside the switch value cases. | ||
| // The type ids should be in DSCode.hpp | ||
| switch (dsCode) { |
There was a problem hiding this comment.
You don't need to modify this function, since the only calling function, SerializationRegistry::serialize, is already handling library pre-defined types, and there is no other call point. Hence, this function will be only called if the type we are looking for is an user-defined type
There was a problem hiding this comment.
Good catch. Thanks for pointing it out.
|
|
||
| theTypeMap_.findDataSerializablePrimitive(dsCode, createType); | ||
| if (createType == nullptr) { | ||
| theTypeMap_.findDataSerializablePrimitive(dsCode, createType); |
There was a problem hiding this comment.
It would be better if this is handled in the default case of above's switch, making it more clear this handles user-defined types and avoiding the need to make the null comparison
Access to the Geode predefined types in the serialization registry has been changed from a mutex synchronized map to a hardwired switch-case in order to avoid contention when several threads access them.
Final commit must be modified as follows: Access to the Geode predefined types in the serialization registry has been changed from a mutex synchronized map to a hardwired switch-case in order to avoid contention when several threads access them.
albertogpz
force-pushed
the
wip/ADPPRG-126821
branch
from
91b8f01
to
17fec6c
Compare
| if (found != dataSerializableFixedIdMap_.end()) { | ||
| func = found->second; | ||
| } | ||
| const std::lock_guard<std::mutex> guard(dataSerializableFixedIdMapMutex_); |
There was a problem hiding this comment.
Clang formatting rules establish that 2 spaces should be used for indentation, I think you are using 4 here.
| if (found != dataSerializablePrimitiveMap_.end()) { | ||
| func = found->second; | ||
| } | ||
| const std::lock_guard<std::mutex> guard(dataSerializablePrimitiveMapMutex_); |
There was a problem hiding this comment.
Clang formatting rules establish that 2 spaces should be used for indentation, I think you are using 4 here.
gaussianrecurrence
force-pushed
the
release-nordix/1.15.1
branch
2 times, most recently
from
579afe9
to
fb1a836
Compare
|
@albertogpz: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Access to the Geode predefined types in the
serialization registry has been changed from a
mutex synchronized map to a hardwired switch-case
in order to avoid contention when several threads
access them.