8300236: Use VarHandle access in Data(Input | Output)Stream classes #12076
Conversation
|
👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into |
| } | ||
|
|
||
| // Alternative with an internal buffer fixed att offset zero. | ||
| public static final class BigEndianAtZeroBuffer { |
There was a problem hiding this comment.
I'd keep it simple an leave the buffer management to the caller. (Omitting this class).
|
|
||
| } | ||
|
|
||
| public static final class BigEndianAtZero { |
There was a problem hiding this comment.
I'd merge these methods (with zero offset) in with the previous class; the overloads with offsets are sufficient to distinguish them and it would make discovery more natural. (omitting this class).
| private Bits() { | ||
| } | ||
|
|
||
| public static final class BigEndian { |
There was a problem hiding this comment.
Are there other names that improve the readability of the uses?
For example ByteArray.getShort() and ByteArray.getDouble.
There's been a move to drop get and put prefixes where they are unnecessary.
Get and put prefixes do make them similar to the existing methods, but...
Is it just a readable to say:
float degrees = ByteArray.float(buf); // for get
ByteArray.int(buf, degrees); // for put
Maybe, maybe not....
|
Performance looks promising for serialization (values in us/operation):
|
minborg
changed the title
Webrevs
|
| ); | ||
| } catch (NoSuchMethodException | IllegalAccessException e) { | ||
| throw new InternalError("Can't lookup Bits.getXXX", e); | ||
| throw new InternalError("Can't lookup BigEndian.getXXX", e); |
There was a problem hiding this comment.
BigEndian -> ByteArrayAccess
| * questions. | ||
| */ | ||
|
|
||
| package jdk.internal.util.access; |
There was a problem hiding this comment.
This is pretty deep; I'd drop the final "access". The package name jdk.internal.util is fine.
There was a problem hiding this comment.
Happy to rename to ByteArray if we keep the package. The reason for proposing a separate package is that if we later decide to export the class, we are able to export to only this package and not all the other classes in `jdk.internal.util'. This could reduce coupling.
There was a problem hiding this comment.
"access" in the package/class name does look a bit strange, and could easily get mixed up with the package and classes that are used for shared secrets. I don't think jdk.internal.util.ByteArrays would look out of place. Hopefully it won't need to be exported to many other modules as we need to keep the qualified exports to a minimum.
| * Methods for unpacking primitive values from byte arrays starting at | ||
| * a given offsets. |
There was a problem hiding this comment.
Typo: "a given offsets".
| */ | ||
|
|
||
| /** | ||
| * Gets a {@code boolean} from the provided {@code array} at the given {@code offset}. |
There was a problem hiding this comment.
This could use the concise javadoc tag that provides both the first line and @return value. For example,
{@return Returns a {@code boolean} from the provided {@code array} at the given {@code offset}}
| * @param array to read a value from. | ||
| * @param offset where extraction in the array should begin | ||
| * @return a {@code boolean} from the array | ||
| * @throws NullPointerException if the provided {@code array} is {@code null} |
There was a problem hiding this comment.
With the class level statement about throwing NullPointer, the @throws is not necessary in each method.
| /* | ||
| * Methods for packing primitive values into byte arrays starting at offset zero. | ||
| */ |
There was a problem hiding this comment.
Is the only advantage to the zero offset versions in the API, shorter invocations with fewer parameters?
Is there any performance difference?
There was a problem hiding this comment.
Very good question. I will take a look at it. A fixed value is inserted in the original var handle so potentially it might improve performance that way too.
There was a problem hiding this comment.
No significant performance increase:
2-parameters
Benchmark Mode Cnt Score Error Units
PrimitiveFieldSerializationBenchmark.serializeData avgt 8 6.761 ± 0.126 ns/op
PrimitiveFieldSerializationBenchmark.serializeRecord avgt 8 6.890 ± 0.093 ns/op
3-parameters
Benchmark Mode Cnt Score Error Units
PrimitiveFieldSerializationBenchmark.serializeData avgt 8 6.850 ± 0.074 ns/op
PrimitiveFieldSerializationBenchmark.serializeRecord avgt 8 6.855 ± 0.057 ns/op
There was a problem hiding this comment.
I will remove the extra methods with lesser parameters.
| @@ -54,6 +56,8 @@ public DataInputStream(InputStream in) { | |||
| super(in); | |||
| } | |||
|
|
|||
| private final byte[] readBuffer = new byte[8]; | |||
There was a problem hiding this comment.
No objection to the readBuffer but it does make it wonder if we should re-visit bytearr and chararr as they are only need for reading modified UTF-8 strings and shouldn't need to be eagerly created.
There was a problem hiding this comment.
| * using {@linkplain ByteOrder#BIG_ENDIAN big endian order} (aka. "network order"). | ||
| * <p> | ||
| * All methods in this class will throw an {@linkplain NullPointerException} if {@code null} is | ||
| * passed in as a method parameter for a byte array. |
There was a problem hiding this comment.
The NPE is described in both the class description and some of the methods, it doesn't need to be both.
|
@minborg this pull request can not be integrated into git checkout bits-api
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
openjdk
bot
removed
the
merge-conflict
|
@minborg This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
| } | ||
|
|
||
| // At-zero methods | ||
|
|
There was a problem hiding this comment.
These AtZero test methods don't add much value? It is already testing an offset of 1 in the original tests.
|
/integrate |
|
Going to push as commit 74e1a8b.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
Not fully related to this PR, but is there a reason that functionality was exposed as a VarHandle factory instead of just methods that do that, taking an array and an index? These byte array handles don't seem to have much state in them other than endianness, that could just be passed in (improving |
This PR proposes using a performance optimization using a new supported API for operations similar to those found in
java.io.BitsProgress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12076/head:pull/12076$ git checkout pull/12076Update a local copy of the PR:
$ git checkout pull/12076$ git pull https://git.openjdk.org/jdk pull/12076/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12076View PR using the GUI difftool:
$ git pr show -t 12076Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12076.diff