[
https://issues.apache.org/jira/browse/GEODE-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17138769#comment-17138769
]
ASF GitHub Bot commented on GEODE-8212:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441799345
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
Review comment:
Just a thought here. Is it necessary to hash like Java client does in
this case? If the `PdxType::hashcode()` is expected to hash similarly to that
of Java version then changes are needed here. If not then I wouldn't try to
implement the `CacheableKey::hashcode` method here and do something more
compatible with C++, like `std::hash` which produces a `size_t`. In fact I
would just create a specialization of `std::hash<PdxType>`.
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first,
std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
Review comment:
Equality of hashcode does not guarantee equality of the object. You
should just implement `PdxType::operator==`.
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first,
std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
+ }
+};
+
+typedef std::unordered_map<std::shared_ptr<PdxType>, int32_t, PdxTypeHashCode,
PdxTypeEqualCmp>
Review comment:
If you implement `std::hash<PdxType>` and `PdxType::operator==` then you
can use the `dereference_hash<std::shared_ptr<PdxType>>` and
`dereference_equal_to<std::shared_ptr< PdxType >>>` here.
----------------------------------------------------------------
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
> Change in PdxType class "<" operator impacts performance
> --------------------------------------------------------
>
> Key: GEODE-8212
> URL: https://issues.apache.org/jira/browse/GEODE-8212
> Project: Geode
> Issue Type: Improvement
> Components: native client
> Reporter: Alberto Bustamante Reyes
> Assignee: Alberto Bustamante Reyes
> Priority: Major
> Attachments: performance_after_GEODE-7694.patch
>
>
> While investigating a performance problem of the C++ native client, it has
> been observed that change in the PdxType class "<" operator introduced by
> GEODE-7694 impacts performance of PdxHelper::serializePdx().
> Im attaching a patch with some code and a test case I have used to benchmark
> that method. With the current implementation in develop branch, creating just
> 10 instances is enough to see the difference. The calls to serializePdx take
> these times:
> {noformat}
> Elapsed Time (serializePdx): 32.1322 milliseconds
> Elapsed Time (serializePdx): 3.06044 milliseconds
> Elapsed Time (serializePdx): 1.58791 milliseconds
> Elapsed Time (serializePdx): 1.62466 milliseconds
> Elapsed Time (serializePdx): 1.53676 milliseconds
> Elapsed Time (serializePdx): 1.59278 milliseconds
> Elapsed Time (serializePdx): 1.82878 milliseconds
> Elapsed Time (serializePdx): 1.36811 milliseconds
> Elapsed Time (serializePdx): 1.6956 milliseconds
> Elapsed Time (serializePdx): 1.46873 milliseconds
> Elapsed Time (serializePdx): 1.53206 milliseconds
> {noformat}
> While these are the times using the old "<" operator:
> {noformat}
> Elapsed Time (serializePdx): 56.1524 milliseconds
> Elapsed Time (serializePdx): 0.012327 milliseconds
> Elapsed Time (serializePdx): 0.006325 milliseconds
> Elapsed Time (serializePdx): 0.005419 milliseconds
> Elapsed Time (serializePdx): 0.005266 milliseconds
> Elapsed Time (serializePdx): 0.005662 milliseconds
> Elapsed Time (serializePdx): 0.005407 milliseconds
> Elapsed Time (serializePdx): 0.005286 milliseconds
> Elapsed Time (serializePdx): 0.005467 milliseconds
> Elapsed Time (serializePdx): 0.005276 milliseconds
> Elapsed Time (serializePdx): 0.005298 milliseconds
> {noformat}
> And after creating 50.000 instances, these are the times for the last 10
> calls with the current "<" operator and with the old one respectively::
> {noformat}
> Elapsed Time (serializePdx): 8.11767 milliseconds
> Elapsed Time (serializePdx): 8.15727 milliseconds
> Elapsed Time (serializePdx): 8.09303 milliseconds
> Elapsed Time (serializePdx): 7.89909 milliseconds
> Elapsed Time (serializePdx): 8.37956 milliseconds
> Elapsed Time (serializePdx): 8.21303 milliseconds
> Elapsed Time (serializePdx): 8.14189 milliseconds
> Elapsed Time (serializePdx): 8.21266 milliseconds
> Elapsed Time (serializePdx): 8.21683 milliseconds
> Elapsed Time (serializePdx): 7.78765 milliseconds
> {noformat}
> {noformat}
> Elapsed Time (serializePdx): 0.003014 milliseconds
> Elapsed Time (serializePdx): 0.003 milliseconds
> Elapsed Time (serializePdx): 0.003084 milliseconds
> Elapsed Time (serializePdx): 0.003112 milliseconds
> Elapsed Time (serializePdx): 0.00305 milliseconds
> Elapsed Time (serializePdx): 0.003018 milliseconds
> Elapsed Time (serializePdx): 0.003029 milliseconds
> Elapsed Time (serializePdx): 0.003061 milliseconds
> Elapsed Time (serializePdx): 0.003043 milliseconds
> Elapsed Time (serializePdx): 0.003086 milliseconds
> {noformat}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
