nonbinaryprogrammer commented on a change in pull request #6296:
URL: https://github.com/apache/geode/pull/6296#discussion_r628390895
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSetTest.java
##########
@@ -165,4 +174,142 @@ public void
setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
o2.fromDelta(in);
assertThat(o2).isEqualTo(o1);
}
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROS_withVaryingMemberCounts() {
+ for (int i = 0; i < 1024; i += 16) {
+ RedisSet set = createRedisSetOfSpecifiedSize(i);
+
+ int expected = reflectionObjectSizer.sizeof(set);
+ Long actual = Long.valueOf(set.getSizeInBytes());
+ Offset<Long> offset = Offset.offset(Math.round(expected * 0.06));
+
+ assertThat(actual).isCloseTo(expected, offset);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROS_withVaryingMemberSize() {
+ for (int i = 0; i < 16; i++) {
+ RedisSet set = createRedisSetWithMemberOfSpecifiedSize(i * 64);
+ int expected = reflectionObjectSizer.sizeof(set);
+ Long actual = Long.valueOf(set.getSizeInBytes());
+ Offset<Long> offset = Offset.offset(Math.round(expected * 0.06));
+
+ assertThat(actual).isCloseTo(expected, offset);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void size_shouldDecrease_WhenValueIsRemoved() {
+ Region region = mock(Region.class);
+ final RedisData returnData = mock(RedisData.class);
+ when(region.put(Object.class, Object.class)).thenReturn(returnData);
+ final RedisKey key = new RedisKey("key".getBytes());
+ final ByteArrayWrapper value = new ByteArrayWrapper("value".getBytes());
+
+ RedisSet set = createRedisSetOfSpecifiedSize(10);
+
+ ArrayList<ByteArrayWrapper> members = new ArrayList<>();
+ members.add(value);
+ set.sadd(members, region, key);
+
+ int initialSize = set.getSizeInBytes();
+
+ set.srem(members, region, key);
+
+ int finalSize = set.getSizeInBytes();
+ int expectedSize = initialSize - value.length() - PER_MEMBER_OVERHEAD;
+
+ assertThat(finalSize).isEqualTo(expectedSize);
+ }
+
+ /******** constants *******/
+ @Test
+ public void baseOverheadConstant_shouldMatchCalculatedValue() {
+ ReflectionObjectSizer reflectionObjectSizer =
ReflectionObjectSizer.getInstance();
+ int baseRedisSetOverhead = reflectionObjectSizer.sizeof(new RedisSet()) +
18;
+
+ HashSet<ByteArrayWrapper> temp_hashset = new HashSet<>();
+ int base_hashset_size = reflectionObjectSizer.sizeof(temp_hashset);
+ baseRedisSetOverhead += base_hashset_size;
+
+ assertThat(baseRedisSetOverhead).isEqualTo(BASE_REDIS_SET_OVERHEAD);
+ }
+
+ @Test
+ public void perMemberOverheadConstant_shouldMatchCalculatedValue() {
+ ReflectionObjectSizer reflectionObjectSizer =
ReflectionObjectSizer.getInstance();
+
+ HashSet<ByteArrayWrapper> temp_hashset = new HashSet<>();
+ ByteArrayWrapper member1 = new ByteArrayWrapper("a".getBytes());
+ ByteArrayWrapper member2 = new ByteArrayWrapper("b".getBytes());
+ temp_hashset.add(member1);
+ int one_entry_hashset_size = reflectionObjectSizer.sizeof(temp_hashset);
+
+ temp_hashset.add(member2);
+ int two_entries_hashset_size = reflectionObjectSizer.sizeof(temp_hashset);
+
+ int perMemberOverhead = two_entries_hashset_size - one_entry_hashset_size
+ 5;
Review comment:
the magic 5 remains, after a lot more testing of RedisSet, and some
changes to the implementation. it technically shouldn't be a constant, and
instead is related to the size of the set, and of the bytearraywrappers within
the set. it is possible for us to be more accurate here, but it requires a lot
of cleverness. Since there's a story to remove ByteArrayWrappers, it seems like
a waste of time to get the size exactly right all the time, when we can be
within 5% without all that math.
--
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]
