asterixdb-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mb...@apache.org
Subject incubator-asterixdb git commit: BufferCache Concurrency Fixes
Date Mon, 02 May 2016 23:56:52 GMT
Repository: incubator-asterixdb
Updated Branches:
  refs/heads/master 8b3412cb8 -> 784417619


BufferCache Concurrency Fixes

1. Fix thread-safety issues in ClockPageReplacementStrategy.findVictimByEviction
2. Fix race-condition between page evicition & file deletion

Change-Id: I01b4ab3000ae6f481f226c0df9fe876c6b16c7aa
Reviewed-on: https://asterix-gerrit.ics.uci.edu/835
Tested-by: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Reviewed-by: Yingyi Bu <buyingyi@gmail.com>


Project: http://git-wip-us.apache.org/repos/asf/incubator-asterixdb/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-asterixdb/commit/78441761
Tree: http://git-wip-us.apache.org/repos/asf/incubator-asterixdb/tree/78441761
Diff: http://git-wip-us.apache.org/repos/asf/incubator-asterixdb/diff/78441761

Branch: refs/heads/master
Commit: 78441761917bb427564bf5eaa2265e93adc45dcd
Parents: 8b3412c
Author: Michael Blow <mblow@apache.org>
Authored: Sun May 1 17:23:57 2016 -0400
Committer: Michael Blow <michael.blow@couchbase.com>
Committed: Mon May 2 16:56:52 2016 -0700

----------------------------------------------------------------------
 .../storage/common/buffercache/BufferCache.java | 33 ++++++++++++--
 .../storage/common/buffercache/CachedPage.java  |  6 +--
 .../ClockPageReplacementStrategy.java           | 48 ++++++++++++--------
 .../common/buffercache/ICachedPageInternal.java |  2 +-
 4 files changed, 61 insertions(+), 28 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-asterixdb/blob/78441761/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java
----------------------------------------------------------------------
diff --git a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java
b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java
index 27d0423..6020d7b 100644
--- a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java
+++ b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java
@@ -283,6 +283,15 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent
{
                      */
                     bucket.bucketLock.lock();
                     try {
+                        if (!victim.pinCount.compareAndSet(0, 1)) {
+                            continue;
+                        }
+                        // now that we have the pin, ensure the victim's dpid still is <
0, if it's not, decrement
+                        // pin count and try again
+                        if (victim.dpid >= 0) {
+                            victim.pinCount.decrementAndGet();
+                            continue;
+                        }
                         if (DEBUG) {
                             confiscateLock.lock();
                             try{
@@ -324,7 +333,12 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent
{
                      */
                     bucket.bucketLock.lock();
                     try {
-                        if (victim.pinCount.get() != 1) {
+                        if (!victim.pinCount.compareAndSet(0, 1)) {
+                            continue;
+                        }
+                        // now that we have the pin, ensure the victim's bucket hasn't changed,
if it has, decrement
+                        // pin count and try again
+                        if (victimHash != hash(victim.dpid)) {
                             victim.pinCount.decrementAndGet();
                             continue;
                         }
@@ -371,7 +385,12 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent
{
                         victimBucket.bucketLock.lock();
                     }
                     try {
-                        if (victim.pinCount.get() != 1) {
+                        if (!victim.pinCount.compareAndSet(0, 1)) {
+                            continue;
+                        }
+                        // now that we have the pin, ensure the victim's bucket hasn't changed,
if it has, decrement
+                        // pin count and try again
+                        if (victimHash != hash(victim.dpid)) {
                             victim.pinCount.decrementAndGet();
                             continue;
                         }
@@ -992,7 +1011,12 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent
{
                 // find a page that would possibly be evicted anyway
                 // Case 1 from findPage()
                 if (victim.dpid < 0) { // new page
-                    if (victim.pinCount.get() != 1) {
+                    if (!victim.pinCount.compareAndSet(0, 1)) {
+                        continue;
+                    }
+                    // now that we have the pin, ensure the victim's dpid still is < 0,
if it's not, decrement
+                    // pin count and try again
+                    if (victim.dpid >= 0) {
                         victim.pinCount.decrementAndGet();
                         continue;
                     }
@@ -1013,8 +1037,7 @@ public class BufferCache implements IBufferCacheInternal, ILifeCycleComponent
{
                         while (curr != null) {
                             if (curr == victim) { // we found where the victim
                                                   // resides in the hash table
-                                if (victim.pinCount.get() != 1) {
-                                    victim.pinCount.decrementAndGet();
+                                if (!victim.pinCount.compareAndSet(0, 1)) {
                                     break;
                                 }
                                 // if this is the first page in the bucket

http://git-wip-us.apache.org/repos/asf/incubator-asterixdb/blob/78441761/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java
----------------------------------------------------------------------
diff --git a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java
b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java
index 305b577..cda81ad 100644
--- a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java
+++ b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java
@@ -91,11 +91,11 @@ public class CachedPage implements ICachedPageInternal {
     }
 
     @Override
-    public boolean pinIfGoodVictim() {
+    public boolean isGoodVictim() {
         if (confiscated.get())
-            return false; //i am not a good victim because i cant flush!
+            return false; // i am not a good victim because i cant flush!
         else {
-            return pinCount.compareAndSet(0, 1);
+            return pinCount.get() == 0;
         }
     }
 

http://git-wip-us.apache.org/repos/asf/incubator-asterixdb/blob/78441761/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ClockPageReplacementStrategy.java
----------------------------------------------------------------------
diff --git a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ClockPageReplacementStrategy.java
b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ClockPageReplacementStrategy.java
index 6a82b97..5c89bb6 100644
--- a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ClockPageReplacementStrategy.java
+++ b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ClockPageReplacementStrategy.java
@@ -63,7 +63,7 @@ public class ClockPageReplacementStrategy implements IPageReplacementStrategy
{
 
     @Override
     public ICachedPageInternal findVictim() {
-        ICachedPageInternal cachedPage = null;
+        ICachedPageInternal cachedPage;
         if (numPages.get() >= maxAllowedNumPages) {
             cachedPage = findVictimByEviction();
         } else {
@@ -75,31 +75,40 @@ public class ClockPageReplacementStrategy implements IPageReplacementStrategy
{
     private ICachedPageInternal findVictimByEviction() {
         //check if we're starved from confiscation
         assert (maxAllowedNumPages > 0);
-        int startClockPtr = clockPtr.get();
+        int clockPtr = advanceClock();
+        int startClockPtr = clockPtr;
+        int lastClockPtr = -1;
         int cycleCount = 0;
-        do {
-            ICachedPageInternal cPage = bufferCache.getPage(clockPtr.get());
+        boolean looped = false;
+        while (true) {
+            ICachedPageInternal cPage = bufferCache.getPage(clockPtr);
 
             /*
              * We do two things here:
              * 1. If the page has been accessed, then we skip it -- The CAS would return
              * false if the current value is false which makes the page a possible candidate
              * for replacement.
-             * 2. We check with the buffer manager if it feels its a good idea to use this
+             * 2. We check with the buffer manager if it feels it's a good idea to use this
              * page as a victim.
              */
             AtomicBoolean accessedFlag = getPerPageObject(cPage);
             if (!accessedFlag.compareAndSet(true, false)) {
-                if (cPage.pinIfGoodVictim()) {
-                        return cPage;
+                if (cPage.isGoodVictim()) {
+                    return cPage;
                 }
             }
-            advanceClock();
-            if (clockPtr.get() == startClockPtr) {
-                ++cycleCount;
+            if (clockPtr < lastClockPtr) {
+                looped = true;
             }
-        } while (cycleCount < MAX_UNSUCCESSFUL_CYCLE_COUNT);
-        return null;
+            if (looped && clockPtr >= startClockPtr) {
+                if (++cycleCount >= MAX_UNSUCCESSFUL_CYCLE_COUNT) {
+                    return null;
+                }
+                looped = false;
+            }
+            lastClockPtr = clockPtr;
+            clockPtr = advanceClock();
+        }
     }
 
     @Override
@@ -113,7 +122,7 @@ public class ClockPageReplacementStrategy implements IPageReplacementStrategy
{
         numPages.incrementAndGet();
         AtomicBoolean accessedFlag = getPerPageObject(cPage);
         if (!accessedFlag.compareAndSet(true, false)) {
-            if (cPage.pinIfGoodVictim()) {
+            if (cPage.isGoodVictim()) {
                 return cPage;
             }
         }
@@ -121,17 +130,18 @@ public class ClockPageReplacementStrategy implements IPageReplacementStrategy
{
     }
 
     //derived from RoundRobinAllocationPolicy in Apache directmemory
-    private int advanceClock(){
-        boolean clockInDial = false;
-        int newClockPtr = 0;
+    private int advanceClock() {
+
+        boolean clockInDial;
+        int currClockPtr;
         do
         {
-            int currClockPtr = clockPtr.get();
-            newClockPtr = ( currClockPtr + 1 ) % numPages.get();
+            currClockPtr = clockPtr.get();
+            int newClockPtr = ( currClockPtr + 1 ) % numPages.get();
             clockInDial = clockPtr.compareAndSet( currClockPtr, newClockPtr );
         }
         while ( !clockInDial );
-        return newClockPtr;
+        return currClockPtr;
 
     }
 

http://git-wip-us.apache.org/repos/asf/incubator-asterixdb/blob/78441761/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ICachedPageInternal.java
----------------------------------------------------------------------
diff --git a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ICachedPageInternal.java
b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ICachedPageInternal.java
index 497192d..6a6c2f5 100644
--- a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ICachedPageInternal.java
+++ b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ICachedPageInternal.java
@@ -25,6 +25,6 @@ public interface ICachedPageInternal extends ICachedPage {
 
     public Object getReplacementStrategyObject();
 
-    public boolean pinIfGoodVictim();
+    public boolean isGoodVictim();
 
 }


Mime
View raw message