You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2021/08/16 03:16:55 UTC
[GitHub] [datasketches-memory] leerho opened a new pull request #139: Major refactor and fix
leerho opened a new pull request #139:
URL: https://github.com/apache/datasketches-memory/pull/139
This fix adds the capability of the RequestMemoryServer to Memory on-heap, wrapped ByteBuffer as well as off-heap.
I took the opportunity to do some major refactoring and eliminated 2 layers of the abstract hierarch, which should make the code easier to follow.
I also applied more rigorous argument checking for all the public APIs in the -java8 -memory package.
Unfortunately, these changes hit a very large number of classes. But some of that is due to the heavy reliance on virtual methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-memory] leerho commented on a change in pull request #139: Major refactor and fix
Posted by GitBox <gi...@apache.org>.
leerho commented on a change in pull request #139:
URL: https://github.com/apache/datasketches-memory/pull/139#discussion_r689664744
##########
File path: datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/WritableBuffer.java
##########
@@ -22,41 +22,47 @@
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
+import java.util.Objects;
-import org.apache.datasketches.memory.internal.WritableBufferImpl;
+import org.apache.datasketches.memory.internal.BaseWritableBufferImpl;
+import org.apache.datasketches.memory.internal.Util;
public interface WritableBuffer extends Buffer {
//BYTE BUFFER
/**
- * Accesses the given ByteBuffer for write operations. The returned WritableBuffer object has
- * the same byte order, as the given ByteBuffer, unless the capacity of the given ByteBuffer is
- * zero, then byte order of the returned WritableBuffer object, as well as backing storage and
- * read-only status are unspecified.
- * @param byteBuf the given ByteBuffer, must not be null.
- * @return a new WritableBuffer for write operations on the given ByteBuffer.
+ * Accesses the given <i>ByteBuffer</i> for write operations. The returned <i>WritableBuffer</i> object has
+ * the same byte order, as the given <i>ByteBuffer</i>.
+ * @param byteBuf the given ByteBuffer. It must be non-null and with capacity >= 0.
+ * @return a new <i>WritableBuffer</i> for write operations on the given <i>ByteBuffer</i>.
*/
static WritableBuffer writableWrap(ByteBuffer byteBuf) {
- return WritableBufferImpl.writableWrap(byteBuf);
+ return writableWrap(byteBuf, byteBuf.order(), defaultMemReqSvr);
}
/**
- * Accesses the given ByteBuffer for write operations. The returned WritableBuffer object has
- * the given byte order, ignoring the byte order of the given ByteBuffer. If the capacity of
- * the given ByteBuffer is zero the byte order of the returned WritableBuffer object
- * (as well as backing storage) is unspecified.
- * @param byteBuf the given ByteBuffer, must not be null
- * @param byteOrder the byte order to be used, which may be independent of the byte order
- * state of the given ByteBuffer
- * @param memReqSvr A user-specified MemoryRequestServer.
- * This is a callback mechanism for a user client to request a larger Memory.
- * @return a new WritableBuffer for write operations on the given ByteBuffer.
+ * Accesses the given <i>ByteBuffer</i> for write operations. The returned <i>WritableBuffer</i> object has
+ * the given byte order, ignoring the byte order of the given <i>ByteBuffer</i> for future writes and following reads.
+ * However, this does not change the byte order of data already in the <i>ByteBuffer</i>.
+ * @param byteBuf the given ByteBuffer. It must be non-null and with capacity >= 0.
+ * @param byteOrder the byte order to be used.
+ * @param memReqSvr A user-specified <i>MemoryRequestServer</i>, which must not be null.
+ * This is a callback mechanism for a user client to request a larger <i>WritableBuffer</i>.
+ * @return a new <i>WritableBuffer</i> for write operations on the given <i>ByteBuffer</i>.
*/
static WritableBuffer writableWrap(ByteBuffer byteBuf, ByteOrder byteOrder, MemoryRequestServer memReqSvr) {
- MemoryRequestServer mReqSvr = (memReqSvr == null) ? defaultMemReqSvr : memReqSvr;
- return WritableBufferImpl.writableWrap(byteBuf, byteOrder, mReqSvr);
+ Objects.requireNonNull(byteBuf, "ByteBuffer 'byteBuf' must not be null");
+ Objects.requireNonNull(byteOrder, "ByteOrder 'byteOrder' must not be null");
+ Util.negativeCheck(byteBuf.capacity(), "byteBuf.capacity");
+ if (byteBuf.isReadOnly()) {
+ throw new ReadOnlyException("Cannot create a WritableBuffer from a ReadOnly ByteBuffer.");
+ }
+ return BaseWritableBufferImpl.wrapByteBuffer(byteBuf, false, byteOrder, memReqSvr);
}
Review comment:
Wherever the documentation says that, it needs to be changed. We had a long discussion about whether the Default memReqSvr should even be there at all. As it is now the DefaultMRSvr is set to null. This forces the user to specifically implement what they want the MRSvr to do. The default serves as an example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-memory] leerho merged pull request #139: Major refactor and fix
Posted by GitBox <gi...@apache.org>.
leerho merged pull request #139:
URL: https://github.com/apache/datasketches-memory/pull/139
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-memory] davecromberge commented on a change in pull request #139: Major refactor and fix
Posted by GitBox <gi...@apache.org>.
davecromberge commented on a change in pull request #139:
URL: https://github.com/apache/datasketches-memory/pull/139#discussion_r689601894
##########
File path: datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/WritableBuffer.java
##########
@@ -22,41 +22,47 @@
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
+import java.util.Objects;
-import org.apache.datasketches.memory.internal.WritableBufferImpl;
+import org.apache.datasketches.memory.internal.BaseWritableBufferImpl;
+import org.apache.datasketches.memory.internal.Util;
public interface WritableBuffer extends Buffer {
//BYTE BUFFER
/**
- * Accesses the given ByteBuffer for write operations. The returned WritableBuffer object has
- * the same byte order, as the given ByteBuffer, unless the capacity of the given ByteBuffer is
- * zero, then byte order of the returned WritableBuffer object, as well as backing storage and
- * read-only status are unspecified.
- * @param byteBuf the given ByteBuffer, must not be null.
- * @return a new WritableBuffer for write operations on the given ByteBuffer.
+ * Accesses the given <i>ByteBuffer</i> for write operations. The returned <i>WritableBuffer</i> object has
+ * the same byte order, as the given <i>ByteBuffer</i>.
+ * @param byteBuf the given ByteBuffer. It must be non-null and with capacity >= 0.
+ * @return a new <i>WritableBuffer</i> for write operations on the given <i>ByteBuffer</i>.
*/
static WritableBuffer writableWrap(ByteBuffer byteBuf) {
- return WritableBufferImpl.writableWrap(byteBuf);
+ return writableWrap(byteBuf, byteBuf.order(), defaultMemReqSvr);
}
/**
- * Accesses the given ByteBuffer for write operations. The returned WritableBuffer object has
- * the given byte order, ignoring the byte order of the given ByteBuffer. If the capacity of
- * the given ByteBuffer is zero the byte order of the returned WritableBuffer object
- * (as well as backing storage) is unspecified.
- * @param byteBuf the given ByteBuffer, must not be null
- * @param byteOrder the byte order to be used, which may be independent of the byte order
- * state of the given ByteBuffer
- * @param memReqSvr A user-specified MemoryRequestServer.
- * This is a callback mechanism for a user client to request a larger Memory.
- * @return a new WritableBuffer for write operations on the given ByteBuffer.
+ * Accesses the given <i>ByteBuffer</i> for write operations. The returned <i>WritableBuffer</i> object has
+ * the given byte order, ignoring the byte order of the given <i>ByteBuffer</i> for future writes and following reads.
+ * However, this does not change the byte order of data already in the <i>ByteBuffer</i>.
+ * @param byteBuf the given ByteBuffer. It must be non-null and with capacity >= 0.
+ * @param byteOrder the byte order to be used.
+ * @param memReqSvr A user-specified <i>MemoryRequestServer</i>, which must not be null.
+ * This is a callback mechanism for a user client to request a larger <i>WritableBuffer</i>.
+ * @return a new <i>WritableBuffer</i> for write operations on the given <i>ByteBuffer</i>.
*/
static WritableBuffer writableWrap(ByteBuffer byteBuf, ByteOrder byteOrder, MemoryRequestServer memReqSvr) {
- MemoryRequestServer mReqSvr = (memReqSvr == null) ? defaultMemReqSvr : memReqSvr;
- return WritableBufferImpl.writableWrap(byteBuf, byteOrder, mReqSvr);
+ Objects.requireNonNull(byteBuf, "ByteBuffer 'byteBuf' must not be null");
+ Objects.requireNonNull(byteOrder, "ByteOrder 'byteOrder' must not be null");
+ Util.negativeCheck(byteBuf.capacity(), "byteBuf.capacity");
+ if (byteBuf.isReadOnly()) {
+ throw new ReadOnlyException("Cannot create a WritableBuffer from a ReadOnly ByteBuffer.");
+ }
+ return BaseWritableBufferImpl.wrapByteBuffer(byteBuf, false, byteOrder, memReqSvr);
}
Review comment:
The documentation specifies that the memReqSvr should not be null, this this be checked in the pre-conditions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org