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 2021/07/29 20:03:41 UTC
[GitHub] [geode] dschneider-pivotal opened a new pull request #6727: optimize sizing of RedisHash, RedisSet, and RedisSortedSet
dschneider-pivotal opened a new pull request #6727:
URL: https://github.com/apache/geode/pull/6727
Currently this is just an experiment on a minor optimization to how the redis data structures compute their size. If this approach is approved then we should probably get rid of the places in the redis code that creates a static SizeableObjectSizer. These classes could instead be abstract forcing the subclass to implement sizeKey, sizeValue, or sizeElement.
Note that RedisSortedSet no longer does extra sizing that it the undoes with the "sizeInBytesAdjustment". Instead it just has a ScoreSet implementation that does not size the element at all since it is already sized by the MemberMap.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6727: GEODE-9493: rework sizing of RedisHash, RedisSet, and RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6727:
URL: https://github.com/apache/geode/pull/6727#discussion_r686341804
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursor.java
##########
@@ -196,46 +194,60 @@ public V put(K k, V v) {
V oldValue = super.put(k, v);
if (oldValue == null) {
// A create
- arrayContentsOverhead += elementSizer.sizeof(k) + elementSizer.sizeof(v);
+ arrayContentsOverhead += sizeKey(k) + sizeValue(v);
} else {
// An update
- arrayContentsOverhead += elementSizer.sizeof(v) - elementSizer.sizeof(oldValue);
+ arrayContentsOverhead += sizeValue(v) - sizeValue(oldValue);
}
return oldValue;
}
@Override
+ @SuppressWarnings("unchecked")
public V remove(Object k) {
V oldValue = super.remove(k);
if (oldValue != null) {
- arrayContentsOverhead -= elementSizer.sizeof(k) + elementSizer.sizeof(oldValue);
+ arrayContentsOverhead -= sizeKey((K) k) + sizeValue(oldValue);
}
return oldValue;
}
@Override
public int getSizeInBytes() {
- return arrayContentsOverhead + calculateBackingArraysOverhead();
+ // The size of the object referenced by the "strategy" field is not included
+ // here because in most cases it is a static singleton.
+ // TODO: The size of the object referenced by these fields is not included here:
+ // - entries an instance of
+ // it.unimi.dsi.fastutil.objects.Object2ObjectOpenCustomHashMap.MapEntrySet
+ // - keys an instance of it.unimi.dsi.fastutil.objects.Object2ObjectOpenCustomHashMap.KeySet
+ // - values an instance of an anonymous subclass of
+ // it.unimi.dsi.fastutil.objects.AbstractObjectCollection
+ //
+ // These fields get lazily initialized when their corresponding method is called.
+ // Once they are initialized they stay that way forever. The object they reference is immutable.
+ // Its only field is the implicit objref to their outer class instance.
+ // Currently it looks like we have redis ops that can end up calling all three of these methods
+ // in which case these objects would account for over 100 bytes on a 64-bit jvm,
+ // It is not clear to me why the implementors of Object2ObjectOpenCustomHashMap chose to
+ // cache this immutable object instead of just creating it every time. They have no state
+ // to initialize when constructed so one thing we could do is override the methods that cache it
+ // to either null out the cached field after calling the super method, or have a brand new impl
+ // that just creates and returns an instance without caching. The cache fields themselves would
+ // be a waste of memory in that case but only 24 bytes (12 with compressed oops).
+ // Another possibility would be for our code that uses the map to only call entrySet(), that way
+ // one instance would be cached instead of 3. You can always get keys and values from the
+ // entrySet.
Review comment:
Could you please create a Jira ticket for this and reference it in the comment. This level of detail feels a bit much for a comment
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
##########
@@ -656,7 +656,9 @@ public void redisSortedSetGetSizeInBytes_isAccurateForZpopmax() {
for (int i = 0; i < numberOfEntries; ++i) {
sortedSet.zpopmax(mockRegion, mockKey, 1);
sortedSet.clearDelta();
- assertThat(sizer.sizeof(sortedSet)).isEqualTo(sortedSet.getSizeInBytes());
+ int actualSize = sizer.sizeof(sortedSet) - sizer.sizeof(ByteArrays.HASH_STRATEGY);
+ int calculatedSize = sortedSet.getSizeInBytes();
+ assertThat(calculatedSize).isEqualTo(actualSize);
Review comment:
the use of "actual" here is a bit confusing, since in general we do assertThat(actual).isEqualTo(expected). In this case the expected is the "actual" size and actual is "calculated". I think it would be less confusing to use a variable name like "measuredSize". (for this and the above tests)
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #6727: GEODE-9493: rework sizing of RedisString, RedisHash, RedisSet, and RedisSortedSet
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal merged pull request #6727:
URL: https://github.com/apache/geode/pull/6727
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6727: optimize sizing of RedisHash, RedisSet, and RedisSortedSet
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6727:
URL: https://github.com/apache/geode/pull/6727#issuecomment-890262560
This pull request **introduces 1 alert** when merging d8235fe1fd03d77af8b7d992b22f549d82eeeace into 138d5b8969a70bde07ad729461f1d914aa0b14f8 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-5983bb8718ffb0adfddfa7bcdee8bc316ee07c4d)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #6727: GEODE-9493: rework sizing of RedisString, RedisHash, RedisSet, and RedisSortedSet
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on pull request #6727:
URL: https://github.com/apache/geode/pull/6727#issuecomment-899626810
The acceptance test failure is the known issue: https://issues.apache.org/jira/browse/GEODE-9319
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6727: GEODE-9493: rework sizing of RedisHash, RedisSet, and RedisSortedSet
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6727:
URL: https://github.com/apache/geode/pull/6727#discussion_r686389685
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursor.java
##########
@@ -196,46 +194,60 @@ public V put(K k, V v) {
V oldValue = super.put(k, v);
if (oldValue == null) {
// A create
- arrayContentsOverhead += elementSizer.sizeof(k) + elementSizer.sizeof(v);
+ arrayContentsOverhead += sizeKey(k) + sizeValue(v);
} else {
// An update
- arrayContentsOverhead += elementSizer.sizeof(v) - elementSizer.sizeof(oldValue);
+ arrayContentsOverhead += sizeValue(v) - sizeValue(oldValue);
}
return oldValue;
}
@Override
+ @SuppressWarnings("unchecked")
public V remove(Object k) {
V oldValue = super.remove(k);
if (oldValue != null) {
- arrayContentsOverhead -= elementSizer.sizeof(k) + elementSizer.sizeof(oldValue);
+ arrayContentsOverhead -= sizeKey((K) k) + sizeValue(oldValue);
}
return oldValue;
}
@Override
public int getSizeInBytes() {
- return arrayContentsOverhead + calculateBackingArraysOverhead();
+ // The size of the object referenced by the "strategy" field is not included
+ // here because in most cases it is a static singleton.
+ // TODO: The size of the object referenced by these fields is not included here:
+ // - entries an instance of
+ // it.unimi.dsi.fastutil.objects.Object2ObjectOpenCustomHashMap.MapEntrySet
+ // - keys an instance of it.unimi.dsi.fastutil.objects.Object2ObjectOpenCustomHashMap.KeySet
+ // - values an instance of an anonymous subclass of
+ // it.unimi.dsi.fastutil.objects.AbstractObjectCollection
+ //
+ // These fields get lazily initialized when their corresponding method is called.
+ // Once they are initialized they stay that way forever. The object they reference is immutable.
+ // Its only field is the implicit objref to their outer class instance.
+ // Currently it looks like we have redis ops that can end up calling all three of these methods
+ // in which case these objects would account for over 100 bytes on a 64-bit jvm,
+ // It is not clear to me why the implementors of Object2ObjectOpenCustomHashMap chose to
+ // cache this immutable object instead of just creating it every time. They have no state
+ // to initialize when constructed so one thing we could do is override the methods that cache it
+ // to either null out the cached field after calling the super method, or have a brand new impl
+ // that just creates and returns an instance without caching. The cache fields themselves would
+ // be a waste of memory in that case but only 24 bytes (12 with compressed oops).
+ // Another possibility would be for our code that uses the map to only call entrySet(), that way
+ // one instance would be cached instead of 3. You can always get keys and values from the
+ // entrySet.
Review comment:
thanks it is GEODE-9497
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6727: GEODE-9493: rework sizing of RedisHash, RedisSet, and RedisSortedSet
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6727:
URL: https://github.com/apache/geode/pull/6727#discussion_r686389856
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
##########
@@ -656,7 +656,9 @@ public void redisSortedSetGetSizeInBytes_isAccurateForZpopmax() {
for (int i = 0; i < numberOfEntries; ++i) {
sortedSet.zpopmax(mockRegion, mockKey, 1);
sortedSet.clearDelta();
- assertThat(sizer.sizeof(sortedSet)).isEqualTo(sortedSet.getSizeInBytes());
+ int actualSize = sizer.sizeof(sortedSet) - sizer.sizeof(ByteArrays.HASH_STRATEGY);
+ int calculatedSize = sortedSet.getSizeInBytes();
+ assertThat(calculatedSize).isEqualTo(actualSize);
Review comment:
I changed them to expectedSize and actualSize
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org