You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2021/02/17 11:58:19 UTC
[GitHub] [iotdb] JunChen00 opened a new pull request #2689: LRULinkedHashMap does not work as an LRU Cache #2688
JunChen00 opened a new pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689
Fixes the issue [#2688](https://github.com/apache/iotdb/issues/2688)
**Describe the bug**
LRULinkedHashMap.java.
LinkedHashMap is used incorrectly.
To make LinkedHashMap an LRU cache, a true boolean should be provided for the param accessOrder of the constructor.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 commented on a change in pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
JackieTien97 commented on a change in pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#discussion_r579588087
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/cache/LRULinkedHashMap.java
##########
@@ -21,71 +21,56 @@
import org.apache.iotdb.tsfile.common.cache.Accountable;
-import java.util.Iterator;
import java.util.LinkedHashMap;
-import java.util.Map.Entry;
-import java.util.Set;
+import java.util.Map;
/** This class is an LRU cache. <b>Note: It's not thread safe.</b> */
-public abstract class LRULinkedHashMap<K extends Accountable, V> {
+public abstract class LRULinkedHashMap<K extends Accountable, V> extends LinkedHashMap<K, V> {
private static final float LOAD_FACTOR_MAP = 0.75f;
private static final int INITIAL_CAPACITY = 128;
- private static final float RETAIN_PERCENT = 0.9f;
private static final int MAP_ENTRY_SIZE = 40;
- private final LinkedHashMap<K, V> linkedHashMap;
-
/** maximum memory threshold. */
private final long maxMemory;
/** current used memory. */
private long usedMemory;
- /** memory size we need to retain while the cache is full */
- private final long retainMemory;
-
protected int count = 0;
protected long averageSize = 0;
public LRULinkedHashMap(long maxMemory) {
- this.linkedHashMap = new LinkedHashMap<>(INITIAL_CAPACITY, LOAD_FACTOR_MAP);
+ super(INITIAL_CAPACITY, LOAD_FACTOR_MAP, true);
this.maxMemory = maxMemory;
- this.retainMemory = (long) (maxMemory * RETAIN_PERCENT);
}
+ @Override
public V put(K key, V value) {
long size = calEntrySize(key, value) + MAP_ENTRY_SIZE;
key.setRamSize(size);
usedMemory += size;
- V v = linkedHashMap.put(key, value);
- if (usedMemory > maxMemory) {
- Iterator<Entry<K, V>> iterator = linkedHashMap.entrySet().iterator();
- while (usedMemory > retainMemory && iterator.hasNext()) {
- Entry<K, V> entry = iterator.next();
- usedMemory -= entry.getKey().getRamSize();
- iterator.remove();
- }
- }
- return v;
+ return super.put(key, value);
}
- public V get(K key) {
- return linkedHashMap.get(key);
- }
-
- public boolean containsKey(K key) {
- return linkedHashMap.containsKey(key);
+ @Override
+ protected boolean removeEldestEntry(Map.Entry<K, V> eldest) {
+ boolean shouldRemove = usedMemory > maxMemory;
+ if (shouldRemove) {
+ usedMemory -= eldest.getKey().getRamSize();
+ }
+ return shouldRemove;
Review comment:
The previous logic is when the LRUMap is full, we should evict 10% object, your implementation is inconsistent with that.
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/cache/LRULinkedHashMap.java
##########
@@ -21,71 +21,56 @@
import org.apache.iotdb.tsfile.common.cache.Accountable;
-import java.util.Iterator;
import java.util.LinkedHashMap;
-import java.util.Map.Entry;
-import java.util.Set;
+import java.util.Map;
/** This class is an LRU cache. <b>Note: It's not thread safe.</b> */
-public abstract class LRULinkedHashMap<K extends Accountable, V> {
+public abstract class LRULinkedHashMap<K extends Accountable, V> extends LinkedHashMap<K, V> {
Review comment:
I think we should keep LinkedHashMap as a field instead of extending LinkedHashMap, because we don't need to expose all the methods of LinkedHashMap to users.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] JunChen00 commented on a change in pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
JunChen00 commented on a change in pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#discussion_r578512168
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/cache/LRULinkedHashMap.java
##########
@@ -55,7 +55,7 @@
protected long averageSize = 0;
public LRULinkedHashMap(long maxMemory) {
- this.linkedHashMap = new LinkedHashMap<>(INITIAL_CAPACITY, LOAD_FACTOR_MAP);
Review comment:
I read the `put()` method, and I think it is ok to just enable `accessOrder = true`.
1、At line 63 of the current file, the removal of elements is realized outside of the LinkedHashMap.
2、Everytimes after an element is accessed, the element is put at the tail, so the head is the eldest one.
3、As the default value returned by `removeEldestEntry()` method is false, the `put()` method of LinkedHashMap does not remove any elements.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] HTHou merged pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
HTHou merged pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] jixuan1989 commented on a change in pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#discussion_r578862169
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/cache/LRULinkedHashMap.java
##########
@@ -55,7 +55,7 @@
protected long averageSize = 0;
public LRULinkedHashMap(long maxMemory) {
- this.linkedHashMap = new LinkedHashMap<>(INITIAL_CAPACITY, LOAD_FACTOR_MAP);
Review comment:
1. but why not use `removeEldestEntry `?
Can calculating the size in `put` bring any benefit ?
2. About added UT:
The UT does not take effect. Even you do not modify `LRULinkedHashMap.java` and only add your UT codes, the UT can also pass, which means the UT does not cover the incorrection of the original `LRULinkedHashMap.java`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] JunChen00 commented on a change in pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
JunChen00 commented on a change in pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#discussion_r578515790
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/cache/LRULinkedHashMap.java
##########
@@ -55,7 +55,7 @@
protected long averageSize = 0;
public LRULinkedHashMap(long maxMemory) {
- this.linkedHashMap = new LinkedHashMap<>(INITIAL_CAPACITY, LOAD_FACTOR_MAP);
Review comment:
I read the `put()` method, and I think it is ok to just enable `accessOrder = true`.
1、At line 63 of the current file, the removal of elements is realized outside of the LinkedHashMap.
2、Everytimes after an element is accessed (including the `put()` method and the `get()` method), the element is put at the tail, so the head is the eldest one.
3、As the default value returned by `removeEldestEntry()` method is false, the `put()` method of LinkedHashMap does not remove any elements.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] chenjun40 commented on pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
chenjun40 commented on pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#issuecomment-783789029
How to merge the commits? @JackieTien97
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] HTHou commented on pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
HTHou commented on pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#issuecomment-786419209
Test result:
https://cwiki.apache.org/confluence/display/IOTDB/PR-2689+LRULinkedHashMap+does+not+work+as+an+LRU+Cache
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] HTHou commented on pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
HTHou commented on pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#issuecomment-783804047
> How to merge the commits? @JackieTien97
Hi, we are working on the performance test of this PR in order to make sure the improvement before the commits merged. We'll share the result once we get it. Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] JunChen00 commented on a change in pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
JunChen00 commented on a change in pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#discussion_r579554658
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/cache/LRULinkedHashMap.java
##########
@@ -55,7 +55,7 @@
protected long averageSize = 0;
public LRULinkedHashMap(long maxMemory) {
- this.linkedHashMap = new LinkedHashMap<>(INITIAL_CAPACITY, LOAD_FACTOR_MAP);
Review comment:
1、extend `removeEldestEntry()` method
2、improve UT
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] JunChen00 commented on a change in pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
JunChen00 commented on a change in pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#discussion_r579667782
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/cache/LRULinkedHashMap.java
##########
@@ -21,71 +21,56 @@
import org.apache.iotdb.tsfile.common.cache.Accountable;
-import java.util.Iterator;
import java.util.LinkedHashMap;
-import java.util.Map.Entry;
-import java.util.Set;
+import java.util.Map;
/** This class is an LRU cache. <b>Note: It's not thread safe.</b> */
-public abstract class LRULinkedHashMap<K extends Accountable, V> {
+public abstract class LRULinkedHashMap<K extends Accountable, V> extends LinkedHashMap<K, V> {
Review comment:
Agree & Retract @JackieTien97
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] jixuan1989 commented on a change in pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689#discussion_r578069241
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/cache/LRULinkedHashMap.java
##########
@@ -55,7 +55,7 @@
protected long averageSize = 0;
public LRULinkedHashMap(long maxMemory) {
- this.linkedHashMap = new LinkedHashMap<>(INITIAL_CAPACITY, LOAD_FACTOR_MAP);
Review comment:
Hi,
I am not sure whether it is enough by just enabling `accessOrder = true`.
If you read the `put()` method, you will find it removes elements at the head of the set, rather than the tail...
Do we need to use `removeEldestEntry ()` method instead? @JackieTien97
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [iotdb] JunChen00 closed pull request #2689: [ISSUE-2688] LRULinkedHashMap does not work as an LRU Cache
Posted by GitBox <gi...@apache.org>.
JunChen00 closed pull request #2689:
URL: https://github.com/apache/iotdb/pull/2689
----------------------------------------------------------------
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:
users@infra.apache.org