[Hive-22760] Adding Clock based eviction strategy. #944

b-slim · 2020-03-07T01:00:49Z

This Pr adds a new Eviction strategy based on simple clock algorithm.
Main goal is to bring down the memory overhead of the cache accounting and make it lock free.
To reduce the memory overhead i have removed the unwanted fields used by lrfu strategy.
To Avoid locking i have added one bit as part of the buffer state that is modified as cas operation.

This change is

Change-Id: I6f2877f92557b05f9334700b007244dde3dc52c4 add methods to set and unset the clock ref bit Change-Id: I0d8406befb214cf696d4f48982cff2fccc58a2a4

Change-Id: I0e3966e3ef5fcc3ecab1a5fbf0a6c1d2f57fcc2d

Change-Id: I17abbae9573a41faf969558d148051f0004caef8

odraese

Minor comments about style/efficiency but no real bugs.

odraese · 2020-03-20T19:35:19Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

@@ -4187,6 +4188,8 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
         "partitions and tables) for reporting."),
    LLAP_USE_LRFU("hive.llap.io.use.lrfu", true,


so you want to flag this as deprecated?

good point.

odraese · 2020-03-20T19:37:01Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

@@ -4187,6 +4188,8 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
         "partitions and tables) for reporting."),
    LLAP_USE_LRFU("hive.llap.io.use.lrfu", true,
        "Whether ORC low-level cache should use LRFU cache policy instead of default (FIFO)."),
+    LLAP_IO_CACHE_STRATEGY("hive.llap.io.replacement.strategy", "lrfu", new StringSet("fifo", "clock", "lrfu"),
+        "Cache replacement strategy used by low-level (default to LRFU)."),


used by low-level what?
would rewrite to "Replacement strategy for IO/data cache"

odraese · 2020-03-20T19:38:21Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

@@ -4196,6 +4199,9 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
    LLAP_LRFU_BP_WRAPPER_SIZE("hive.llap.io.lrfu.bp.wrapper.size", 64, "thread local queue "
        + "used to amortize the lock contention, the idea hear is to try locking as soon we reach max size / 2 "
        + "and block when max queue size reached"),
+    LLAP_IO_MAX_CLOCK_ROTATION("hive.llap.io.clock.max.rotation",


do we really need to configure this? Sounds to me like something that a customer would never override (and which we would have a const)

Agree with you this is not for customer at all, it is more for dev nob in case of issue we can push the nob on the customer side without deploying code. Will add more comments about it. this makes me thing that we need to add some annotation about what configs are suppose to be dev configs and the one that can be used by customer. Any other suggestion will be welcomed.

odraese · 2020-03-20T22:05:55Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/ClockCachePolicy.java

+   *
+   * @param buffer buffer that just got unlocked after a read.
+   */
+  @Override public void notifyUnlock(LlapCacheableBuffer buffer) {


Maybe unlock would be the better position to set the clock bit (this is the point until the buffer was used). While it is locked, the buffer is protected anyway and no-one is really looking at the clock bit.

The bit set is lock free so i do not think will change much TBH. to avoid changing lot of the tests setup i will keep it as is and change it in the future if we see a strong case for it. Thanks

odraese · 2020-03-20T22:28:45Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/ClockCachePolicy.java

+      // clock position as soon we are done with looping
+      LlapCacheableBuffer currentClockHead = clockHand;
+
+      while (evicted < memoryToReserve && currentClockHead != null && fullClockRotation < maxCircles) {


There is currently no code path, which could lead to currentClockHead being null.

good point.

odraese · 2020-03-20T23:08:17Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/LrfuCacheAttribute.java

+
+public final class LrfuCacheAttribute implements LlapCacheableBuffer.CacheAttribute {
+  /** Priority for cache policy (should be pretty universal). */
+  public double priority;


all these have public setter/getter methods. there isn't any reason to have the fields public as well...?

odraese · 2020-03-20T23:14:58Z

llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java

+    if (strings == null) {
+      return new int[0];
+    }
+    return Arrays.stream(strings).map(x -> {


not that I don't appreciate a nice stream function but I suspect this is the least efficient method to covnert integer strings to int. each instance is wrapped into an optional just fo filter out the non-parsable directly again?

i removed the wrapping

odraese · 2020-03-20T23:16:56Z

llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/MetadataCache.java

@@ -406,7 +405,8 @@ private boolean lockBuffer(LlapBufferOrBuffers buffers, boolean doNotifyPolicy)
    for (int i = 0; i < bufferArray.length; ++i) {
      if (lockOneBuffer(bufferArray[i], doNotifyPolicy)) continue;
      for (int j = 0; j < i; ++j) {
-        unlockSingleBuffer(buffer, true);
+        //@TODO this is a bug by it self.


I would suggest to either fix the bug while you're at it or to add more documentation to this comment, why this is a bug.

odraese · 2020-03-20T23:18:01Z

llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/OrcFileEstimateErrors.java

@@ -32,6 +32,11 @@
 import org.apache.hadoop.hive.ql.util.IncrementalObjectSizeEstimator;
 import org.apache.hadoop.hive.ql.util.IncrementalObjectSizeEstimator.ObjectEstimator;

+/**
+ * Metadata Cache entry that does hold information about Files.
+ * No actual allocated bytebuffers is used by this class,


bytebuffers -> bytebuffer

odraese · 2020-03-20T23:19:14Z

storage-api/src/java/org/apache/hadoop/hive/common/io/CacheTag.java

@@ -239,7 +239,7 @@ public int hashCode() {
  /**
   * CacheTag for tables with more than one partition level.
   */
-  public static final class MultiPartitionCacheTag extends PartitionCacheTag {
+  public static final class   MultiPartitionCacheTag extends PartitionCacheTag {


I assume, these additional spaces were unintentional?

Change-Id: I81f896b067cda067dc4e695945851672701b5e84

Change-Id: Iccad6704f4f98020364f4ac05ebe63bbc62e1deb

Change-Id: I2bff7d5c044844a461ed439c26c71543922cd302

Change-Id: I807142e9ec312cf735dce079f85b9270b4e2caf0

prasanthj · 2020-03-30T18:46:01Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/CacheAttribute.java

+   * @return actual time where the buffer was touched last.
+   */
+  long getLastUpdate();
+  void setTouchTime(long time);


nit: add interface doc for the public methods?

prasanthj · 2020-03-30T18:48:58Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/ClockCachePolicy.java

+ * Eviction start at the current clock hand following the next pointer.
+ *
+ */
+public class ClockCachePolicy implements LowLevelCachePolicy {


+1. I was expecting this to be ring buffer based lock free implementation as well. This can be done in a follow up too.

prasanthj · 2020-03-30T18:49:48Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/ClockCachePolicy.java

+   * @param buffer new entry to be added.
+   * @return the ring head.
+   */
+  private static LlapCacheableBuffer appendToCircularList(LlapCacheableBuffer head, LlapCacheableBuffer buffer) {


static required?

makes compiler/inlining better and testing much simpler.

prasanthj · 2020-03-30T18:49:58Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/ClockCachePolicy.java

+   * @param buffer input
+   * @return buffer
+   */
+  private static LlapCacheableBuffer linkToItSelf(LlapCacheableBuffer buffer) {


same here. why static?

Same as above those method are generic enough to be static

prasanthj · 2020-03-30T18:51:02Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/ClockCachePolicy.java

+  /**
+   * Signals to the policy that it has to evict some entries from the cache.
+   * Policy has to at least evict the amount memory requested.
+   * Not that is method will block until at least {@code memoryToReserve} bytes are evicted.


Will be good to log how long it took to allocate the requested memory for debugging.

prasanthj · 2020-03-30T18:55:47Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/ClockCachePolicy.java

+    }
+  }
+
+  @VisibleForTesting protected Iterator<LlapCacheableBuffer> getIterator() {


If this is required only for testing. Should there be protected access? Or can it be package private?

prasanthj · 2020-03-30T18:59:55Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapAllocatorBuffer.java

+    static final int FLAG_MEM_RELEASED = 0b01000; // The memory was released to memory manager.
+    static final int FLAG_NEW_ALLOC =    0b10000; // New allocation before the first use; cannot force-evict.
+
+    static final int FLAGS_WIDTH = 5;


can you add a comment of how these width's are laid out?

prasanthj · 2020-03-30T19:02:36Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/LlapAllocatorBuffer.java

+  /**
+   * Utility class to manipulate the buffer state.
+   */
+   static final class State {


There seems to be many global state manipulation. Can you lease a comment that the state manipulations are global. Can rename the class to be CacheState/GlobalCacheState to be more precise about what state it represents.

Sure I will but, it will make the diff bigger therefore more code review :D

prasanthj · 2020-03-30T20:59:48Z

llap-server/src/java/org/apache/hadoop/hive/llap/cache/LowLevelCachePolicy.java

+  static LowLevelCachePolicy provideFromConf(Configuration conf) {
+    final long totalMemorySize = HiveConf.getSizeVar(conf, HiveConf.ConfVars.LLAP_IO_MEMORY_MAX_SIZE);
+    final int minAllocSize = (int) HiveConf.getSizeVar(conf, HiveConf.ConfVars.LLAP_ALLOCATOR_MIN_ALLOC);
+    String policyName = HiveConf.getVar(conf,HiveConf.ConfVars.LLAP_IO_CACHE_STRATEGY);


or make the policy name lowercase here to be case insensitive always?

github-actions · 2020-06-06T00:23:15Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

b-slim added 3 commits March 6, 2020 15:57

making room for clock bit

8b32d7a

Change-Id: I6f2877f92557b05f9334700b007244dde3dc52c4 add methods to set and unset the clock ref bit Change-Id: I0d8406befb214cf696d4f48982cff2fccc58a2a4

Added clock Policy with tests

5df637c

Change-Id: I0e3966e3ef5fcc3ecab1a5fbf0a6c1d2f57fcc2d

Misc fixes for style and find bug module

54302ae

Change-Id: I17abbae9573a41faf969558d148051f0004caef8

odraese approved these changes Mar 20, 2020

View reviewed changes

b-slim added 4 commits March 23, 2020 08:35

Merge branch 'master' into HIVE-22760_add_clock

a1624e5

Change-Id: I81f896b067cda067dc4e695945851672701b5e84

Addressed most of Odrease comments

31b4978

Change-Id: Iccad6704f4f98020364f4ac05ebe63bbc62e1deb

more cleanup after Olli comments

a5bc17e

Change-Id: I2bff7d5c044844a461ed439c26c71543922cd302

Merge branch 'master' into HIVE-22760_add_clock

861e10b

Change-Id: I807142e9ec312cf735dce079f85b9270b4e2caf0

prasanthj reviewed Mar 30, 2020

View reviewed changes

github-actions bot added the stale label Jun 6, 2020

github-actions bot closed this Jun 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hive-22760] Adding Clock based eviction strategy. #944

[Hive-22760] Adding Clock based eviction strategy. #944

b-slim commented Mar 7, 2020 •

edited

odraese left a comment

odraese Mar 20, 2020

b-slim Mar 23, 2020

odraese Mar 20, 2020

odraese Mar 20, 2020

b-slim Mar 23, 2020

odraese Mar 20, 2020

b-slim Mar 23, 2020

odraese Mar 20, 2020

b-slim Mar 23, 2020

odraese Mar 20, 2020

odraese Mar 20, 2020

b-slim Mar 23, 2020

odraese Mar 20, 2020

odraese Mar 20, 2020

odraese Mar 20, 2020

prasanthj Mar 30, 2020

prasanthj Mar 30, 2020

prasanthj Mar 30, 2020

b-slim Apr 1, 2020

prasanthj Mar 30, 2020

b-slim Apr 1, 2020

prasanthj Mar 30, 2020

prasanthj Mar 30, 2020

prasanthj Mar 30, 2020

prasanthj Mar 30, 2020

prasanthj Mar 30, 2020

b-slim Apr 1, 2020

prasanthj Mar 30, 2020

github-actions bot commented Jun 6, 2020

		@@ -4187,6 +4188,8 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
		"partitions and tables) for reporting."),
		LLAP_USE_LRFU("hive.llap.io.use.lrfu", true,

[Hive-22760] Adding Clock based eviction strategy. #944

[Hive-22760] Adding Clock based eviction strategy. #944

Conversation

b-slim commented Mar 7, 2020 • edited

odraese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 6, 2020

b-slim commented Mar 7, 2020 •

edited