dschneider-pivotal commented on a change in pull request #6590:
URL: https://github.com/apache/geode/pull/6590#discussion_r648533960
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursor.java
##########
@@ -29,52 +33,58 @@
* The scan method provides the same guarantees as Redis's HSCAN, and in fact
* uses the same algorithm.
*/
-public class Object2ObjectOpenCustomHashMapWithCursor<K, V>
- extends Object2ObjectOpenCustomHashMap<K, V> {
+public class SizeableObject2ObjectOpenCustomHashMapWithCursor<K, V>
Review comment:
since our redis data classes always use the class with K=byte[] and
V=byte[] it seems like you could make this class be
SizeableByteArray2ByteArrayOpenCustomHashMapWithCursor extends
Object2ObjectOpenCustomHashMap<byte[], byte[]>
You could also do this for the HashSet class.
Then in the size code instead of using the ReflectionSingleObjectSizer you
can just compute the size of a byte[] which is very simple.
ReflectionSingleObjectSizer has to do extra work to handle any type of array.
It seems like this would give you fast sizing.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursor.java
##########
@@ -29,52 +33,58 @@
* The scan method provides the same guarantees as Redis's HSCAN, and in fact
* uses the same algorithm.
*/
-public class Object2ObjectOpenCustomHashMapWithCursor<K, V>
- extends Object2ObjectOpenCustomHashMap<K, V> {
+public class SizeableObject2ObjectOpenCustomHashMapWithCursor<K, V>
+ extends Object2ObjectOpenCustomHashMap<K, V> implements Sizeable {
+
+ private static final long serialVersionUID = 9079713776660851891L;
+ public static final int BACKING_ARRAY_OVERHEAD_CONSTANT = 136;
+ public static final int BACKING_ARRAY_LENGTH_COEFFICIENT = 4;
+ private static final ReflectionSingleObjectSizer elementSizer =
+ ReflectionSingleObjectSizer.getInstance();
+ private int keysOverhead;
+ private int valuesOverhead;
Review comment:
I'm okay with having two overhead variables but I don't see any need for
two. It looks like you could just have one "sizeInBytes" that tracks both keys
and values. If you initialized it to BACKING_ARRAY_OVERHEAD_CONSTANT then the
getSizeInBytes could just be "return sizeInBytes;"
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursor.java
##########
@@ -29,52 +33,58 @@
* The scan method provides the same guarantees as Redis's HSCAN, and in fact
* uses the same algorithm.
*/
-public class Object2ObjectOpenCustomHashMapWithCursor<K, V>
- extends Object2ObjectOpenCustomHashMap<K, V> {
+public class SizeableObject2ObjectOpenCustomHashMapWithCursor<K, V>
+ extends Object2ObjectOpenCustomHashMap<K, V> implements Sizeable {
+
+ private static final long serialVersionUID = 9079713776660851891L;
+ public static final int BACKING_ARRAY_OVERHEAD_CONSTANT = 136;
+ public static final int BACKING_ARRAY_LENGTH_COEFFICIENT = 4;
+ private static final ReflectionSingleObjectSizer elementSizer =
+ ReflectionSingleObjectSizer.getInstance();
+ private int keysOverhead;
Review comment:
Currently these "Overhead" ints only include the size of the byte
arrays. But if every time you changed the Overhead for a byte array you also
included the BACKING_ARRAY_LENGTH_COEFFICIENT (by incing it or decing it) then
you would not need to consult the length the arrays later when you compute the
total size. Seems like it would make it a little faster.
--
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:
[email protected]
