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/05/18 20:03:08 UTC
[GitHub] [datasketches-memory] leerho opened a new pull request #129: Create internal2
leerho opened a new pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129
This is a major refactoring of the Memory package.
All _memory_ classes have been moved to _internal_ package. Created interface proxies for
the _memory_ package. This creates possibility to restrict access to
_internal_ package using JPMS in JDK 9+.
All tests pass and the vast majority of tests are black-box tests.
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
leerho commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r637259795
##########
File path: src/main/java/org/apache/datasketches/memory/internal/package-info.java
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * <p>This package provides high performance primitive and primitive array access to direct (native),
+ * off-heap memory and memory-mapped file resources, and consistent views into
+ * {@link java.nio.ByteBuffer}, and on-heap primitive arrays. It can be used as a more
+ * comprehensive and flexible replacement for {@link java.nio.ByteBuffer}.
+ * </p>
+ *
+ * <p>In addition, this package provides:</p>
+ *
+ * <ul><li>Two different access APIs: read-only {@link org.apache.datasketches.memory.internal.MemoryImpl} and
+ * {@link org.apache.datasketches.memory.internal.WritableMemoryImpl} for absolute offset access,
+ * and read-only {@link org.apache.datasketches.memory.internal.BufferImpl} and
+ * {@link org.apache.datasketches.memory.internal.WritableBufferImpl}
+ * for relative positional access (similar to ByteBuffer).</li>
+ *
+ * <li>Clean separation of Read-only API from Writable API, which makes writable versus read-only
+ * resources detectable at compile time.</li>
+ *
+ * <li>The conversion from Writable to read-only is just a cast, so no unnecessary objects are
+ * created. For example:
+ * <blockquote><pre>
+ * WritableMemoryImpl wMem = ...
+ * MemoryImpl mem = wMem;
+ * </pre></blockquote>
+ * </li>
+ *
+ * <li> {@link java.lang.AutoCloseable} for the external resources that require it,
+ * which enables compile-time checks for non-closed resources.</li>
+ *
+ * <li>Immediate invalidation of all downstream references of an AutoCloseable
+ * resource when that resource is closed, either manually or by the JVM.
+ * This virtually eliminates the possibility of accidentally writing into the memory space
+ * previously owned by a closed resource.</li>
+ *
+ * <li>Improved performance over the prior MemoryImpl implementation.</li>
+ *
+ * <li>Cleaner internal architecture, which will make it easier to extend in the future.</li>
+ *
+ * <li>No external dependencies, which makes it simple to install in virtually any Java environment.
+ * </li>
+ * </ul>
+ *
+ * <p>More specifically, this package provides access to four different types of resources using
+ * two different access APIs. These resources are contiguous blobs of bytes that provide at least
+ * byte-level read and write access. The four resources are:</p>
+ *
+ * <ul><li>Direct (a.k.a. Native) off-heap memory allocated by the user.</li>
+ * <li>MemoryImpl-mapped files, both writable and read-only.</li>
+ * <li>{@code ByteBuffers}, both heap-based and direct, writable and read-only.</li>
+ * <li>Heap-based primitive arrays, which can be accessed as writable or read-only.</li>
+ * </ul>
+ *
+ * <p>The two different access APIs are:</p>
+ * <ul><li><i>MemoryImpl, WritableMemoryImpl</i>: Absolute offset addressing into a resource.</li>
+ * <li><i>BufferImpl, WritableBufferImpl</i>: Position relative addressing into a resource.</li>
+ * </ul>
+ *
+ * <p>In addition, all combinations of access APIs and backing resources can be accessed via
+ * multibyte primitive methods (e.g.
+ * <i>getLong(...), getLongArray(...), putLong(...), putLongArray(...)</i>) as either
+ * {@link java.nio.ByteOrder#BIG_ENDIAN} or {@link java.nio.ByteOrder#LITTLE_ENDIAN}.</p>
+ *
+ * <p>The resources don't know or care about the access APIs, and the access
+ * APIs don't really know or care what resource they are accessing.</p>
+ *
+ * <p>An access API is joined with
+ * a resource either with a static factory method or in combination with a
+ * {@link org.apache.datasketches.memory.Handle}, which is used exclusively for resources that are
+ * external to the JVM, such as allocation of direct memory and memory-mapped files.</p>
+ *
+ * <p>The role of a Handle is to hold onto the reference of a resource that is outside the control
+ * of the JVM. The resource is obtained from the handle with {@code get()}.</p>
+ *
+ * <p>When a handle is extended for an AutoCloseable resource and then joined with an access API
+ * it becomes an <i>implementation handle</i>. There are 3 implementation handles:</p>
+ *
+ * <ul><li>{@link org.apache.datasketches.memory.MapHandle}
+ * for read-only access to a memory-mapped file</li>
+ * <li>{@link org.apache.datasketches.memory.WritableMapHandle}
+ * for writable access to a memory-mapped file</li>
+ * <li>{@link org.apache.datasketches.memory.WritableDirectHandle}
+ * for writable access to off-heap memory.</li>
+ * </ul>
+ *
+ * <p>As long as the implementation handle is valid the JVM will not attempt to close the resource.</p>
+ *
+ * <p>An implementation handle implements {@link java.lang.AutoCloseable},
+ * which also enables compile-time checks for non-closed resources. If a Handle is acquired
+ * in a try-with-resources (TWR) block, it's associated resource will be automatically closed by
+ * the JVM at the end of the block.
+ * The resource can also be explicitly closed by the user by calling {@code Handle.close()}.</p>
+ * <blockquote><pre>
+ * //Using try-with-resources block:
+ * try (WritableyMapHandle handle = WritableMemoryImpl.map(File file)) {
+ * WritableMemoryImpl wMem = handle.get();
+ * doWork(wMem) // read and write to memory mapped file.
+ * }
+ *
+ * //Using explicit close():
+ * WritableMapHandleImpl handle = WritableMemoryImpl.map(File file);
+ * WritableMemoryImpl wMem = handle.get();
+ * doWork(wMem) // read and write to memory mapped file.
+ * handle.close();
+ * </pre></blockquote>
+ *
+ * <p>Where it is desirable to pass ownership of the resource (and the {@code close()}
+ * responsibility) one can not use the TWR block. Instead:</p>
+ * <blockquote><pre>
+ * WritableMapHandleImpl handler = WritableMemoryImpl.map(File file);
+ * doWorkAndClose(handle); //passes the handle to object that closes the resource.
+ * </pre></blockquote>
+ *
+ * <p>Whatever part of your process is responsible for allocating a resource external
+ * to the JVM must be responsible for closing it or making sure it gets closed.
+ * Since only the implementation Handles implement AutoCloseable, you must not let go of the
+ * handle reference until you are done with its associated resource.</p>
+ *
+ * <p>As mentioned above, there are two ways to do this:</p>
+ * <ul><li>Use a try-with-resources block. At the end of the block, the JVM will automatically
+ * close the resource.</li>
+ *
+ * <li>If you need to pass an external resource, pass the implementation resource handle, not the
+ * access API. This means you are also passing the responsibility to close the resource.
+ * If you have different parts of your code holding references to the same handle,
+ * whichever one closes it first will make all the other resources invalid, so be careful.
+ * As long as there is at least one reference to the handle that is still valid and the resource
+ * has not been closed, the resource will remain valid. If you drop all references to all handles,
+ * the JVM will eventually close the resource, making it invalid, but it is possible that you might
+ * run out of memory first. Depending on this is a bad idea and a could be a serious,
+ * hard-to-find bug.</li>
+ * </ul>
+ *
+ *<p>Moving back and forth between <i>MemoryImpl</i> and <i>BufferImpl</i>:</p>
+ *<blockquote><pre>
+ * MemoryImpl mem = ...
+ * BufferImpl buf = mem.asBuffer();
+ * ...
+ * MemoryImpl mem2 = buf.asMemory();
+ * ...
+ * </pre></blockquote>
+ *
+ * <p>Hierarchical memory regions can be easily created:</p>
+ * <blockquote><pre>
+ * WritableMemoryImpl wMem = ...
+ * WritableMemoryImpl wReg = wMem.writableRegion(offset, length); //OR
+ * MemoryImpl reg = wMem.region(offset, length);
+ * </pre></blockquote>
+ *
+ * <p>With asserts enabled in the JVM, all methods are checked for bounds and
+ * use-after-close violations.</p>
+ *
+ * @author Lee Rhodes
+ */
+package org.apache.datasketches.memory.internal;
Review comment:
Move the docs, thanks!
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
davecromberge commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r636862050
##########
File path: src/main/java/org/apache/datasketches/memory/BaseBuffer.java
##########
@@ -143,11 +92,7 @@ public final BaseBuffer setPosition(final long position) {
* @param position the given current position.
* @return BaseBuffer
*/
- public final BaseBuffer setAndCheckPosition(final long position) {
- checkInvariants(start, position, end, capacity);
- pos = position;
- return this;
- }
+ BaseBuffer setAndCheckPosition(long position);
Review comment:
Do these variants of the interface that check arguments imply that the client typically would expect and handle a possible `IllegalArgumentException`? How does this differ in severity from an `AssertionError`?
##########
File path: src/main/java/org/apache/datasketches/memory/internal/BaseBufferImpl.java
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.datasketches.memory.internal;
+
+import org.apache.datasketches.memory.BaseBuffer;
+
+/**
+ * A new positional API. This is different from and simpler than Java BufferImpl positional approach.
+ * <ul><li>All based on longs instead of ints.</li>
+ * <li>Eliminated "mark". Rarely used and confusing with its silent side effects.</li>
+ * <li>The invariants are {@code 0 <= start <= position <= end <= capacity}.</li>
+ * <li>It always starts up as (0, 0, capacity, capacity).</li>
+ * <li>You set (start, position, end) in one call with
+ * {@link #setStartPositionEnd(long, long, long)}</li>
+ * <li>Position can be set directly or indirectly when using the positional get/put methods.
+ * <li>Added incrementPosition(long), which is much easier when you know the increment.</li>
+ * <li>This approach eliminated a number of methods and checks, and has no unseen side effects,
+ * e.g., mark being invalidated.</li>
+ * <li>Clearer method naming (IMHO).</li>
+ * </ul>
+ *
+ * @author Lee Rhodes
+ */
+public abstract class BaseBufferImpl extends BaseStateImpl implements BaseBuffer {
+ private long capacity;
+ private long start = 0;
+ private long pos = 0;
+ private long end;
+
+ //Pass-through ctor
+ BaseBufferImpl(final Object unsafeObj, final long nativeBaseOffset,
+ final long regionOffset, final long capacityBytes) {
+ super(unsafeObj, nativeBaseOffset, regionOffset, capacityBytes);
+ capacity = end = capacityBytes;
+ }
+
+ @Override
+ public final BaseBufferImpl incrementPosition(final long increment) {
+ incrementAndAssertPositionForRead(pos, increment);
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl incrementAndCheckPosition(final long increment) {
+ incrementAndCheckPositionForRead(pos, increment);
+ return this;
+ }
+
+ @Override
+ public final long getEnd() {
+ return end;
+ }
+
+ @Override
+ public final long getPosition() {
+ return pos;
+ }
+
+ @Override
+ public final long getStart() {
+ return start;
+ }
+
+ @Override
+ public final long getRemaining() {
+ return end - pos;
+ }
+
+ @Override
+ public final boolean hasRemaining() {
+ return (end - pos) > 0;
+ }
+
+ @Override
+ public final BaseBufferImpl resetPosition() {
+ pos = start;
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl setPosition(final long position) {
+ assertInvariants(start, position, end, capacity);
+ pos = position;
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl setAndCheckPosition(final long position) {
+ checkInvariants(start, position, end, capacity);
+ pos = position;
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl setStartPositionEnd(final long start, final long position,
+ final long end) {
+ assertInvariants(start, position, end, capacity);
+ this.start = start;
+ this.end = end;
+ pos = position;
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl setAndCheckStartPositionEnd(final long start, final long position,
+ final long end) {
+ checkInvariants(start, position, end, capacity);
+ this.start = start;
+ this.end = end;
+ pos = position;
+ return this;
+ }
+
+ //RESTRICTED
+ final void incrementAndAssertPositionForRead(final long position, final long increment) {
+ assertValid();
+ final long newPos = position + increment;
+ assertInvariants(start, newPos, end, capacity);
+ pos = newPos;
+ }
+
+ final void incrementAndAssertPositionForWrite(final long position, final long increment) {
+ assertValid();
+ assert !isReadOnly() : "BufferImpl is read-only.";
+ final long newPos = position + increment;
+ assertInvariants(start, newPos, end, capacity);
+ pos = newPos;
+ }
+
+ final void incrementAndCheckPositionForRead(final long position, final long increment) {
+ checkValid();
+ final long newPos = position + increment;
+ checkInvariants(start, newPos, end, capacity);
+ pos = newPos;
+ }
+
+ final void incrementAndCheckPositionForWrite(final long position, final long increment) {
+ checkValidForWrite();
+ final long newPos = position + increment;
+ checkInvariants(start, newPos, end, capacity);
+ pos = newPos;
+ }
+
+ final void checkValidForWrite() {
+ checkValid();
+ if (isReadOnly()) {
+ throw new ReadOnlyException("BufferImpl is read-only.");
+ }
+ }
Review comment:
These restricted methods are used by subclasses, should they be marked as `protected`?
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
davecromberge commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r636868991
##########
File path: src/main/java/org/apache/datasketches/memory/BaseState.java
##########
@@ -152,43 +70,30 @@ public final boolean equals(final Object that) {
* @return true if the given object has equal contents to this object in the given range of
* bytes.
*/
- public final boolean equalTo(final long thisOffsetBytes, final Object that,
- final long thatOffsetBytes, final long lengthBytes) {
- return that instanceof BaseState
- ? CompareAndCopy.equals(this, thisOffsetBytes, (BaseState) that, thatOffsetBytes, lengthBytes)
- : false;
- }
-
+ boolean equalTo(long thisOffsetBytes, Object that,
+ long thatOffsetBytes, long lengthBytes);
+
Review comment:
Minor: Typo in the documentation `eauals` should be `equals`.
##########
File path: src/main/java/org/apache/datasketches/memory/internal/package-info.java
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * <p>This package provides high performance primitive and primitive array access to direct (native),
+ * off-heap memory and memory-mapped file resources, and consistent views into
+ * {@link java.nio.ByteBuffer}, and on-heap primitive arrays. It can be used as a more
+ * comprehensive and flexible replacement for {@link java.nio.ByteBuffer}.
+ * </p>
+ *
+ * <p>In addition, this package provides:</p>
+ *
+ * <ul><li>Two different access APIs: read-only {@link org.apache.datasketches.memory.internal.MemoryImpl} and
+ * {@link org.apache.datasketches.memory.internal.WritableMemoryImpl} for absolute offset access,
+ * and read-only {@link org.apache.datasketches.memory.internal.BufferImpl} and
+ * {@link org.apache.datasketches.memory.internal.WritableBufferImpl}
+ * for relative positional access (similar to ByteBuffer).</li>
+ *
+ * <li>Clean separation of Read-only API from Writable API, which makes writable versus read-only
+ * resources detectable at compile time.</li>
+ *
+ * <li>The conversion from Writable to read-only is just a cast, so no unnecessary objects are
+ * created. For example:
+ * <blockquote><pre>
+ * WritableMemoryImpl wMem = ...
+ * MemoryImpl mem = wMem;
+ * </pre></blockquote>
+ * </li>
+ *
+ * <li> {@link java.lang.AutoCloseable} for the external resources that require it,
+ * which enables compile-time checks for non-closed resources.</li>
+ *
+ * <li>Immediate invalidation of all downstream references of an AutoCloseable
+ * resource when that resource is closed, either manually or by the JVM.
+ * This virtually eliminates the possibility of accidentally writing into the memory space
+ * previously owned by a closed resource.</li>
+ *
+ * <li>Improved performance over the prior MemoryImpl implementation.</li>
+ *
+ * <li>Cleaner internal architecture, which will make it easier to extend in the future.</li>
+ *
+ * <li>No external dependencies, which makes it simple to install in virtually any Java environment.
+ * </li>
+ * </ul>
+ *
+ * <p>More specifically, this package provides access to four different types of resources using
+ * two different access APIs. These resources are contiguous blobs of bytes that provide at least
+ * byte-level read and write access. The four resources are:</p>
+ *
+ * <ul><li>Direct (a.k.a. Native) off-heap memory allocated by the user.</li>
+ * <li>MemoryImpl-mapped files, both writable and read-only.</li>
+ * <li>{@code ByteBuffers}, both heap-based and direct, writable and read-only.</li>
+ * <li>Heap-based primitive arrays, which can be accessed as writable or read-only.</li>
+ * </ul>
+ *
+ * <p>The two different access APIs are:</p>
+ * <ul><li><i>MemoryImpl, WritableMemoryImpl</i>: Absolute offset addressing into a resource.</li>
+ * <li><i>BufferImpl, WritableBufferImpl</i>: Position relative addressing into a resource.</li>
+ * </ul>
+ *
+ * <p>In addition, all combinations of access APIs and backing resources can be accessed via
+ * multibyte primitive methods (e.g.
+ * <i>getLong(...), getLongArray(...), putLong(...), putLongArray(...)</i>) as either
+ * {@link java.nio.ByteOrder#BIG_ENDIAN} or {@link java.nio.ByteOrder#LITTLE_ENDIAN}.</p>
+ *
+ * <p>The resources don't know or care about the access APIs, and the access
+ * APIs don't really know or care what resource they are accessing.</p>
+ *
+ * <p>An access API is joined with
+ * a resource either with a static factory method or in combination with a
+ * {@link org.apache.datasketches.memory.Handle}, which is used exclusively for resources that are
+ * external to the JVM, such as allocation of direct memory and memory-mapped files.</p>
+ *
+ * <p>The role of a Handle is to hold onto the reference of a resource that is outside the control
+ * of the JVM. The resource is obtained from the handle with {@code get()}.</p>
+ *
+ * <p>When a handle is extended for an AutoCloseable resource and then joined with an access API
+ * it becomes an <i>implementation handle</i>. There are 3 implementation handles:</p>
+ *
+ * <ul><li>{@link org.apache.datasketches.memory.MapHandle}
+ * for read-only access to a memory-mapped file</li>
+ * <li>{@link org.apache.datasketches.memory.WritableMapHandle}
+ * for writable access to a memory-mapped file</li>
+ * <li>{@link org.apache.datasketches.memory.WritableDirectHandle}
+ * for writable access to off-heap memory.</li>
+ * </ul>
+ *
+ * <p>As long as the implementation handle is valid the JVM will not attempt to close the resource.</p>
+ *
+ * <p>An implementation handle implements {@link java.lang.AutoCloseable},
+ * which also enables compile-time checks for non-closed resources. If a Handle is acquired
+ * in a try-with-resources (TWR) block, it's associated resource will be automatically closed by
+ * the JVM at the end of the block.
+ * The resource can also be explicitly closed by the user by calling {@code Handle.close()}.</p>
+ * <blockquote><pre>
+ * //Using try-with-resources block:
+ * try (WritableyMapHandle handle = WritableMemoryImpl.map(File file)) {
+ * WritableMemoryImpl wMem = handle.get();
+ * doWork(wMem) // read and write to memory mapped file.
+ * }
+ *
+ * //Using explicit close():
+ * WritableMapHandleImpl handle = WritableMemoryImpl.map(File file);
+ * WritableMemoryImpl wMem = handle.get();
+ * doWork(wMem) // read and write to memory mapped file.
+ * handle.close();
+ * </pre></blockquote>
+ *
+ * <p>Where it is desirable to pass ownership of the resource (and the {@code close()}
+ * responsibility) one can not use the TWR block. Instead:</p>
+ * <blockquote><pre>
+ * WritableMapHandleImpl handler = WritableMemoryImpl.map(File file);
+ * doWorkAndClose(handle); //passes the handle to object that closes the resource.
+ * </pre></blockquote>
+ *
+ * <p>Whatever part of your process is responsible for allocating a resource external
+ * to the JVM must be responsible for closing it or making sure it gets closed.
+ * Since only the implementation Handles implement AutoCloseable, you must not let go of the
+ * handle reference until you are done with its associated resource.</p>
+ *
+ * <p>As mentioned above, there are two ways to do this:</p>
+ * <ul><li>Use a try-with-resources block. At the end of the block, the JVM will automatically
+ * close the resource.</li>
+ *
+ * <li>If you need to pass an external resource, pass the implementation resource handle, not the
+ * access API. This means you are also passing the responsibility to close the resource.
+ * If you have different parts of your code holding references to the same handle,
+ * whichever one closes it first will make all the other resources invalid, so be careful.
+ * As long as there is at least one reference to the handle that is still valid and the resource
+ * has not been closed, the resource will remain valid. If you drop all references to all handles,
+ * the JVM will eventually close the resource, making it invalid, but it is possible that you might
+ * run out of memory first. Depending on this is a bad idea and a could be a serious,
+ * hard-to-find bug.</li>
+ * </ul>
+ *
+ *<p>Moving back and forth between <i>MemoryImpl</i> and <i>BufferImpl</i>:</p>
+ *<blockquote><pre>
+ * MemoryImpl mem = ...
+ * BufferImpl buf = mem.asBuffer();
+ * ...
+ * MemoryImpl mem2 = buf.asMemory();
+ * ...
+ * </pre></blockquote>
+ *
+ * <p>Hierarchical memory regions can be easily created:</p>
+ * <blockquote><pre>
+ * WritableMemoryImpl wMem = ...
+ * WritableMemoryImpl wReg = wMem.writableRegion(offset, length); //OR
+ * MemoryImpl reg = wMem.region(offset, length);
+ * </pre></blockquote>
+ *
+ * <p>With asserts enabled in the JVM, all methods are checked for bounds and
+ * use-after-close violations.</p>
+ *
+ * @author Lee Rhodes
+ */
+package org.apache.datasketches.memory.internal;
Review comment:
There is another package info file in the root package that does not contain this documentation - did you intend to put this version in the internal package-info.java?
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
davecromberge commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r637043441
##########
File path: src/main/java/org/apache/datasketches/memory/Memory.java
##########
@@ -17,36 +17,23 @@
* under the License.
*/
-package org.apache.datasketches.memory;
-import static org.apache.datasketches.memory.Util.negativeCheck;
-import static org.apache.datasketches.memory.Util.nullCheck;
-import static org.apache.datasketches.memory.Util.zeroCheck;
+package org.apache.datasketches.memory;
import java.io.File;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.channels.WritableByteChannel;
-/**
- * Provides read-only primitive and primitive array methods to any of the four resources
- * mentioned in the package level documentation.
- *
- * @author Roman Leventov
- * @author Lee Rhodes
- *
- * @see org.apache.datasketches.memory
- */
-public abstract class Memory extends BaseState {
+import org.apache.datasketches.memory.internal.MemoryImpl;
+import org.apache.datasketches.memory.internal.Utf8CodingException;
Review comment:
Since this exception may be thrown whilst using a public method, it might be necessary to move this into the root package - otherwise a developer may not be able to explicitly catch this exception directly.
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
leerho commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r637256659
##########
File path: src/main/java/org/apache/datasketches/memory/BaseBuffer.java
##########
@@ -143,11 +92,7 @@ public final BaseBuffer setPosition(final long position) {
* @param position the given current position.
* @return BaseBuffer
*/
- public final BaseBuffer setAndCheckPosition(final long position) {
- checkInvariants(start, position, end, capacity);
- pos = position;
- return this;
- }
+ BaseBuffer setAndCheckPosition(long position);
Review comment:
There are two types of checks: "check(blah)" and "assert(blah)". Asserts only apply when running test or if the JVM flag -ea is set. The check methods will throw an exception. Note that checks are primarily used when copying arrays or blocks. but when just setting primitives, I use asserts. The primary reason is speed during runtime, otherwise, the cost of the check would overwhelm the actual operation. If the user is concerned about memory violations, all they have to do is set -ea.
##########
File path: src/main/java/org/apache/datasketches/memory/internal/BaseBufferImpl.java
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.datasketches.memory.internal;
+
+import org.apache.datasketches.memory.BaseBuffer;
+
+/**
+ * A new positional API. This is different from and simpler than Java BufferImpl positional approach.
+ * <ul><li>All based on longs instead of ints.</li>
+ * <li>Eliminated "mark". Rarely used and confusing with its silent side effects.</li>
+ * <li>The invariants are {@code 0 <= start <= position <= end <= capacity}.</li>
+ * <li>It always starts up as (0, 0, capacity, capacity).</li>
+ * <li>You set (start, position, end) in one call with
+ * {@link #setStartPositionEnd(long, long, long)}</li>
+ * <li>Position can be set directly or indirectly when using the positional get/put methods.
+ * <li>Added incrementPosition(long), which is much easier when you know the increment.</li>
+ * <li>This approach eliminated a number of methods and checks, and has no unseen side effects,
+ * e.g., mark being invalidated.</li>
+ * <li>Clearer method naming (IMHO).</li>
+ * </ul>
+ *
+ * @author Lee Rhodes
+ */
+public abstract class BaseBufferImpl extends BaseStateImpl implements BaseBuffer {
+ private long capacity;
+ private long start = 0;
+ private long pos = 0;
+ private long end;
+
+ //Pass-through ctor
+ BaseBufferImpl(final Object unsafeObj, final long nativeBaseOffset,
+ final long regionOffset, final long capacityBytes) {
+ super(unsafeObj, nativeBaseOffset, regionOffset, capacityBytes);
+ capacity = end = capacityBytes;
+ }
+
+ @Override
+ public final BaseBufferImpl incrementPosition(final long increment) {
+ incrementAndAssertPositionForRead(pos, increment);
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl incrementAndCheckPosition(final long increment) {
+ incrementAndCheckPositionForRead(pos, increment);
+ return this;
+ }
+
+ @Override
+ public final long getEnd() {
+ return end;
+ }
+
+ @Override
+ public final long getPosition() {
+ return pos;
+ }
+
+ @Override
+ public final long getStart() {
+ return start;
+ }
+
+ @Override
+ public final long getRemaining() {
+ return end - pos;
+ }
+
+ @Override
+ public final boolean hasRemaining() {
+ return (end - pos) > 0;
+ }
+
+ @Override
+ public final BaseBufferImpl resetPosition() {
+ pos = start;
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl setPosition(final long position) {
+ assertInvariants(start, position, end, capacity);
+ pos = position;
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl setAndCheckPosition(final long position) {
+ checkInvariants(start, position, end, capacity);
+ pos = position;
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl setStartPositionEnd(final long start, final long position,
+ final long end) {
+ assertInvariants(start, position, end, capacity);
+ this.start = start;
+ this.end = end;
+ pos = position;
+ return this;
+ }
+
+ @Override
+ public final BaseBufferImpl setAndCheckStartPositionEnd(final long start, final long position,
+ final long end) {
+ checkInvariants(start, position, end, capacity);
+ this.start = start;
+ this.end = end;
+ pos = position;
+ return this;
+ }
+
+ //RESTRICTED
+ final void incrementAndAssertPositionForRead(final long position, final long increment) {
+ assertValid();
+ final long newPos = position + increment;
+ assertInvariants(start, newPos, end, capacity);
+ pos = newPos;
+ }
+
+ final void incrementAndAssertPositionForWrite(final long position, final long increment) {
+ assertValid();
+ assert !isReadOnly() : "BufferImpl is read-only.";
+ final long newPos = position + increment;
+ assertInvariants(start, newPos, end, capacity);
+ pos = newPos;
+ }
+
+ final void incrementAndCheckPositionForRead(final long position, final long increment) {
+ checkValid();
+ final long newPos = position + increment;
+ checkInvariants(start, newPos, end, capacity);
+ pos = newPos;
+ }
+
+ final void incrementAndCheckPositionForWrite(final long position, final long increment) {
+ checkValidForWrite();
+ final long newPos = position + increment;
+ checkInvariants(start, newPos, end, capacity);
+ pos = newPos;
+ }
+
+ final void checkValidForWrite() {
+ checkValid();
+ if (isReadOnly()) {
+ throw new ReadOnlyException("BufferImpl is read-only.");
+ }
+ }
Review comment:
When subclasses are in the same package, the subclasses can access via package-private. Keeping these package-private prevents users from attempting to subclass from another package. Thus, package-private is more restrictive than protected.
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
davecromberge commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r637785280
##########
File path: src/main/java/org/apache/datasketches/memory/BaseBuffer.java
##########
@@ -143,11 +92,7 @@ public final BaseBuffer setPosition(final long position) {
* @param position the given current position.
* @return BaseBuffer
*/
- public final BaseBuffer setAndCheckPosition(final long position) {
- checkInvariants(start, position, end, capacity);
- pos = position;
- return this;
- }
+ BaseBuffer setAndCheckPosition(long position);
Review comment:
Thanks for the detailed answer - this makes more sense to me now.
--
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.
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 pull request #129: Create internal2
Posted by GitBox <gi...@apache.org>.
davecromberge commented on pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#issuecomment-850406407
@leerho it looks like all the issues I raised have been addressed - PR looks good 👍
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
leerho commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r637257594
##########
File path: src/main/java/org/apache/datasketches/memory/BaseState.java
##########
@@ -152,43 +70,30 @@ public final boolean equals(final Object that) {
* @return true if the given object has equal contents to this object in the given range of
* bytes.
*/
- public final boolean equalTo(final long thisOffsetBytes, final Object that,
- final long thatOffsetBytes, final long lengthBytes) {
- return that instanceof BaseState
- ? CompareAndCopy.equals(this, thisOffsetBytes, (BaseState) that, thatOffsetBytes, lengthBytes)
- : false;
- }
-
+ boolean equalTo(long thisOffsetBytes, Object that,
+ long thatOffsetBytes, long lengthBytes);
+
Review comment:
Thanks.
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
leerho merged pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129
--
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.
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 #129: Create internal2
Posted by GitBox <gi...@apache.org>.
leerho commented on a change in pull request #129:
URL: https://github.com/apache/datasketches-memory/pull/129#discussion_r637261846
##########
File path: src/main/java/org/apache/datasketches/memory/Memory.java
##########
@@ -17,36 +17,23 @@
* under the License.
*/
-package org.apache.datasketches.memory;
-import static org.apache.datasketches.memory.Util.negativeCheck;
-import static org.apache.datasketches.memory.Util.nullCheck;
-import static org.apache.datasketches.memory.Util.zeroCheck;
+package org.apache.datasketches.memory;
import java.io.File;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.channels.WritableByteChannel;
-/**
- * Provides read-only primitive and primitive array methods to any of the four resources
- * mentioned in the package level documentation.
- *
- * @author Roman Leventov
- * @author Lee Rhodes
- *
- * @see org.apache.datasketches.memory
- */
-public abstract class Memory extends BaseState {
+import org.apache.datasketches.memory.internal.MemoryImpl;
+import org.apache.datasketches.memory.internal.Utf8CodingException;
Review comment:
Good catch! Thanks!
--
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.
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