geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dschnei...@apache.org
Subject incubator-geode git commit: GEODE-1886: optimize off-heap reference checks
Date Fri, 16 Sep 2016 18:25:49 GMT
Repository: incubator-geode
Updated Branches:
  refs/heads/develop 8341d7783 -> b7518411d


GEODE-1886: optimize off-heap reference checks

EntryEventImpl now tests the region's off-heap boolean
before it will ask if the new/old value is an instanceof
StoredObject.


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

Branch: refs/heads/develop
Commit: b7518411d93ded8a8c83af6964ff74acb04a31ac
Parents: 8341d77
Author: Darrel Schneider <dschneider@pivotal.io>
Authored: Mon Sep 12 16:45:59 2016 -0700
Committer: Darrel Schneider <dschneider@pivotal.io>
Committed: Fri Sep 16 11:14:33 2016 -0700

----------------------------------------------------------------------
 .../geode/internal/cache/EntryEventImpl.java    | 92 +++++++++++++-------
 .../geode/internal/offheap/StoredObject.java    |  6 ++
 .../internal/cache/EntryEventImplTest.java      |  9 ++
 .../cache/SearchLoadAndWriteProcessorTest.java  |  1 +
 4 files changed, 78 insertions(+), 30 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/b7518411/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
index a555cca..6a964c0 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
@@ -770,7 +770,11 @@ public class EntryEventImpl
    * Note: to prevent the heap copy use getRawNewValue instead
    */
   public final Object getRawNewValueAsHeapObject() {
-    return OffHeapHelper.getHeapForm(OffHeapHelper.copyIfNeeded(basicGetNewValue()));
+    Object result = basicGetNewValue();
+    if (mayHaveOffHeapReferences()) {
+      result = OffHeapHelper.copyIfNeeded(result);
+    }
+    return result;
   }
   
   /**
@@ -791,27 +795,23 @@ public class EntryEventImpl
   @Released(ENTRY_EVENT_NEW_VALUE)
   protected void basicSetNewValue(@Retained(ENTRY_EVENT_NEW_VALUE) Object v) {
     if (v == this.newValue) return;
-    if (this.offHeapOk) {
-      OffHeapHelper.releaseAndTrackOwner(this.newValue, this);
-    }
-    if (isOffHeapReference(v)) {
-      ReferenceCountHelper.setReferenceCountOwner(this);
-      if (!((StoredObject) v).retain()) {
+    if (mayHaveOffHeapReferences()) {
+      if (this.offHeapOk) {
+        OffHeapHelper.releaseAndTrackOwner(this.newValue, this);
+      }
+      if (StoredObject.isOffHeapReference(v)) {
+        ReferenceCountHelper.setReferenceCountOwner(this);
+        if (!((StoredObject) v).retain()) {
+          ReferenceCountHelper.setReferenceCountOwner(null);
+          this.newValue = null;
+          return;
+        }
         ReferenceCountHelper.setReferenceCountOwner(null);
-        this.newValue = null;
-        return;
       }
-      ReferenceCountHelper.setReferenceCountOwner(null);
     }
     this.newValue = v;
     this.cachedSerializedNewValue = null;
   }
-  /**
-   * Returns true if this event has a reference to an off-heap new or old value.
-   */
-  public boolean hasOffHeapValue() {
-    return isOffHeapReference(this.newValue) || isOffHeapReference(this.oldValue);
-  }
   
   @Unretained
   protected final Object basicGetNewValue() {
@@ -823,8 +823,8 @@ public class EntryEventImpl
     return result;
   }
   
-  private static boolean isOffHeapReference(Object ref) {
-    return (ref instanceof StoredObject) && ((StoredObject)ref).hasRefCount();
+  private boolean isOffHeapReference(Object ref) {
+    return mayHaveOffHeapReferences() && StoredObject.isOffHeapReference(ref);
   }
   
   private class OldValueOwner {
@@ -859,7 +859,7 @@ public class EntryEventImpl
   private void basicSetOldValue(@Unretained(ENTRY_EVENT_OLD_VALUE) Object v) {
     @Released final Object curOldValue = this.oldValue;
     if (v == curOldValue) return;
-    if (this.offHeapOk) {
+    if (this.offHeapOk && mayHaveOffHeapReferences()) {
       if (ReferenceCountHelper.trackReferenceCounts()) {
         OffHeapHelper.releaseAndTrackOwner(curOldValue, new OldValueOwner());
       } else {
@@ -910,7 +910,11 @@ public class EntryEventImpl
    * To avoid the heap copy use getRawOldValue instead.
    */
   public final Object getRawOldValueAsHeapObject() {
-    return OffHeapHelper.getHeapForm(OffHeapHelper.copyIfNeeded(basicGetOldValue()));
+    Object result = basicGetOldValue();
+    if (mayHaveOffHeapReferences()) {
+      result = OffHeapHelper.copyIfNeeded(result);
+    }
+    return result;
   }
   /*
    * If the old value is off-heap return the StoredObject form (unretained OFF_HEAP_REFERENCE).

@@ -927,7 +931,7 @@ public class EntryEventImpl
   @Unretained(ENTRY_EVENT_OLD_VALUE)
   public final Object getOldValueAsOffHeapDeserializedOrRaw() {
     Object result = basicGetOldValue();
-    if (result instanceof StoredObject) {
+    if (mayHaveOffHeapReferences() && result instanceof StoredObject) {
       result = ((StoredObject) result).getDeserializedForReading();
     }
     return AbstractRegion.handleNotAvailable(result); // fixes 49499
@@ -1290,7 +1294,7 @@ public class EntryEventImpl
   @Unretained(ENTRY_EVENT_NEW_VALUE)
   public final Object getNewValueAsOffHeapDeserializedOrRaw() {
     Object result = getRawNewValue();
-    if (result instanceof StoredObject) {
+    if (mayHaveOffHeapReferences() && result instanceof StoredObject) {
       result = ((StoredObject) result).getDeserializedForReading();
     }
     return AbstractRegion.handleNotAvailable(result); // fixes 49499
@@ -1314,7 +1318,10 @@ public class EntryEventImpl
     return convertToStoredObject(basicGetOldValue());
   }
 
-  private static StoredObject convertToStoredObject(final Object tmp) {
+  private StoredObject convertToStoredObject(final Object tmp) {
+    if (!mayHaveOffHeapReferences()) {
+      return null;
+    }
     if (!(tmp instanceof StoredObject)) {
       return null;
     }
@@ -1780,7 +1787,11 @@ public class EntryEventImpl
     if (this.op != Operation.LOCAL_INVALIDATE
         && this.op != Operation.LOCAL_DESTROY) {
       // fix for bug 34387
-      tx.setPendingValue(OffHeapHelper.copyIfNeeded(v));
+      Object pv = v;
+      if (mayHaveOffHeapReferences()) {
+        pv = OffHeapHelper.copyIfNeeded(v);
+      }
+      tx.setPendingValue(pv);
     }
     tx.setCallbackArgument(getCallbackArgument());
   }
@@ -1797,7 +1808,9 @@ public class EntryEventImpl
       try {
         return setOldValue(v);
       } finally {
-        OffHeapHelper.releaseWithNoTracking(v);
+        if (mayHaveOffHeapReferences()) {
+          OffHeapHelper.releaseWithNoTracking(v);
+        }
       }
     }
     catch (EntryNotFoundException ex) {
@@ -2557,7 +2570,7 @@ public class EntryEventImpl
     private final byte[] serializedValue;
     
     SerializedCacheValueImpl(EntryEventImpl event, Region r, RegionEntry re, @Unretained
CachedDeserializable cd, byte[] serializedBytes) {
-      if (isOffHeapReference(cd)) {
+      if (event.isOffHeapReference(cd)) {
         this.event = event;
       } else {
         this.event = null;
@@ -2748,6 +2761,9 @@ public class EntryEventImpl
   public void release() {
     // noop if already freed or values can not be off-heap
     if (!this.offHeapOk) return;
+    if (!mayHaveOffHeapReferences()) {
+      return;
+    }
     synchronized (this.offHeapLock) {
       // Note that this method does not set the old/new values to null but
       // leaves them set to the off-heap value so that future calls to getOld/NewValue
@@ -2771,6 +2787,18 @@ public class EntryEventImpl
     }
   }
   
+  /**
+   * Return true if this EntryEvent may have off-heap references.
+   */
+  private boolean mayHaveOffHeapReferences() {
+    LocalRegion lr = this.region;
+    if (lr != null) {
+      return lr.getOffHeap();
+    }
+    // if region field is null it is possible that we have off-heap values
+    return true;
+  }
+
   void testHookReleaseInProgress() {
     // unit test can mock or override this method
   }
@@ -2781,7 +2809,7 @@ public class EntryEventImpl
    */
   public void disallowOffHeapValues() {
     if (isOffHeapReference(this.newValue) || isOffHeapReference(this.oldValue)) {
-      throw new IllegalStateException("This event does not support off-heap values");
+      throw new IllegalStateException("This event already has off-heap values");
     }
     synchronized (this.offHeapLock) {
       this.offHeapOk = false;
@@ -2794,9 +2822,13 @@ public class EntryEventImpl
    */
   @Released({ENTRY_EVENT_NEW_VALUE, ENTRY_EVENT_OLD_VALUE})
   public void copyOffHeapToHeap() {
+    if (!mayHaveOffHeapReferences()) {
+      this.offHeapOk = false;
+      return;
+    }
     synchronized (this.offHeapLock) {
       Object ov = basicGetOldValue();
-      if (isOffHeapReference(ov)) {
+      if (StoredObject.isOffHeapReference(ov)) {
         if (ReferenceCountHelper.trackReferenceCounts()) {
           ReferenceCountHelper.setReferenceCountOwner(new OldValueOwner());
           this.oldValue = OffHeapHelper.copyAndReleaseIfNeeded(ov);
@@ -2806,12 +2838,12 @@ public class EntryEventImpl
         }
       }
       Object nv = basicGetNewValue();
-      if (isOffHeapReference(nv)) {
+      if (StoredObject.isOffHeapReference(nv)) {
         ReferenceCountHelper.setReferenceCountOwner(this);
         this.newValue = OffHeapHelper.copyAndReleaseIfNeeded(nv);
         ReferenceCountHelper.setReferenceCountOwner(null);
       }
-      if (isOffHeapReference(this.newValue) || isOffHeapReference(this.oldValue)) {
+      if (StoredObject.isOffHeapReference(this.newValue) || StoredObject.isOffHeapReference(this.oldValue))
{
         throw new IllegalStateException("event's old/new value still off-heap after calling
copyOffHeapToHeap");
       }
       this.offHeapOk = false;

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/b7518411/geode-core/src/main/java/org/apache/geode/internal/offheap/StoredObject.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/internal/offheap/StoredObject.java
b/geode-core/src/main/java/org/apache/geode/internal/offheap/StoredObject.java
index 4fd12ef..87e2ca5 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/offheap/StoredObject.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/offheap/StoredObject.java
@@ -170,4 +170,10 @@ public interface StoredObject extends Sendable, CachedDeserializable,
Releasable
    */
   public StoredObject getStoredObjectWithoutHeapForm();
 
+  /**
+   * Return true if the given "o" is reference to off-heap memory.
+   */
+  public static boolean isOffHeapReference(Object o) {
+    return (o instanceof StoredObject) && ((StoredObject)o).hasRefCount();
+  }
 }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/b7518411/geode-core/src/test/java/org/apache/geode/internal/cache/EntryEventImplTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/EntryEventImplTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/EntryEventImplTest.java
index 973787c..e2b47d5 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/EntryEventImplTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/EntryEventImplTest.java
@@ -68,6 +68,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExportNewValueWithUnserializedStoredObject() {
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject newValue = mock(StoredObject.class);
     byte[] newValueBytes = new byte[]{1,2,3};
     when(newValue.getValueAsHeapByteArray()).thenReturn(newValueBytes);
@@ -174,6 +175,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExportNewValueWithSerializedStoredObjectAndImporterPrefersSerialized()
{
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject newValue = mock(StoredObject.class);
     when(newValue.isSerialized()).thenReturn(true);
     byte[] newValueBytes = new byte[]{1,2,3};
@@ -190,6 +192,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExportNewValueWithSerializedStoredObject() {
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject newValue = mock(StoredObject.class);
     when(newValue.isSerialized()).thenReturn(true);
     Object newValueDeserialized = "newValueDeserialized";
@@ -205,6 +208,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExportNewValueWithSerializedStoredObjectAndUnretainedNewReferenceOk()
{
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject newValue = mock(StoredObject.class);
     when(newValue.isSerialized()).thenReturn(true);
     Object newValueDeserialized = "newValueDeserialized";
@@ -221,6 +225,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExportOldValueWithUnserializedStoredObject() {
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject oldValue = mock(StoredObject.class);
     byte[] oldValueBytes = new byte[]{1,2,3};
     when(oldValue.getValueAsHeapByteArray()).thenReturn(oldValueBytes);
@@ -325,6 +330,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExportOldValueWithSerializedStoredObjectAndImporterPrefersSerialized()
{
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject oldValue = mock(StoredObject.class);
     when(oldValue.isSerialized()).thenReturn(true);
     byte[] oldValueBytes = new byte[]{1,2,3};
@@ -342,6 +348,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExportOldValueWithSerializedStoredObject() {
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject oldValue = mock(StoredObject.class);
     when(oldValue.isSerialized()).thenReturn(true);
     Object oldValueDeserialized = "newValueDeserialized";
@@ -358,6 +365,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExportOldValueWithSerializedStoredObjectAndUnretainedOldReferenceOk()
{
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject oldValue = mock(StoredObject.class);
     when(oldValue.isSerialized()).thenReturn(true);
     Object oldValueDeserialized = "oldValueDeserialized";
@@ -375,6 +383,7 @@ public class EntryEventImplTest {
   @Test
   public void verifyExternalReadMethodsBlockedByRelease() throws InterruptedException {
     LocalRegion region = mock(LocalRegion.class);
+    when(region.getOffHeap()).thenReturn(true);
     StoredObject newValue = mock(StoredObject.class);
     when(newValue.hasRefCount()).thenReturn(true);
     when(newValue.isSerialized()).thenReturn(true);

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/b7518411/geode-core/src/test/java/org/apache/geode/internal/cache/SearchLoadAndWriteProcessorTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/SearchLoadAndWriteProcessorTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/SearchLoadAndWriteProcessorTest.java
index 84c6652..135e27f 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/SearchLoadAndWriteProcessorTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/SearchLoadAndWriteProcessorTest.java
@@ -40,6 +40,7 @@ public class SearchLoadAndWriteProcessorTest {
     // setup
     SearchLoadAndWriteProcessor processor = SearchLoadAndWriteProcessor.getProcessor();
     LocalRegion lr = mock(LocalRegion.class);
+    when(lr.getOffHeap()).thenReturn(true);
     when(lr.getScope()).thenReturn(Scope.DISTRIBUTED_ACK);
     Object key = "key";
     StoredObject value = mock(StoredObject.class);


Mime
View raw message