DonalEvans commented on a change in pull request #6699:
URL: https://github.com/apache/geode/pull/6699#discussion_r673417456
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -173,20 +176,23 @@ public int getDSFID() {
}
protected synchronized byte[] memberAdd(byte[] memberToAdd, byte[]
scoreToAdd) {
- byte[] oldScore = null;
-
- OrderedSetEntry newEntry = new OrderedSetEntry(memberToAdd, scoreToAdd);
- OrderedSetEntry orderedSetEntry = members.put(memberToAdd, newEntry);
- if (orderedSetEntry == null) {
+ OrderedSetEntry existingEntry = members.get(memberToAdd);
+ if (existingEntry == null) {
+ OrderedSetEntry newEntry = new OrderedSetEntry(memberToAdd, scoreToAdd);
+ members.put(memberToAdd, newEntry);
scoreSet.add(newEntry);
- sizeInBytes += calculateSizeOfFieldValuePair(memberToAdd, scoreToAdd);
+ // Without this adjustment, we count the entry and member name array
twice, since references
+ // to them appear in both backing collections.
+ sizeInBytesAdjustment += newEntry.getSizeInBytes() +
calculateByteArraySize(memberToAdd);
+ return null;
} else {
- scoreSet.remove(orderedSetEntry);
- scoreSet.add(newEntry);
- oldScore = orderedSetEntry.getScoreBytes();
- sizeInBytes += scoreToAdd.length - oldScore.length;
+ scoreSet.remove(existingEntry);
+ byte[] oldScore = existingEntry.scoreBytes;
+ existingEntry.updateScore(stripTrailingZeroFromDouble(scoreToAdd));
+ members.put(memberToAdd, existingEntry);
Review comment:
I think that the reason we store both the byte array and double is that
some of the SortedSet commands (zadd and zrem) were implemented before we had
an actual sorted set implementation, so were just using the member and score
byte arrays in a `SizeableObject2ObjectOpenCustomHashMapWithCursor`. With that
original implementation, it was faster to leave the scores in their byte array
format at all times, but after new commands and the `OrderStatisticsSet` were
added to RedisSortedSet, this is no longer the case.
I think that the only time we need a byte array rather than a double is when
we're sending the response, so we can eliminate the field from
`AbstractOrderedStatisticsEntry` entirely and just change how we return from
certain sorted set methods. However, this is a potentially large refactor
that's not strictly within the scope of this PR, so I'd prefer to open a new
ticket for it rather than do it as part of this one.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
