Prepare for java 8,11 3.0.0 #190

leerho · 2024-06-11T21:59:05Z

Prepares the java 8,11 code-base for a 3.0.0 release.

will look like with java 17 FFM Panama

leerho · 2024-06-20T03:21:56Z

This is ready for review with a target version of DS-memory 3.0.0. There are quite a few changes -- all with the intent of moving to a java 8,11 API that resembles what the API will be in Java 17. It is not perfect, but a lot closer that what it was.
DS-memory 4.0.0 will be the version implemented using FFM-Panama. It is also pretty much ready to go.

I need to release this before I can release DS-java 6.1.0 which will still be java 8,11, and depend on this.
The DS-Java 6.1.0 is ready to go as soon as this gets done.
The DS-Java 7.0.0 (FFM-panama depending on ds-memory 4.0.0 is also pretty much ready to go.

Improved the toString methods. Simplified BaseBuffer API.

And BaseBufferImpl to PositionalImpl. Cleaned up some duplicate methods. Improved some javadocs.

Corrected some tests that were referring to the wrong exception.

jmalkin

There are a few very, very minor things.

We should try to move all the <i> stuff to <code> but that's not a priority for this PR.

datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/Buffer.java

jmalkin · 2024-06-28T23:05:53Z

datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/MemoryRequestServer.java

+   * determine the byte order of the returned WritableMemory and other properties.
   * @param currentWritableMemory the current writableMemory of the client. It must be non-null.
-   * @param capacityBytes The capacity being requested. It must be &ge; 0.
+   * @param newCapacityBytes The capacity being requested. It must be &gt; the capacity of the currentWritableMemory.


Why does it need to be larger?

This is the MemoryRequestServer and the intent is to request a larger memory. It doesn't make sense to request a memory space smaller than what you already have.

So we don't support the pattern of getting a smaller amount of additional memory while keeping the current memory?

Not with this interface, and never have. If you want a separate smaller memory, use the regular allocate methods for that. This is explicitly for the case where a sketch runs out of its allocated memory

datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/Memory.java

jmalkin · 2024-06-28T23:40:05Z

datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/Memory.java

-   * {@link #map(File, long, long, ByteOrder) map(file, 0, file.length(), ByteOrder.nativeOrder())}.
-   * @param file the given file to map. It must be non-null, length &ge; 0, and readable.
+   * {@link #map(File, long, long, ByteOrder)
+   * map(file, 0, file.length(), scope, ByteOrder.nativeOrder())}.


link has 4 parameters, example has 5. Make sure it points to the right place.

Yep. this is a javadoc comment bug and it occurred in 3 places. Will fix. I also searched the entire ds-memory repo for this bug and it is clean.

jmalkin · 2024-06-28T23:48:20Z

datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/Memory.java

-   * @param capacityBytes the size of the mapped memory. It must not be negative.
-   * @param byteOrder the byte order to be used for the mapped memory. It must be non-null.
+   * @param file the given file to map. It must be non-null, readable and length &ge; 0.
+   * @param fileOffsetBytes the position in the given file in bytes. It must &ge; 0.


must be ≥ 0

currently missing the word "be"

jmalkin · 2024-07-01T22:07:50Z

datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/WritableMemory.java

-   * @param capacityBytes the size of the desired memory in bytes. It must be &ge; 0.
-   * @return WritableMemory for this off-heap resource.
+   * @param capacityBytes the size of the desired memory in bytes.
+   * @return WritableMemory for this off-heap, native resource.


I think it's worth specifying "byte order" explicitly so it's clear what "native" refers to.

In this specific context I meant native=off-heap, but it is equally valid to mean native byte-order! This double use of "native" bugs me. But the double use also exists in Panama. I'm not sure what to do about this. I could go through the entire repo and check for all uses of "native" and make sure the context is clear.

jmalkin · 2024-07-02T00:33:18Z

...ketches-memory-java8/src/main/java/org/apache/datasketches/memory/internal/ResourceImpl.java

-    sb.append("MemReq, hashCode    : ").append(memReqStr).append(LS);
-    sb.append("Valid               : ").append(state.isValid()).append(LS);
+    sb.append("MemReqSvr, hashCode : ").append(memReqStr).append(LS);
+    sb.append("Valid               : ").append(state.isAlive()).append(LS);


Should the text be changed to Alive to reflect the method name?

Yes, and I will check the whole repo for this substitution.

jmalkin · 2024-07-02T01:58:11Z

datasketches-memory-java8/src/test/java/org/apache/datasketches/memory/internal/MemoryTest.java

    }
    // check 0 length array wraps
-    Memory memZeroLengthArrayBoolean = WritableMemory.writableWrap(new boolean[0]);
+    //Memory memZeroLengthArrayBoolean = WritableMemory.writableWrap(new boolean[0]);


should just remove these entirely instead of commenting them out

Will remove.

jmalkin · 2024-07-02T02:08:44Z

...a8/src/test/java/org/apache/datasketches/memory/internal/ExampleMemoryRequestServerTest.java

+/**
+ * Example of how to use the MemoryRequestServer with a memory hungry client.
+ *
+ * <p>Note: this example only works with Java 17.</p>


Is this accurate? If so, is it excluded from running?

Not accurate. Removed. It works fine in Java8

1. I have corrected the javadocs and code comments to use "off-heap" instead of "direct" or "native". However, I decided not to change the use of "Direct" in existing method calls or in the /internal/ directory. That would cause a huge number of changes (100's). 2. I have corrected the javadocs and code comments to use "alive" or "isAlive" instead of "valid" or "isValid". However, I decided not to change the word "Valid" in the /internal/ directory. That would cause a huge number of changes (100's).

jmalkin

Thankfully a single commit so I was able to look at that diff rather than try to figure out what changed in the entire PR :)

leerho added 6 commits June 11, 2024 14:57

Prepare for java 8,11 3.0.0

6bea9d2

Preparing for java 8/11 3.0.0

4f7252e

aligning java 8/11 3.0.0 API & behavior to be as similar to what 4.0.0

60b4ef7

will look like with java 17 FFM Panama

Update. Removing Handle references.

74f570a

Fixing a few minor things.

deed40d

Set poms to 3.0.0-SNAPSHOT

aaa40d6

leerho marked this pull request as ready for review June 20, 2024 15:37

leerho requested review from AlexanderSaydakov and jmalkin June 21, 2024 00:45

leerho added 3 commits June 25, 2024 16:22

Made corrections in javadocs, primarily.

d048aa7

Improved the toString methods. Simplified BaseBuffer API.

Changed name of BaseBuffer to Positional.

c6b61b7

And BaseBufferImpl to PositionalImpl. Cleaned up some duplicate methods. Improved some javadocs.

Renamed BaseBufferTest to PositionalTest

4502300

Corrected some tests that were referring to the wrong exception.

jmalkin requested changes Jul 2, 2024

View reviewed changes

leerho requested a review from jmalkin July 2, 2024 19:11

jmalkin approved these changes Jul 2, 2024

View reviewed changes

leerho merged commit 9fa68a7 into master Jul 2, 2024

leerho deleted the prepare_java8_11_3.0.0 branch July 2, 2024 21:08

Prepare for java 8,11 3.0.0 #190

Prepare for java 8,11 3.0.0 #190

Uh oh!

Conversation

leerho commented Jun 11, 2024

Uh oh!

leerho commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmalkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmalkin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leerho commented Jun 20, 2024 •

edited

Loading