From 90c501bfe57031eb32a0a3a2d215a07f3a1fe633 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Wed, 25 Nov 2020 14:16:50 -0800 Subject: [PATCH 1/3] Don't call getCapacity() when asserting read and write --- src/main/java/org/apache/datasketches/memory/BaseState.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/datasketches/memory/BaseState.java b/src/main/java/org/apache/datasketches/memory/BaseState.java index 0f430da5..5375b10d 100644 --- a/src/main/java/org/apache/datasketches/memory/BaseState.java +++ b/src/main/java/org/apache/datasketches/memory/BaseState.java @@ -371,12 +371,12 @@ final void checkValid() { final void assertValidAndBoundsForRead(final long offsetBytes, final long lengthBytes) { assertValid(); - assertBounds(offsetBytes, lengthBytes, getCapacity()); + assertBounds(offsetBytes, lengthBytes, capacityBytes_); } final void assertValidAndBoundsForWrite(final long offsetBytes, final long lengthBytes) { assertValid(); - assertBounds(offsetBytes, lengthBytes, getCapacity()); + assertBounds(offsetBytes, lengthBytes, capacityBytes_); assert !isReadOnly() : "Memory is read-only."; } From 17031615ddf37a0cd4dd3dddfb476732653cca03 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Wed, 25 Nov 2020 14:52:57 -0800 Subject: [PATCH 2/3] add some comments --- src/main/java/org/apache/datasketches/memory/BaseState.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/org/apache/datasketches/memory/BaseState.java b/src/main/java/org/apache/datasketches/memory/BaseState.java index 5375b10d..0ff1c033 100644 --- a/src/main/java/org/apache/datasketches/memory/BaseState.java +++ b/src/main/java/org/apache/datasketches/memory/BaseState.java @@ -371,11 +371,17 @@ final void checkValid() { final void assertValidAndBoundsForRead(final long offsetBytes, final long lengthBytes) { assertValid(); + // capacityBytes_ is intentionally read directly instead of calling getCapacity() + // because the later can make JVM to not inline the assert code path (and entirely remove it) + // even though it does nothing in production code path. assertBounds(offsetBytes, lengthBytes, capacityBytes_); } final void assertValidAndBoundsForWrite(final long offsetBytes, final long lengthBytes) { assertValid(); + // capacityBytes_ is intentionally read directly instead of calling getCapacity() + // because the later can make JVM to not inline the assert code path (and entirely remove it) + // even though it does nothing in production code path. assertBounds(offsetBytes, lengthBytes, capacityBytes_); assert !isReadOnly() : "Memory is read-only."; } From d866a0d63547ccea82fa8f318fb71250df09c482 Mon Sep 17 00:00:00 2001 From: Lee Rhodes Date: Tue, 1 Dec 2020 13:43:19 -0800 Subject: [PATCH 3/3] Extended the PR by Jihoonson to other places that could also benefit from the direct call. --- .../apache/datasketches/memory/BaseState.java | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/apache/datasketches/memory/BaseState.java b/src/main/java/org/apache/datasketches/memory/BaseState.java index 0ff1c033..9f6d9849 100644 --- a/src/main/java/org/apache/datasketches/memory/BaseState.java +++ b/src/main/java/org/apache/datasketches/memory/BaseState.java @@ -38,7 +38,7 @@ abstract class BaseState { //Byte Order related static final ByteOrder nativeByteOrder = ByteOrder.nativeOrder(); - static final ByteOrder nonNativeByteOrder = (nativeByteOrder == ByteOrder.LITTLE_ENDIAN) + static final ByteOrder nonNativeByteOrder = nativeByteOrder == ByteOrder.LITTLE_ENDIAN ? ByteOrder.BIG_ENDIAN : ByteOrder.LITTLE_ENDIAN; //Monitoring @@ -92,7 +92,7 @@ abstract class BaseState { BaseState(final Object unsafeObj, final long nativeBaseOffset, final long regionOffset, final long capacityBytes) { capacityBytes_ = capacityBytes; - cumBaseOffset_ = regionOffset + ((unsafeObj == null) + cumBaseOffset_ = regionOffset + (unsafeObj == null ? nativeBaseOffset : UnsafeUtil.getArrayBaseOffset(unsafeObj.getClass())); } @@ -125,7 +125,7 @@ static boolean isNativeByteOrder(final ByteOrder byteOrder) { if (byteOrder == null) { throw new IllegalArgumentException("ByteOrder parameter cannot be null."); } - return (BaseState.nativeByteOrder == byteOrder); + return BaseState.nativeByteOrder == byteOrder; } /** @@ -137,7 +137,7 @@ static boolean isNativeByteOrder(final ByteOrder byteOrder) { */ public final boolean isByteOrderCompatible(final ByteOrder byteOrder) { final ByteOrder typeBO = getTypeByteOrder(); - return ((typeBO == getNativeByteOrder()) && (typeBO == byteOrder)); + return typeBO == getNativeByteOrder() && typeBO == byteOrder; } /** @@ -148,8 +148,8 @@ public final boolean isByteOrderCompatible(final ByteOrder byteOrder) { @Override public final boolean equals(final Object that) { if (this == that) { return true; } - return (that instanceof BaseState) - ? CompareAndCopy.equals(this, ((BaseState) that)) + return that instanceof BaseState + ? CompareAndCopy.equals(this, (BaseState) that) : false; } @@ -166,7 +166,7 @@ public final boolean equals(final Object that) { */ public final boolean equalTo(final long thisOffsetBytes, final Object that, final long thatOffsetBytes, final long lengthBytes) { - return (that instanceof BaseState) + return that instanceof BaseState ? CompareAndCopy.equals(this, thisOffsetBytes, (BaseState) that, thatOffsetBytes, lengthBytes) : false; } @@ -231,7 +231,7 @@ long getNativeBaseOffset() { */ public final long getRegionOffset() { final Object unsafeObj = getUnsafeObject(); - return (unsafeObj == null) + return unsafeObj == null ? cumBaseOffset_ - getNativeBaseOffset() : cumBaseOffset_ - UnsafeUtil.getArrayBaseOffset(unsafeObj.getClass()); } @@ -281,7 +281,7 @@ public final boolean hasArray() { */ @Override public final int hashCode() { - return (int) xxHash64(0, getCapacity(), 0); + return (int) xxHash64(0, capacityBytes_, 0); //xxHash64() calls checkValid() } /** @@ -297,7 +297,7 @@ public final int hashCode() { */ public final long xxHash64(final long offsetBytes, final long lengthBytes, final long seed) { checkValid(); - return XxHash64.hash(getUnsafeObject(), getCumulativeOffset() + offsetBytes, lengthBytes, seed); + return XxHash64.hash(getUnsafeObject(), cumBaseOffset_ + offsetBytes, lengthBytes, seed); } /** @@ -342,10 +342,10 @@ public final boolean isSameResource(final Object that) { that1.checkValid(); if (this == that1) { return true; } - return (getCumulativeOffset() == that1.getCumulativeOffset()) - && (getCapacity() == that1.getCapacity()) - && (getUnsafeObject() == that1.getUnsafeObject()) - && (getByteBuffer() == that1.getByteBuffer()); + return cumBaseOffset_ == that1.cumBaseOffset_ + && capacityBytes_ == that1.capacityBytes_ + && getUnsafeObject() == that1.getUnsafeObject() + && getByteBuffer() == that1.getByteBuffer(); } /** @@ -395,12 +395,14 @@ final void assertValidAndBoundsForWrite(final long offsetBytes, final long lengt */ public final void checkValidAndBounds(final long offsetBytes, final long lengthBytes) { checkValid(); - checkBounds(offsetBytes, lengthBytes, getCapacity()); + //read capacityBytes_ directly to eliminate extra checkValid() call + checkBounds(offsetBytes, lengthBytes, capacityBytes_); } final void checkValidAndBoundsForWrite(final long offsetBytes, final long lengthBytes) { checkValid(); - checkBounds(offsetBytes, lengthBytes, getCapacity()); + //read capacityBytes_ directly to eliminate extra checkValid() call + checkBounds(offsetBytes, lengthBytes, capacityBytes_); if (isReadOnly()) { throw new ReadOnlyException("Memory is read-only."); } @@ -428,19 +430,19 @@ final boolean isNonNativeType() { } final boolean isHeapType() { - return ((getTypeId() >>> 3) & 3) == 0; + return (getTypeId() >>> 3 & 3) == 0; } final boolean isDirectType() { - return ((getTypeId() >>> 3) & 3) == 1; + return (getTypeId() >>> 3 & 3) == 1; } final boolean isMapType() { - return ((getTypeId() >>> 3) & 3) == 2; + return (getTypeId() >>> 3 & 3) == 2; } final boolean isBBType() { - return ((getTypeId() >>> 3) & 3) == 3; + return (getTypeId() >>> 3 & 3) == 3; } //MONITORING @@ -527,10 +529,10 @@ static final String toHex(final BaseState state, final String preamble, final lo uObjHeader = UnsafeUtil.getArrayBaseOffset(uObj.getClass()); } final ByteBuffer bb = state.getByteBuffer(); - final String bbStr = (bb == null) ? "null" + final String bbStr = bb == null ? "null" : bb.getClass().getSimpleName() + ", " + (bb.hashCode() & 0XFFFFFFFFL); final MemoryRequestServer memReqSvr = state.getMemoryRequestServer(); - final String memReqStr = (memReqSvr != null) + final String memReqStr = memReqSvr != null ? memReqSvr.getClass().getSimpleName() + ", " + (memReqSvr.hashCode() & 0XFFFFFFFFL) : "null"; final long cumBaseOffset = state.getCumulativeOffset(); @@ -552,7 +554,7 @@ static final String toHex(final BaseState state, final String preamble, final lo for (long i = 0; i < lengthBytes; i++) { final int b = unsafe.getByte(uObj, cumBaseOffset + offsetBytes + i) & 0XFF; - if ((i % 8) == 0) { //row header + if (i % 8 == 0) { //row header sb.append(String.format("%n%20s: ", offsetBytes + i)); } sb.append(String.format("%02x ", b));