Return-Path: X-Original-To: apmail-asterixdb-notifications-archive@minotaur.apache.org Delivered-To: apmail-asterixdb-notifications-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 1FE2719C46 for ; Thu, 28 Apr 2016 03:34:35 +0000 (UTC) Received: (qmail 71806 invoked by uid 500); 28 Apr 2016 03:34:35 -0000 Delivered-To: apmail-asterixdb-notifications-archive@asterixdb.apache.org Received: (qmail 71767 invoked by uid 500); 28 Apr 2016 03:34:35 -0000 Mailing-List: contact notifications-help@asterixdb.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@asterixdb.incubator.apache.org Delivered-To: mailing list notifications@asterixdb.incubator.apache.org Received: (qmail 71758 invoked by uid 99); 28 Apr 2016 03:34:35 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 28 Apr 2016 03:34:35 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id 91961C299D for ; Thu, 28 Apr 2016 03:34:34 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 0.919 X-Spam-Level: X-Spam-Status: No, score=0.919 tagged_above=-999 required=6.31 tests=[SPF_FAIL=0.919] autolearn=disabled Received: from mx2-lw-us.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id ruIf50wSFDWl for ; Thu, 28 Apr 2016 03:34:32 +0000 (UTC) Received: from unhygienix.ics.uci.edu (unhygienix.ics.uci.edu [128.195.14.130]) by mx2-lw-us.apache.org (ASF Mail Server at mx2-lw-us.apache.org) with ESMTP id B1E855F119 for ; Thu, 28 Apr 2016 03:34:31 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by unhygienix.ics.uci.edu (Postfix) with ESMTP id E9184240687; Wed, 27 Apr 2016 20:35:08 -0700 (PDT) Date: Wed, 27 Apr 2016 20:35:08 -0700 From: "Michael Blow (Code Review)" Message-ID: Reply-To: michael.blow@couchbase.com X-Gerrit-MessageType: newchange Subject: Change in asterixdb[master]: BufferCache Concurrency Fixes X-Gerrit-Change-Id: I01b4ab3000ae6f481f226c0df9fe876c6b16c7aa X-Gerrit-ChangeURL: X-Gerrit-Commit: e19a66450134f84666149a0285953ae49aa9dd82 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/2.8.4 To: undisclosed-recipients:; 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