[GitHub] [hbase] thangTang commented on a diff in pull request #4233: HBASE-26850 Optimize the implementation of LRUCache in LRUDictionary

[GitBox <gi...@apache.org>]

thangTang commented on code in PR #4233:
URL: https://github.com/apache/hbase/pull/4233#discussion_r851850601


##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/util/LRUDictionary.java:
##########
@@ -95,23 +95,32 @@ private short put(byte[] array, int offset, int length) {
       byte[] stored = new byte[length];
       Bytes.putBytes(stored, 0, array, offset, length);
 
-      if (currSize < initSize) {
-        // There is space to add without evicting.
-        if (indexToNode[currSize] == null) {
-          indexToNode[currSize] = new Node();
-        }
-        indexToNode[currSize].setContents(stored, 0, stored.length);
-        setHead(indexToNode[currSize]);
-        short ret = (short) currSize++;
-        nodeToIndex.put(indexToNode[ret], ret);
-        return ret;
+      Node node = new Node();
+      node.setContents(stored, 0, stored.length);
+      if (nodeToIndex.containsKey(node)) {
+        short index = nodeToIndex.get(node);
+        node = indexToNode[index];
+        moveToHead(node);

Review Comment:
   > the old node is not reused if the contents being stored are different.
   
   Completely correct.
   This patch only reuse SAME node.
   Actually, In the previous implementation, if the nodes are same, the existing nodes will also be reused too, the only difference is this logic were in _findIdx_:
   ```
   private short findIdx(byte[] array, int offset, int length) {
         Short s;
         final Node comparisonNode = new Node();
         comparisonNode.setContents(array, offset, length);
         if ((s = nodeToIndex.get(comparisonNode)) != null) {
           moveToHead(indexToNode[s]);
           return s;
         } else {
           return -1;
         }
       }
   ```
   
   For the write link:
   
   ```
   CompressedKvEncoder#write
   ->
   LRUDictionary#findEntry (LRUDictionary#findIdx)
   ->
   LRUDictionary#addEntry
   ```
   
   But for the read link:
   ```
   CompressedKVDecoder#readIntoArray
   ->
   LRUDirectory#addEntry
   ```
   
   We could see, on the read link, it just addEntry directly, without findIdx(reuse the existing same node).
   So, I just thought it would be more beautiful to write this way.



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org