asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Blow (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: BufferCache Concurrency Fixes
Date Thu, 28 Apr 2016 03:35:08 GMT
Michael Blow has uploaded a new change for review.

  https://asterix-gerrit.ics.uci.edu/835

Change subject: BufferCache Concurrency Fixes
......................................................................

BufferCache Concurrency Fixes

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

Change-Id: I01b4ab3000ae6f481f226c0df9fe876c6b16c7aa
---
M hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java
M hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java
M hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ClockPageReplacementStrategy.java
M hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/ICachedPageInternal.java
4 files changed, 40 insertions(+), 31 deletions(-)


  git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb refs/changes/35/835/1

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..df1b205 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,9 @@
                      */
                     bucket.bucketLock.lock();
                     try {
+                        if (!victim.pinCount.compareAndSet(0, 1)) {
+                            continue;
+                        }
                         if (DEBUG) {
                             confiscateLock.lock();
                             try{
@@ -324,8 +327,7 @@
                      */
                     bucket.bucketLock.lock();
                     try {
-                        if (victim.pinCount.get() != 1) {
-                            victim.pinCount.decrementAndGet();
+                        if (!victim.pinCount.compareAndSet(0, 1)) {
                             continue;
                         }
                         if (DEBUG) {
@@ -371,8 +373,7 @@
                         victimBucket.bucketLock.lock();
                     }
                     try {
-                        if (victim.pinCount.get() != 1) {
-                            victim.pinCount.decrementAndGet();
+                        if (!victim.pinCount.compareAndSet(0, 1)) {
                             continue;
                         }
                         if (DEBUG) {
@@ -992,8 +993,7 @@
                 // find a page that would possibly be evicted anyway
                 // Case 1 from findPage()
                 if (victim.dpid < 0) { // new page
-                    if (victim.pinCount.get() != 1) {
-                        victim.pinCount.decrementAndGet();
+                    if (!victim.pinCount.compareAndSet(0, 1)) {
                         continue;
                     }
                     returnPage = victim;
@@ -1013,8 +1013,7 @@
                         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
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 @@
     }
 
     @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;
         }
     }
 
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 @@
 
     @Override
     public ICachedPageInternal findVictim() {
-        ICachedPageInternal cachedPage = null;
+        ICachedPageInternal cachedPage;
         if (numPages.get() >= maxAllowedNumPages) {
             cachedPage = findVictimByEviction();
         } else {
@@ -75,31 +75,40 @@
     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 @@
         numPages.incrementAndGet();
         AtomicBoolean accessedFlag = getPerPageObject(cPage);
         if (!accessedFlag.compareAndSet(true, false)) {
-            if (cPage.pinIfGoodVictim()) {
+            if (cPage.isGoodVictim()) {
                 return cPage;
             }
         }
@@ -121,17 +130,18 @@
     }
 
     //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;
 
     }
 
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 Object getReplacementStrategyObject();
 
-    public boolean pinIfGoodVictim();
+    public boolean isGoodVictim();
 
 }

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/835
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I01b4ab3000ae6f481f226c0df9fe876c6b16c7aa
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Michael Blow <michael.blow@couchbase.com>

Mime
View raw message