incorporated review feedback
Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/74a994c0 Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/74a994c0 Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/74a994c0 Branch: refs/heads/feature/GEODE-1886 Commit: 74a994c095ad3d67882c0aca9d48da4cdee8d448 Parents: 0877a26 Author: Darrel Schneider <[email protected]> Authored: Wed Sep 14 14:31:28 2016 -0700 Committer: Darrel Schneider <[email protected]> Committed: Wed Sep 14 14:31:28 2016 -0700 ---------------------------------------------------------------------- .../gemfire/internal/cache/EntryEventImpl.java | 53 +++++++++----------- .../internal/cache/OffHeapRegionEntry.java | 4 ++ .../gemfire/internal/cache/RegionEntry.java | 4 ++ .../gemfire/internal/offheap/StoredObject.java | 6 +++ .../internal/cache/EntryEventImplTest.java | 22 -------- .../cache/SearchLoadAndWriteProcessorTest.java | 1 - 6 files changed, 39 insertions(+), 51 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/74a994c0/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/EntryEventImpl.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/EntryEventImpl.java b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/EntryEventImpl.java index f66770d..1f0d937 100644 --- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/EntryEventImpl.java +++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/EntryEventImpl.java @@ -771,7 +771,7 @@ public class EntryEventImpl */ public final Object getRawNewValueAsHeapObject() { Object result = basicGetNewValue(); - if (this.isOffHeapPossible()) { + if (this.mayHaveOffHeapReferences()) { result = OffHeapHelper.copyIfNeeded(result); } return result; @@ -795,11 +795,11 @@ public class EntryEventImpl @Released(ENTRY_EVENT_NEW_VALUE) protected void basicSetNewValue(@Retained(ENTRY_EVENT_NEW_VALUE) Object v) { if (v == this.newValue) return; - if (isOffHeapPossible()) { + if (mayHaveOffHeapReferences()) { if (this.offHeapOk) { OffHeapHelper.releaseAndTrackOwner(this.newValue, this); } - if (isOffHeapReference(v)) { + if (StoredObject.hasReferenceCount(v)) { ReferenceCountHelper.setReferenceCountOwner(this); if (!((StoredObject) v).retain()) { ReferenceCountHelper.setReferenceCountOwner(null); @@ -816,15 +816,15 @@ public class EntryEventImpl @Unretained protected final Object basicGetNewValue() { Object result = this.newValue; - if (!this.offHeapOk && isOffHeapPossible() && isOffHeapReference(result)) { + if (!this.offHeapOk && isOffHeapReference(result)) { //this.region.getCache().getLogger().info("DEBUG new value already freed " + System.identityHashCode(result)); throw new IllegalStateException("Attempt to access off heap value after the EntryEvent was released."); } return result; } - private static boolean isOffHeapReference(Object ref) { - return (ref instanceof StoredObject) && ((StoredObject)ref).hasRefCount(); + private boolean isOffHeapReference(Object ref) { + return this.mayHaveOffHeapReferences() && StoredObject.hasReferenceCount(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 && isOffHeapPossible()) { + if (this.offHeapOk && mayHaveOffHeapReferences()) { if (ReferenceCountHelper.trackReferenceCounts()) { OffHeapHelper.releaseAndTrackOwner(curOldValue, new OldValueOwner()); } else { @@ -874,7 +874,7 @@ public class EntryEventImpl private void retainAndSetOldValue(@Retained(ENTRY_EVENT_OLD_VALUE) Object v) { if (v == this.oldValue) return; - if (isOffHeapPossible() && isOffHeapReference(v)) { + if (isOffHeapReference(v)) { StoredObject so = (StoredObject) v; if (ReferenceCountHelper.trackReferenceCounts()) { ReferenceCountHelper.setReferenceCountOwner(new OldValueOwner()); @@ -898,7 +898,7 @@ public class EntryEventImpl private Object basicGetOldValue() { @Unretained(ENTRY_EVENT_OLD_VALUE) Object result = this.oldValue; - if (!this.offHeapOk && isOffHeapPossible() && isOffHeapReference(result)) { + if (!this.offHeapOk && isOffHeapReference(result)) { //this.region.getCache().getLogger().info("DEBUG old value already freed " + System.identityHashCode(result)); throw new IllegalStateException("Attempt to access off heap value after the EntryEvent was released."); } @@ -911,7 +911,7 @@ public class EntryEventImpl */ public final Object getRawOldValueAsHeapObject() { Object result = basicGetOldValue(); - if (isOffHeapPossible()) { + if (mayHaveOffHeapReferences()) { result = OffHeapHelper.copyIfNeeded(result); } return result; @@ -931,7 +931,7 @@ public class EntryEventImpl @Unretained(ENTRY_EVENT_OLD_VALUE) public final Object getOldValueAsOffHeapDeserializedOrRaw() { Object result = basicGetOldValue(); - if (isOffHeapPossible() && result instanceof StoredObject) { + if (mayHaveOffHeapReferences() && result instanceof StoredObject) { result = ((StoredObject) result).getDeserializedForReading(); } return AbstractRegion.handleNotAvailable(result); // fixes 49499 @@ -988,7 +988,7 @@ public class EntryEventImpl * @return the value returned from invoking the function */ private <T,R> R callWithOffHeapLock(T value, Function<T, R> function) { - if (isOffHeapPossible() && isOffHeapReference(value)) { + if (isOffHeapReference(value)) { synchronized (this.offHeapLock) { if (!this.offHeapOk) { throw new IllegalStateException("Attempt to access off heap value after the EntryEvent was released."); @@ -1294,7 +1294,7 @@ public class EntryEventImpl @Unretained(ENTRY_EVENT_NEW_VALUE) public final Object getNewValueAsOffHeapDeserializedOrRaw() { Object result = getRawNewValue(); - if (isOffHeapPossible() && result instanceof StoredObject) { + if (mayHaveOffHeapReferences() && result instanceof StoredObject) { result = ((StoredObject) result).getDeserializedForReading(); } return AbstractRegion.handleNotAvailable(result); // fixes 49499 @@ -1319,7 +1319,7 @@ public class EntryEventImpl } private StoredObject convertToStoredObject(final Object tmp) { - if (!isOffHeapPossible()) { + if (!mayHaveOffHeapReferences()) { return null; } if (!(tmp instanceof StoredObject)) { @@ -1788,7 +1788,7 @@ public class EntryEventImpl && this.op != Operation.LOCAL_DESTROY) { // fix for bug 34387 Object pv = v; - if (isOffHeapPossible()) { + if (mayHaveOffHeapReferences()) { pv = OffHeapHelper.copyIfNeeded(v); } tx.setPendingValue(pv); @@ -1808,7 +1808,7 @@ public class EntryEventImpl try { return setOldValue(v); } finally { - if (isOffHeapPossible()) { + if (mayHaveOffHeapReferences()) { OffHeapHelper.releaseWithNoTracking(v); } } @@ -2570,7 +2570,7 @@ public class EntryEventImpl private final byte[] serializedValue; SerializedCacheValueImpl(EntryEventImpl event, Region r, RegionEntry re, @Unretained CachedDeserializable cd, byte[] serializedBytes) { - if (event.isOffHeapPossible() && isOffHeapReference(cd)) { + if (event.isOffHeapReference(cd)) { this.event = event; } else { this.event = null; @@ -2761,7 +2761,7 @@ public class EntryEventImpl public void release() { // noop if already freed or values can not be off-heap if (!this.offHeapOk) return; - if (!isOffHeapPossible()) { + if (!mayHaveOffHeapReferences()) { return; } synchronized (this.offHeapLock) { @@ -2788,12 +2788,12 @@ public class EntryEventImpl } /** - * Return true if it is possible that this EntryEvent has off-heap references. + * Return true if this EntryEvent may have off-heap references. */ - private boolean isOffHeapPossible() { + private boolean mayHaveOffHeapReferences() { LocalRegion lr = this.region; if (lr != null) { - return lr.getAttributes().getOffHeap(); + return lr.getOffHeap(); } // if region field is null it is possible that we have off-heap values return true; @@ -2822,13 +2822,13 @@ public class EntryEventImpl */ @Released({ENTRY_EVENT_NEW_VALUE, ENTRY_EVENT_OLD_VALUE}) public void copyOffHeapToHeap() { - if (!isOffHeapPossible()) { + if (!mayHaveOffHeapReferences()) { this.offHeapOk = false; return; } synchronized (this.offHeapLock) { Object ov = basicGetOldValue(); - if (isOffHeapReference(ov)) { + if (StoredObject.hasReferenceCount(ov)) { if (ReferenceCountHelper.trackReferenceCounts()) { ReferenceCountHelper.setReferenceCountOwner(new OldValueOwner()); this.oldValue = OffHeapHelper.copyAndReleaseIfNeeded(ov); @@ -2838,12 +2838,12 @@ public class EntryEventImpl } } Object nv = basicGetNewValue(); - if (isOffHeapReference(nv)) { + if (StoredObject.hasReferenceCount(nv)) { ReferenceCountHelper.setReferenceCountOwner(this); this.newValue = OffHeapHelper.copyAndReleaseIfNeeded(nv); ReferenceCountHelper.setReferenceCountOwner(null); } - if (isOffHeapReference(this.newValue) || isOffHeapReference(this.oldValue)) { + if (StoredObject.hasReferenceCount(this.newValue) || StoredObject.hasReferenceCount(this.oldValue)) { throw new IllegalStateException("event's old/new value still off-heap after calling copyOffHeapToHeap"); } this.offHeapOk = false; @@ -2851,9 +2851,6 @@ public class EntryEventImpl } public boolean isOldValueOffHeap() { - if (!isOffHeapPossible()) { - return false; - } return isOffHeapReference(this.oldValue); } } \ No newline at end of file http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/74a994c0/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/OffHeapRegionEntry.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/OffHeapRegionEntry.java b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/OffHeapRegionEntry.java index acd1e63..aa075dd 100644 --- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/OffHeapRegionEntry.java +++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/OffHeapRegionEntry.java @@ -38,4 +38,8 @@ public interface OffHeapRegionEntry extends RegionEntry, Releasable { * @return newAddr OFF_HEAP_ADDRESS */ public boolean setAddress(long expectedAddr, long newAddr); + @Override + public default boolean isOffHeap() { + return true; + } } http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/74a994c0/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionEntry.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionEntry.java b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionEntry.java index 48ed5db..23920ff 100644 --- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionEntry.java +++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/RegionEntry.java @@ -490,4 +490,8 @@ public interface RegionEntry { @Retained(ABSTRACT_REGION_ENTRY_PREPARE_VALUE_FOR_CACHE) public Object prepareValueForCache(RegionEntryContext r, Object val, EntryEventImpl event, boolean isEntryUpdate); + + public default boolean isOffHeap() { + return false; + } } http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/74a994c0/geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/StoredObject.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/StoredObject.java b/geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/StoredObject.java index 47ad4e2..300c243 100644 --- a/geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/StoredObject.java +++ b/geode-core/src/main/java/com/gemstone/gemfire/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 a StoredObject and has a reference count. + */ + public static boolean hasReferenceCount(Object o) { + return (o instanceof StoredObject) && ((StoredObject)o).hasRefCount(); + } } http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/74a994c0/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/EntryEventImplTest.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/EntryEventImplTest.java b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/EntryEventImplTest.java index df53f3a..868e6f5 100644 --- a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/EntryEventImplTest.java +++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/EntryEventImplTest.java @@ -48,7 +48,6 @@ public class EntryEventImplTest { public void verifyToStringOutputHasRegionName() { // mock a region object LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); String expectedRegionName = "ExpectedFullRegionPathName"; String value = "value1"; KeyInfo keyInfo = new KeyInfo(key, value, null); @@ -69,7 +68,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithUnserializedStoredObject() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject newValue = mock(StoredObject.class); byte[] newValueBytes = new byte[]{1,2,3}; @@ -85,7 +83,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithByteArray() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); byte[] newValue = new byte[]{1,2,3}; NewValueImporter nvImporter = mock(NewValueImporter.class); EntryEventImpl e = createEntryEvent(region, newValue); @@ -98,7 +95,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithStringIgnoresNewValueBytes() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); String newValue = "newValue"; NewValueImporter nvImporter = mock(NewValueImporter.class); when(nvImporter.prefersNewSerialized()).thenReturn(true); @@ -114,7 +110,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithByteArrayCachedDeserializable() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); CachedDeserializable newValue = mock(CachedDeserializable.class); byte[] newValueBytes = new byte[] {1,2,3}; when(newValue.getValue()).thenReturn(newValueBytes); @@ -129,7 +124,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithStringCachedDeserializable() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); CachedDeserializable newValue = mock(CachedDeserializable.class); Object newValueObj = "newValueObj"; when(newValue.getValue()).thenReturn(newValueObj); @@ -147,7 +141,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithStringCachedDeserializablePrefersNewValueBytes() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); CachedDeserializable newValue = mock(CachedDeserializable.class); Object newValueObj = "newValueObj"; when(newValue.getValue()).thenReturn(newValueObj); @@ -165,7 +158,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithStringCachedDeserializablePrefersCachedSerializedNewValue() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); CachedDeserializable newValue = mock(CachedDeserializable.class); Object newValueObj = "newValueObj"; when(newValue.getValue()).thenReturn(newValueObj); @@ -183,7 +175,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithSerializedStoredObjectAndImporterPrefersSerialized() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject newValue = mock(StoredObject.class); when(newValue.isSerialized()).thenReturn(true); @@ -201,7 +192,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithSerializedStoredObject() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject newValue = mock(StoredObject.class); when(newValue.isSerialized()).thenReturn(true); @@ -218,7 +208,6 @@ public class EntryEventImplTest { @Test public void verifyExportNewValueWithSerializedStoredObjectAndUnretainedNewReferenceOk() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject newValue = mock(StoredObject.class); when(newValue.isSerialized()).thenReturn(true); @@ -236,7 +225,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithUnserializedStoredObject() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject oldValue = mock(StoredObject.class); byte[] oldValueBytes = new byte[]{1,2,3}; @@ -253,7 +241,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithByteArray() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); byte[] oldValue = new byte[]{1,2,3}; OldValueImporter ovImporter = mock(OldValueImporter.class); EntryEventImpl e = createEntryEvent(region, null); @@ -267,7 +254,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithStringIgnoresOldValueBytes() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); String oldValue = "oldValue"; OldValueImporter ovImporter = mock(OldValueImporter.class); when(ovImporter.prefersOldSerialized()).thenReturn(true); @@ -284,7 +270,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValuePrefersOldValueBytes() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); OldValueImporter ovImporter = mock(OldValueImporter.class); when(ovImporter.prefersOldSerialized()).thenReturn(true); EntryEventImpl e = createEntryEvent(region, null); @@ -299,7 +284,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithCacheDeserializableByteArray() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); CachedDeserializable oldValue = mock(CachedDeserializable.class); byte[] oldValueBytes = new byte[]{1,2,3}; when(oldValue.getValue()).thenReturn(oldValueBytes); @@ -315,7 +299,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithCacheDeserializableString() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); CachedDeserializable oldValue = mock(CachedDeserializable.class); Object oldValueObj = "oldValueObj"; when(oldValue.getValue()).thenReturn(oldValueObj); @@ -331,7 +314,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithCacheDeserializableOk() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); CachedDeserializable oldValue = mock(CachedDeserializable.class); Object oldValueObj = "oldValueObj"; when(oldValue.getValue()).thenReturn(oldValueObj); @@ -348,7 +330,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithSerializedStoredObjectAndImporterPrefersSerialized() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject oldValue = mock(StoredObject.class); when(oldValue.isSerialized()).thenReturn(true); @@ -367,7 +348,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithSerializedStoredObject() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject oldValue = mock(StoredObject.class); when(oldValue.isSerialized()).thenReturn(true); @@ -385,7 +365,6 @@ public class EntryEventImplTest { @Test public void verifyExportOldValueWithSerializedStoredObjectAndUnretainedOldReferenceOk() { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject oldValue = mock(StoredObject.class); when(oldValue.isSerialized()).thenReturn(true); @@ -404,7 +383,6 @@ public class EntryEventImplTest { @Test public void verifyExternalReadMethodsBlockedByRelease() throws InterruptedException { LocalRegion region = mock(LocalRegion.class); - when(region.getAttributes()).thenReturn(region); when(region.getOffHeap()).thenReturn(true); StoredObject newValue = mock(StoredObject.class); when(newValue.hasRefCount()).thenReturn(true); http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/74a994c0/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/SearchLoadAndWriteProcessorTest.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/SearchLoadAndWriteProcessorTest.java b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/SearchLoadAndWriteProcessorTest.java index 3a95ca5..ce3d59e 100644 --- a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/SearchLoadAndWriteProcessorTest.java +++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/SearchLoadAndWriteProcessorTest.java @@ -40,7 +40,6 @@ public class SearchLoadAndWriteProcessorTest { // setup SearchLoadAndWriteProcessor processor = SearchLoadAndWriteProcessor.getProcessor(); LocalRegion lr = mock(LocalRegion.class); - when(lr.getAttributes()).thenReturn(lr); when(lr.getOffHeap()).thenReturn(true); when(lr.getScope()).thenReturn(Scope.DISTRIBUTED_ACK); Object key = "key";
