DonalEvans commented on a change in pull request #6590:
URL: https://github.com/apache/geode/pull/6590#discussion_r650203570
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObjectOpenCustomHashSetTest.java
##########
@@ -99,13 +50,15 @@ public void backingArrayOverheadCalculationTest() {
// Calculate the overhead associated only with the backing array
backingArrayOverhead = sizer.sizeof(set) - set.getMemberOverhead();
+ int expected = BACKING_ARRAY_OVERHEAD_CONSTANT
+ + BACKING_ARRAY_LENGTH_COEFFICIENT * set.getBackingArrayLength();
softly.assertThat(backingArrayOverhead)
Review comment:
May I ask why? For this test in particular, the use of soft assertions
is helpful in determining in one run what's going wrong with the sizing, as if
we just failed on the first assertion, it wouldn't be clear if the size was off
by a constant amount or if there was a compounding error in the calculation.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/collections/SizeableObjectOpenCustomHashSet.java
##########
@@ -14,23 +14,23 @@
*/
package org.apache.geode.redis.internal.collections;
-import static
org.apache.geode.internal.size.ReflectionSingleObjectSizer.roundUpSize;
-
import java.util.Collection;
import java.util.Iterator;
import it.unimi.dsi.fastutil.objects.ObjectCollection;
import it.unimi.dsi.fastutil.objects.ObjectOpenCustomHashSet;
import org.apache.geode.annotations.VisibleForTesting;
+import org.apache.geode.internal.size.ReflectionSingleObjectSizer;
import org.apache.geode.internal.size.Sizeable;
public class SizeableObjectOpenCustomHashSet<K> extends
ObjectOpenCustomHashSet<K>
implements Sizeable {
private static final long serialVersionUID = 9174920505089089517L;
- public static final int MEMBER_OVERHEAD_CONSTANT = 16;
public static final int BACKING_ARRAY_OVERHEAD_CONSTANT = 92;
public static final int BACKING_ARRAY_LENGTH_COEFFICIENT = 4;
+ private static final ReflectionSingleObjectSizer elementSizer =
+ ReflectionSingleObjectSizer.getInstance();
Review comment:
Done!
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursorTest.java
##########
@@ -0,0 +1,349 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.collections;
+
+import static
org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_LENGTH_COEFFICIENT;
+import static
org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_OVERHEAD_CONSTANT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.stream.IntStream;
+
+import it.unimi.dsi.fastutil.Hash;
+import it.unimi.dsi.fastutil.bytes.ByteArrays;
+import org.assertj.core.api.SoftAssertions;
+import org.junit.Test;
+
+import org.apache.geode.internal.size.ReflectionObjectSizer;
+
+public class SizeableObject2ObjectOpenCustomHashMapWithCursorTest {
+ private final ReflectionObjectSizer sizer =
ReflectionObjectSizer.getInstance();
Review comment:
The `ReflectionObjectSizer` is a pre-existing singleton class that
doesn't allow instantiation. Removing/refactoring that is outside the scope of
this PR, so I'll leave this as is (but change the type to the `ObjectSizer`
interface).
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursorTest.java
##########
@@ -0,0 +1,349 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.collections;
+
+import static
org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_LENGTH_COEFFICIENT;
+import static
org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_OVERHEAD_CONSTANT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.stream.IntStream;
+
+import it.unimi.dsi.fastutil.Hash;
+import it.unimi.dsi.fastutil.bytes.ByteArrays;
+import org.assertj.core.api.SoftAssertions;
+import org.junit.Test;
+
+import org.apache.geode.internal.size.ReflectionObjectSizer;
+
+public class SizeableObject2ObjectOpenCustomHashMapWithCursorTest {
+ private final ReflectionObjectSizer sizer =
ReflectionObjectSizer.getInstance();
+
+ private static final Hash.Strategy<Integer> NATURAL_HASH = new
Hash.Strategy<Integer>() {
+ @Override
+ public int hashCode(Integer o) {
+ return o.hashCode();
+ }
+
+ @Override
+ public boolean equals(Integer a, Integer b) {
+ return a.equals(b);
+ }
+ };
+
+ private static Hash.Strategy<Integer> COLLIDING_HASH = new
Hash.Strategy<Integer>() {
+ @Override
+ public int hashCode(Integer o) {
+ return o % 5;
+ }
+
+ @Override
+ public boolean equals(Integer a, Integer b) {
+ return a.equals(b);
+ }
+ };
+
+ @Test
+ public void scanEntireMap_ReturnsExpectedElements() {
+ SizeableObject2ObjectOpenCustomHashMapWithCursor<Integer, String> map =
+ new SizeableObject2ObjectOpenCustomHashMapWithCursor<>(NATURAL_HASH);
+ IntStream.range(0, 10).forEach(i -> map.put(i, "value-" + i));
+
+ HashMap<Integer, String> scanned = new HashMap<>();
Review comment:
Thanks for catching this. This code was just moved, not actually written
for this PR, but I'll clean up the class while I'm here.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/size/ReflectionSingleObjectSizer.java
##########
@@ -179,5 +181,8 @@ private static int sizeType(Class<?> t) {
}
}
+ public static ReflectionSingleObjectSizer getInstance() {
+ return INSTANCE;
+ }
Review comment:
Done!
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursor.java
##########
@@ -183,6 +192,50 @@ private int hash(K key) {
return mix(strategy.hashCode(key)) & mask;
}
+ @Override
+ public V put(K k, V v) {
+ V oldValue = super.put(k, v);
+ if (oldValue == null) {
+ // A create
+ arrayContentsOverhead += (int) (elementSizer.sizeof(k) +
elementSizer.sizeof(v));
+ } else {
+ // An update
+ arrayContentsOverhead += (int) (elementSizer.sizeof(v) -
elementSizer.sizeof(oldValue));
+ }
+ return oldValue;
+ }
+
+ @Override
+ public V remove(Object k) {
+ V oldValue = super.remove(k);
+ if (oldValue != null) {
+ arrayContentsOverhead -= (elementSizer.sizeof(k) +
elementSizer.sizeof(oldValue));
+ }
+ return oldValue;
+ }
+
+ @Override
+ public int getSizeInBytes() {
+ return arrayContentsOverhead + calculateBackingArraysOverhead();
+ }
+
+ @VisibleForTesting
+ int calculateBackingArraysOverhead() {
+ // This formula determined experimentally using tests.
+ return BACKING_ARRAY_OVERHEAD_CONSTANT
+ + BACKING_ARRAY_LENGTH_COEFFICIENT * (key.length + value.length);
Review comment:
Done!
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursorTest.java
##########
@@ -0,0 +1,349 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.collections;
+
+import static
org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_LENGTH_COEFFICIENT;
+import static
org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_OVERHEAD_CONSTANT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.stream.IntStream;
+
+import it.unimi.dsi.fastutil.Hash;
+import it.unimi.dsi.fastutil.bytes.ByteArrays;
+import org.assertj.core.api.SoftAssertions;
+import org.junit.Test;
+
+import org.apache.geode.internal.size.ReflectionObjectSizer;
+
+public class SizeableObject2ObjectOpenCustomHashMapWithCursorTest {
+ private final ReflectionObjectSizer sizer =
ReflectionObjectSizer.getInstance();
+
+ private static final Hash.Strategy<Integer> NATURAL_HASH = new
Hash.Strategy<Integer>() {
+ @Override
+ public int hashCode(Integer o) {
+ return o.hashCode();
+ }
+
+ @Override
+ public boolean equals(Integer a, Integer b) {
+ return a.equals(b);
+ }
+ };
+
+ private static Hash.Strategy<Integer> COLLIDING_HASH = new
Hash.Strategy<Integer>() {
+ @Override
+ public int hashCode(Integer o) {
+ return o % 5;
+ }
+
+ @Override
+ public boolean equals(Integer a, Integer b) {
+ return a.equals(b);
+ }
+ };
+
+ @Test
+ public void scanEntireMap_ReturnsExpectedElements() {
+ SizeableObject2ObjectOpenCustomHashMapWithCursor<Integer, String> map =
+ new SizeableObject2ObjectOpenCustomHashMapWithCursor<>(NATURAL_HASH);
+ IntStream.range(0, 10).forEach(i -> map.put(i, "value-" + i));
+
+ HashMap<Integer, String> scanned = new HashMap<>();
+ int result = map.scan(0, 10000, HashMap::put, scanned);
+ assertThat(result).isEqualTo(0);
+ assertThat(scanned).isEqualTo(map);
+ }
+
+ @Test
+ public void twoScansWithNoModifications_ReturnsExpectedElements() {
+ SizeableObject2ObjectOpenCustomHashMapWithCursor<Integer, String> map =
+ new SizeableObject2ObjectOpenCustomHashMapWithCursor<>(NATURAL_HASH);
+ IntStream.range(0, 10).forEach(i -> map.put(i, "value-" + i));
+
+ HashMap<Integer, String> scanned = new HashMap<>();
+ int cursor = map.scan(0, 3, HashMap::put, scanned);
+ assertThat(scanned).hasSize(3);
+ cursor = map.scan(cursor, 3, HashMap::put, scanned);
+ assertThat(scanned).hasSize(6);
+ cursor = map.scan(cursor, 4, HashMap::put, scanned);
+ assertThat(scanned).hasSize(10);
+ cursor = map.scan(cursor, 4, HashMap::put, scanned);
+ assertThat(scanned).hasSize(10);
+ assertThat(cursor).isEqualTo(0);
+
+ assertThat(scanned).isEqualTo(map);
+ }
Review comment:
I didn't write this test so I could be misunderstanding what it's trying
to do, but I don't think it's possible to have a separate test for each
assertion, as the test is showing that successive calls to scan behave as
expected. I've refactored the test slightly to simplify it to just two calls.
--
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]
