Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template
On Fri, 26 Apr 2024 14:06:02 GMT, Hamlin Li wrote: > Hi, > Can you help to review this simple patch? > Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not > necessary, could be removed. > Thanks The intrinsic implementation will not perform bounds checks. I think what you may be observing is the result of bounds checks when Java code is interpreted (or perhaps from compiled C1 code). You need to first ensure the code is compiled to C2 before executing with out of bounds values. - PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2125465612
Re: [External] : Gatherer and primitive specialization
> On Jun 13, 2024, at 8:20 AM, fo...@univ-mlv.fr wrote: > > > > From: "Viktor Klang" > To: "Remi Forax" , "core-libs-dev" > > Sent: Thursday, June 13, 2024 12:52:03 PM > Subject: Re: [External] : Gatherer and primitive specialization > Hi Rémi, > > Given that Collector has not been specialized since it was introduced, we > opted to not specialize Gatherer eagerly as Valhalla Value Classes may also > move the needle a bit regarding the need for specialization in general. > > As i said previously, most collectors uses collections so until collections > are specialized, specializing the collector API is not that useful. > A gatherer unlike a Collector can just transform values, thus specializing it > is more useful. > > For Valhalla, we are at year 10 and value classes are not there yet, > specialization of generics is in my opinion 10 years ahead. > I'm not complaining, if we had rush value classes, everybody will have regret > it, but those things take a lot of time. > Also we may benefit from stack flattening with the boxed primitives. I would hold off on any such explicit specialization (the pressure on Streams was too great, less so for Gatherers IMO). Gatherers may provide a useful way to explore the effectiveness of using primitive boxes that don’t have identity. Paul.
Re: RFR: 8335252: ForceInline j.u.Formatter.Conversion#isValid [v2]
On Thu, 27 Jun 2024 14:12:36 GMT, Shaojin Wen wrote: >> Currently, the java.util.Formatter$Conversion::isValid method is implemented >> based on switch, which cannot be inlined because codeSize > 325. This >> problem can be avoided by implementing it with ImmutableBitSetPredicate. >> >> use `-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining` to see the master >> branch: >> >> @ 109 java.util.Formatter$Conversion::isValid (358 bytes) failed to >> inline: hot method too big >> >> >> current version >> >> @ 109 java.util.Formatter$Conversion::isValid (10 bytes) inline (hot) >> @ 4 >> jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test >> (50 bytes) inline (hot) > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > revert & use `@ForceInline` Can you provide some additional context here? I think we need to be very careful about the general use @ForceInline in core libraries. While you show a modest performance benefit using a the micro benchmark, will it actually make any difference overall given formatting strings is not particular efficient? String templates, currently removed, provided good string formatting performance, and the redesign will i think also provide good performance. - PR Comment: https://git.openjdk.org/jdk/pull/19926#issuecomment-2195095064
Re: RFR: 8335252: Reduce size of j.u.Formatter.Conversion#isValid [v4]
On Fri, 28 Jun 2024 12:46:49 GMT, Shaojin Wen wrote: >> Currently, the java.util.Formatter$Conversion::isValid method is implemented >> based on switch, which cannot be inlined because codeSize > 325. This >> problem can be avoided by implementing it with ImmutableBitSetPredicate. >> >> use `-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining` to see the master >> branch: >> >> @ 109 java.util.Formatter$Conversion::isValid (358 bytes) failed to >> inline: hot method too big >> >> >> current version >> >> @ 109 java.util.Formatter$Conversion::isValid (10 bytes) inline (hot) >> @ 4 >> jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test >> (50 bytes) inline (hot) > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > fix comment That's a clever change and more preferable (but still potentially fragile in the source to bytecode translation process). However I still don't understand the overall motivation, why does it matter? - PR Comment: https://git.openjdk.org/jdk/pull/19926#issuecomment-2197275066
Re: RFR: 8327854: Test java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpStatefulTest.java failed with RuntimeException
On Sat, 1 Jun 2024 11:49:39 GMT, Viktor Klang wrote: > This PR improves the test failure output for the failing test case, and the > underlying issue is likely going to be addressed by > https://github.com/openjdk/jdk/pull/19131 /cc @DougLea Marked as reviewed by psandoz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19508#pullrequestreview-2152007241
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception
On Wed, 3 Jul 2024 19:43:05 GMT, Hannes Greule wrote:
> Similar to how `MethodHandle#invoke(Exact)` methods are already handled, this
> change adds special casing for `VarHandle.{access-mode}` methods.
>
> The exception message is less exact, but I think that's acceptable.
src/hotspot/share/prims/methodHandles.cpp line 1371:
> 1369: * invoked directly.
> 1370: */
> 1371: JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject mh, jobjectArray args)) {
Suggestion:
JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject vh, jobjectArray args)) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664836522
Re: RFR: 8338023: Support two vector selectFrom API
On Thu, 8 Aug 2024 06:57:28 GMT, Jatin Bhateja wrote: > Hi All, > > As per the discussion on panama-dev mailing list[1], patch adds the support > for following new two vector permutation APIs. > > > Declaration:- > Vector.selectFrom(Vector v1, Vector v2) > > > Semantics:- > Using index values stored in the lanes of "this" vector, assemble the > values stored in first (v1) and second (v2) vector arguments. Thus, first and > second vector serves as a table, whose elements are selected based on index > value vector. API is applicable to all integral and floating-point types. > The result of this operation is semantically equivalent to expression > v1.rearrange(this.toShuffle(), v2). Values held in index vector lanes must > lie within valid two vector index range [0, 2*VLEN) else an > IndexOutOfBoundException is thrown. > > Summary of changes: > - Java side implementation of new selectFrom API. > - C2 compiler IR and inline expander changes. > - In absence of direct two vector permutation instruction in target ISA, a > lowering transformation dismantles new IR into constituent IR supported by > target platforms. > - Optimized x86 backend implementation for AVX512 and legacy target. > - Function tests covering new API. > > JMH micro included with this patch shows around 10-15x gain over existing > rearrange API :- > Test System: Intel(R) Xeon(R) Platinum 8480+ [ Sapphire Rapids Server] > > > Benchmark (size) Mode Cnt Score > Error Units > SelectFromBenchmark.rearrangeFromByteVector 1024 thrpt2 2041.762 >ops/ms > SelectFromBenchmark.rearrangeFromByteVector 2048 thrpt2 1028.550 >ops/ms > SelectFromBenchmark.rearrangeFromIntVector 1024 thrpt2962.605 >ops/ms > SelectFromBenchmark.rearrangeFromIntVector 2048 thrpt2479.004 >ops/ms > SelectFromBenchmark.rearrangeFromLongVector 1024 thrpt2359.758 >ops/ms > SelectFromBenchmark.rearrangeFromLongVector 2048 thrpt2178.192 >ops/ms > SelectFromBenchmark.rearrangeFromShortVector1024 thrpt2 1463.459 >ops/ms > SelectFromBenchmark.rearrangeFromShortVector2048 thrpt2727.556 >ops/ms > SelectFromBenchmark.selectFromByteVector1024 thrpt2 33254.830 >ops/ms > SelectFromBenchmark.selectFromByteVector2048 thrpt2 17313.174 >ops/ms > SelectFromBenchmark.selectFromIntVector 1024 thrpt2 10756.804 >ops/ms > SelectFromBenchmark.selectFromIntVector 2048 thrpt2 5398.2... The results look promising. I can provide guidance on the specification e.g., we can specify the behavior in terms of rearrange, with the addition of throwing on out of bounds indexes. Regarding the throwing of exceptions, some wider context will help to know where we are heading before we finalize the specification. I believe we are considering changing the default throwing behavior for index out of bounds to wrapping, thereby we can avoid bounds checks. If that is the case we should wait until that is done then update rather than submitting a CSR just yet? I see you created a specific intrinsic, which will avoid the cost of shuffle creation. Should we apply the same approach (in a subsequent PR) to the single argument shuffle? Or perhaps if we manage to optimize shuffles and change the default wrapping we don't require a specific intrinsic and can just use defer to rearrange? - PR Review: https://git.openjdk.org/jdk/pull/20508#pullrequestreview-2234095541
Re: RFR: 8338021: Support saturating vector operators in VectorAPI [v2]
On Thu, 8 Aug 2024 17:20:06 GMT, Jatin Bhateja wrote:
>> Hi All,
>>
>> As per the discussion on panama-dev mailing list[1], patch adds the support
>> following new vector operators.
>>
>>
>> . SATURATING_UADD : Saturating unsigned addition.
>> . SATURATING_ADD: Saturating signed addition.
>> . SATURATING_USUB : Saturating unsigned subtraction.
>> . SATURATING_SUB: Saturating signed subtraction.
>> . UMAX : Unsigned max
>> . UMIN : Unsigned min.
>>
>>
>> New vector operators are applicable to only integral types since their
>> values wraparound in over/underflowing scenarios after setting appropriate
>> status flags. For floating point types, as per IEEE 754 specs there are
>> multiple schemes to handler underflow, one of them is gradual underflow
>> which transitions the value to subnormal range. Similarly, overflow
>> implicitly saturates the floating-point value to an Infinite value.
>>
>> As the name suggests, these are saturating operations, i.e. the result of
>> the computation is strictly capped by lower and upper bounds of the result
>> type and is not wrapped around in underflowing or overflowing scenarios.
>>
>> Summary of changes:
>> - Java side implementation of new vector operators.
>> - Add new scalar saturating APIs for each of the above saturating vector
>> operator in corresponding primitive box classes, fallback implementation of
>> vector operators is based over it.
>> - C2 compiler IR and inline expander changes.
>> - Optimized x86 backend implementation for new vector operators and their
>> predicated counterparts.
>> - Extends existing VectorAPI Jtreg test suite to cover new operations.
>>
>> Kindly review and share your feedback.
>>
>> Best Regards,
>> PS: Intrinsification and auto-vectorization of new core-lib API will be
>> addressed separately in a follow-up patch.
>>
>> [1] https://mail.openjdk.org/pipermail/panama-dev/2024-May/020408.html
>
> Jatin Bhateja has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains three additional
> commits since the last revision:
>
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8338201
> - Removed redundant comment
> - 8338021: Support saturating vector operators in VectorAPI
Naming wise for the scalar methods i recommend the pattern of
`op{Saturating}{Unsigned}`, that fits better with naming patterns used
elsewhere, where we tend to be literal.
For the vector operations we should refer to unsigned consistently with the
unsigned compare operation names. Here we can be more terse. Which makes me
wonder if we should use `U` consistently for unsigned and `S` for saturating
e.g. `SUADD`, `UGT`, `UMAX` etc. Then that flows into the names used in
`VectorSupport.java` and `vectorSupport.hpp`.
-
PR Review: https://git.openjdk.org/jdk/pull/20507#pullrequestreview-2234118377
Re: RFR: 8338023: Support two vector selectFrom API [v3]
On Wed, 21 Aug 2024 16:42:44 GMT, Jatin Bhateja wrote: >> Hi All, >> >> As per the discussion on panama-dev mailing list[1], patch adds the support >> for following new two vector permutation APIs. >> >> >> Declaration:- >> Vector.selectFrom(Vector v1, Vector v2) >> >> >> Semantics:- >> Using index values stored in the lanes of "this" vector, assemble the >> values stored in first (v1) and second (v2) vector arguments. Thus, first >> and second vector serves as a table, whose elements are selected based on >> index value vector. API is applicable to all integral and floating-point >> types. The result of this operation is semantically equivalent to >> expression v1.rearrange(this.toShuffle(), v2). Values held in index vector >> lanes must lie within valid two vector index range [0, 2*VLEN) else an >> IndexOutOfBoundException is thrown. >> >> Summary of changes: >> - Java side implementation of new selectFrom API. >> - C2 compiler IR and inline expander changes. >> - In absence of direct two vector permutation instruction in target ISA, a >> lowering transformation dismantles new IR into constituent IR supported by >> target platforms. >> - Optimized x86 backend implementation for AVX512 and legacy target. >> - Function tests covering new API. >> >> JMH micro included with this patch shows around 10-15x gain over existing >> rearrange API :- >> Test System: Intel(R) Xeon(R) Platinum 8480+ [ Sapphire Rapids Server] >> >> >> Benchmark (size) Mode Cnt >> Score Error Units >> SelectFromBenchmark.rearrangeFromByteVector 1024 thrpt2 2041.762 >> ops/ms >> SelectFromBenchmark.rearrangeFromByteVector 2048 thrpt2 1028.550 >> ops/ms >> SelectFromBenchmark.rearrangeFromIntVector 1024 thrpt2962.605 >> ops/ms >> SelectFromBenchmark.rearrangeFromIntVector 2048 thrpt2479.004 >> ops/ms >> SelectFromBenchmark.rearrangeFromLongVector 1024 thrpt2359.758 >> ops/ms >> SelectFromBenchmark.rearrangeFromLongVector 2048 thrpt2178.192 >> ops/ms >> SelectFromBenchmark.rearrangeFromShortVector1024 thrpt2 1463.459 >> ops/ms >> SelectFromBenchmark.rearrangeFromShortVector2048 thrpt2727.556 >> ops/ms >> SelectFromBenchmark.selectFromByteVector1024 thrpt2 33254.830 >> ops/ms >> SelectFromBenchmark.selectFromByteVector2048 thrpt2 17313.174 >> ops/ms >> SelectFromBenchmark.selectFromIntVector 1024 thrpt2 10756.804 >> ops/ms >> S... > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > Pass explicit wrap argument to selectFrom API with default value set to > true. Is it possible for the intrinsic to be responsible for wrapping, if needed? If was looking at [`vpermi2b`](https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=vpermi2b&ig_expand=4917,4982,5004,5010,5014&techs=AVX_512) and AFAICT it implicitly wraps, operating on the lower N bits. Is that correct? - PR Comment: https://git.openjdk.org/jdk/pull/20508#issuecomment-2302707611
Re: RFR: 8338023: Support two vector selectFrom API [v5]
On Fri, 23 Aug 2024 06:09:48 GMT, Jatin Bhateja wrote: >> Hi All, >> >> As per the discussion on panama-dev mailing list[1], patch adds the support >> for following new two vector permutation APIs. >> >> >> Declaration:- >> Vector.selectFrom(Vector v1, Vector v2) >> >> >> Semantics:- >> Using index values stored in the lanes of "this" vector, assemble the >> values stored in first (v1) and second (v2) vector arguments. Thus, first >> and second vector serves as a table, whose elements are selected based on >> index value vector. API is applicable to all integral and floating-point >> types. The result of this operation is semantically equivalent to >> expression v1.rearrange(this.toShuffle(), v2). Values held in index vector >> lanes must lie within valid two vector index range [0, 2*VLEN) else an >> IndexOutOfBoundException is thrown. >> >> Summary of changes: >> - Java side implementation of new selectFrom API. >> - C2 compiler IR and inline expander changes. >> - In absence of direct two vector permutation instruction in target ISA, a >> lowering transformation dismantles new IR into constituent IR supported by >> target platforms. >> - Optimized x86 backend implementation for AVX512 and legacy target. >> - Function tests covering new API. >> >> JMH micro included with this patch shows around 10-15x gain over existing >> rearrange API :- >> Test System: Intel(R) Xeon(R) Platinum 8480+ [ Sapphire Rapids Server] >> >> >> Benchmark (size) Mode Cnt >> Score Error Units >> SelectFromBenchmark.rearrangeFromByteVector 1024 thrpt2 2041.762 >> ops/ms >> SelectFromBenchmark.rearrangeFromByteVector 2048 thrpt2 1028.550 >> ops/ms >> SelectFromBenchmark.rearrangeFromIntVector 1024 thrpt2962.605 >> ops/ms >> SelectFromBenchmark.rearrangeFromIntVector 2048 thrpt2479.004 >> ops/ms >> SelectFromBenchmark.rearrangeFromLongVector 1024 thrpt2359.758 >> ops/ms >> SelectFromBenchmark.rearrangeFromLongVector 2048 thrpt2178.192 >> ops/ms >> SelectFromBenchmark.rearrangeFromShortVector1024 thrpt2 1463.459 >> ops/ms >> SelectFromBenchmark.rearrangeFromShortVector2048 thrpt2727.556 >> ops/ms >> SelectFromBenchmark.selectFromByteVector1024 thrpt2 33254.830 >> ops/ms >> SelectFromBenchmark.selectFromByteVector2048 thrpt2 17313.174 >> ops/ms >> SelectFromBenchmark.selectFromIntVector 1024 thrpt2 10756.804 >> ops/ms >> S... > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > Removing redundant checkIndex routine API changes look good. (Note at the moment we are not proposing to change how shuffles works - as you point out the two vector `selectFrom` and `rearrange` differ in the index representation.) IIUC if the more direct two-table instruction is not available you fall back to calling two single arg rearranges with a blend, as a lowering transformation, similar to the fallback Java expression. The float/double conversion bothers me, not suggesting we do something about it here, noting down for any future conversation on shuffles. Ideally we would want the equivalent integral vector (int or long) to represent the index, tricky to express in the API, or alternative treat as a bitwise no-op conversion (there is also impact on `toShuffle` too). - PR Comment: https://git.openjdk.org/jdk/pull/20508#issuecomment-2307886044
Re: RFR: 8338728: Misc issues in memory layout javadoc [v2]
On Wed, 21 Aug 2024 13:24:15 GMT, Maurizio Cimadamore wrote: >> This PR fixes two minor issues in the `MemoryLayout` javadoc: >> * the section describing dereference path talks about `P` and `P'` but then >> only uses `P` in the code; >> * the `ceilDiv` math on the `PathElement::sequenceElement(long, long)` is >> wrong, as the division returns a negative number for F < 0, which is >> incorrect > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > * Clarify javadoc for var handles with dereference path elements > * Add test where dereference path element is last element in path Marked as reviewed by psandoz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20659#pullrequestreview-2261775558
Re: RFR: 8338731: MemoryLayout::offsetHandle can return a negative offset
On Wed, 21 Aug 2024 13:26:58 GMT, Maurizio Cimadamore wrote: > When working on startup improvements, I noticed that the method handle > returned by `MemoryLayout::offsetHandle` can overflow if the client calls the > handle with a base offset that is too big. > > In other similar situations, the layout API always fails with > `ArithmeticException` (see `MemoryLayout::scale`), so we should do the same > here. > > The fix is to use a `Math::addExact(long, long)` for the outermost add > operation in the computation of the offset method handle. That outermost > computation in fact is the only one that can overflow: it is an addition > between a user-provided base offset `B` and a layout offset `L`. `L` is > guaranteed not to overflow, by construction (as `L` is derived from a layout > path). But `B` + `L` might overflow, so the new logic checks for that. Marked as reviewed by psandoz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20662#pullrequestreview-2261784414
Re: RFR: 8338023: Support two vector selectFrom API [v6]
On Tue, 27 Aug 2024 09:58:44 GMT, Jatin Bhateja wrote: >> Hi All, >> >> As per the discussion on panama-dev mailing list[1], patch adds the support >> for following new two vector permutation APIs. >> >> >> Declaration:- >> Vector.selectFrom(Vector v1, Vector v2) >> >> >> Semantics:- >> Using index values stored in the lanes of "this" vector, assemble the >> values stored in first (v1) and second (v2) vector arguments. Thus, first >> and second vector serves as a table, whose elements are selected based on >> index value vector. API is applicable to all integral and floating-point >> types. The result of this operation is semantically equivalent to >> expression v1.rearrange(this.toShuffle(), v2). Values held in index vector >> lanes must lie within valid two vector index range [0, 2*VLEN) else an >> IndexOutOfBoundException is thrown. >> >> Summary of changes: >> - Java side implementation of new selectFrom API. >> - C2 compiler IR and inline expander changes. >> - In absence of direct two vector permutation instruction in target ISA, a >> lowering transformation dismantles new IR into constituent IR supported by >> target platforms. >> - Optimized x86 backend implementation for AVX512 and legacy target. >> - Function tests covering new API. >> >> JMH micro included with this patch shows around 10-15x gain over existing >> rearrange API :- >> Test System: Intel(R) Xeon(R) Platinum 8480+ [ Sapphire Rapids Server] >> >> >> Benchmark (size) Mode Cnt >> Score Error Units >> SelectFromBenchmark.rearrangeFromByteVector 1024 thrpt2 2041.762 >> ops/ms >> SelectFromBenchmark.rearrangeFromByteVector 2048 thrpt2 1028.550 >> ops/ms >> SelectFromBenchmark.rearrangeFromIntVector 1024 thrpt2962.605 >> ops/ms >> SelectFromBenchmark.rearrangeFromIntVector 2048 thrpt2479.004 >> ops/ms >> SelectFromBenchmark.rearrangeFromLongVector 1024 thrpt2359.758 >> ops/ms >> SelectFromBenchmark.rearrangeFromLongVector 2048 thrpt2178.192 >> ops/ms >> SelectFromBenchmark.rearrangeFromShortVector1024 thrpt2 1463.459 >> ops/ms >> SelectFromBenchmark.rearrangeFromShortVector2048 thrpt2727.556 >> ops/ms >> SelectFromBenchmark.selectFromByteVector1024 thrpt2 33254.830 >> ops/ms >> SelectFromBenchmark.selectFromByteVector2048 thrpt2 17313.174 >> ops/ms >> SelectFromBenchmark.selectFromIntVector 1024 thrpt2 10756.804 >> ops/ms >> S... > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments resolutions. I think we should leave the fallback expression as `vec2.rearrange(vec1.toShuffle(), vec3);`, lets address that separately if needed. Otherwise, you have introduced an additional code path that requires more explicit testing. My comment was related to understanding what `SelectFromTwoVectorNode::Ideal` and `VectorRearrangeNode::Ideal` are doing - the former lowers, if needed, into the rearrange expression and the latter adjusts, if needed, the index vector (a comment describing this transformation would be useful, like you have in the former method). - PR Comment: https://git.openjdk.org/jdk/pull/20508#issuecomment-2313401788
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v5]
On Tue, 27 Aug 2024 10:38:46 GMT, Per Minborg wrote: >> The performance of the `MemorySegment::fil` can be improved by replacing the >> `checkAccess()` method call with calling `checkReadOnly()` instead (as the >> bounds of the segment itself do not need to be checked). >> >> Also, smaller segments can be handled directly by Java code rather than >> transitioning to native code. >> >> Here is how the `MemorySegment::fill` performance is improved by this PR: >> >>  >> >> Operations involving 8 or more bytes are delegated to native code whereas >> smaller segments are handled via a switch rake. >> >> It should be noted that `Arena::allocate` is using `MemorySegment::fil`. >> Hence, this PR will also have a positive effect on memory allocation >> performance. > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Merge branch 'master' into fill-performance > - Fix typo > - Add a comment about the old switch type > - Remove unused import > - Reduce kick-in size and add test > - Initial implementation How fast do we need to be here given we are measuring in a few nanoseconds per operation? What if the goal is not to regress from say explicitly filling in a small sized segment or a comparable array (e.g., < 8 bytes) then maybe a loop suffices and the code is simple? - PR Comment: https://git.openjdk.org/jdk/pull/20712#issuecomment-2313446118
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v7]
On Wed, 28 Aug 2024 11:10:05 GMT, Per Minborg wrote: >> The performance of the `MemorySegment::fil` can be improved by replacing the >> `checkAccess()` method call with calling `checkReadOnly()` instead (as the >> bounds of the segment itself do not need to be checked). >> >> Also, smaller segments can be handled directly by Java code rather than >> transitioning to native code. >> >> Here is how the `MemorySegment::fill` performance is improved by this PR: >> >>  >> >> Operations involving 8 or more bytes are delegated to native code whereas >> smaller segments are handled via a switch rake. >> >> It should be noted that `Arena::allocate` is using `MemorySegment::fil`. >> Hence, this PR will also have a positive effect on memory allocation >> performance. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Make minor updates after comments Current approach looks reasonable. - Marked as reviewed by psandoz (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20712#pullrequestreview-2266627806
Re: Collections.shuffle to accept RandomGenerator
That looks reasonable.
Thinking a little more broadly.
We could also change Collections.shuffle(List) to use ThreadLocalRandom so it
does not have to cache the Random instance, plus it has better concurrent and
PRNG properties. The spec does not describe the precise details of randomness.
It’s tempting to lift Collections.shuffle(List, RandomGenerator) to
List.shuffle(RandomGenerator ), similar to what we did for Collections.sort, to
align with that pattern and which also aligns with reverse of sequenced
collections. There is likely not much performance advantage to do doing so as
there was with sort, but that location is much easier to find and use IMHO.
Since the method accepts RandomGenerator the compatibility risk would likely be
low.
Paul.
> On Sep 27, 2022, at 3:11 AM, Tagir Valeev wrote:
>
> Hello!
>
> Currently, Collections.shuffle(List, Random) accepts an outdated
> Random class instead of RandomGenerator interface. It looks like,
> RandomGenerator would suffice. The code change looks trivial (aside
> from documentation and testing), as shuffle() implementation does not
> need anything except nextInt:
>
> diff --git a/src/java.base/share/classes/java/util/Collections.java
> b/src/java.base/share/classes/java/util/Collections.java
> --- a/src/java.base/share/classes/java/util/Collections.java (revision
> cab590517bf705418c7376edd5d7066b13b6dde8)
> +++ b/src/java.base/share/classes/java/util/Collections.java (date
> 1664273240190)
> @@ -37,6 +37,7 @@
> import java.util.function.IntFunction;
> import java.util.function.Predicate;
> import java.util.function.UnaryOperator;
> +import java.util.random.RandomGenerator;
> import java.util.stream.IntStream;
> import java.util.stream.Stream;
> import java.util.stream.StreamSupport;
> @@ -454,8 +455,12 @@
> * @throws UnsupportedOperationException if the specified list or its
> * list-iterator does not support the {@code set} operation.
> */
> -@SuppressWarnings({"rawtypes", "unchecked"})
> public static void shuffle(List list, Random rnd) {
> +shuffle(list, (RandomGenerator) rnd);
> +}
> +
> +@SuppressWarnings({"rawtypes", "unchecked"})
> +public static void shuffle(List list, RandomGenerator rnd) {
> int size = list.size();
> if (size < SHUFFLE_THRESHOLD || list instanceof RandomAccess) {
> for (int i=size; i>1; i--)
>
> What do you think? Should we implement this improvement? I think I can
> contribute if there's a general agreement that such an enhancement
> would be useful.
>
> With best regards,
> Tagir Valeev
Re: RFR: 8279361: Error in documentation of third Stream.reduce method
On Wed, 28 Sep 2022 15:36:50 GMT, Viktor Klang wrote: > This JavaDoc change attempts to shine some light on the `combiner`-function > as it relates to the third variant of `Stream.reduce` since it according to > the bug submission in JBS can be confusing that the `combiner` is not > mentioned in the code example in the documentation. The current JavaDoc is not wrong :-) (same in other places). I think what you propose may not add much clarity (given the complexities of reasoning about parallelism). We could add something after "but is not constrained to execute sequentially." e.g. " Parallel execution may result in intermediate results computed as if by the code above. Such results are combined using the combining function to produce the final result." " - PR: https://git.openjdk.org/jdk/pull/10469
Re: RFR: JDK-8294539: Augment discussion of equivalence relations on floating-point values [v2]
On Fri, 30 Sep 2022 17:24:40 GMT, Joe Darcy wrote:
>> While the floating-point == operation is *not* an equivalence relation,
>> there are useful equivalence relations that can be defined over
>> floating-point values. Text is added to java.lang.Double to discuss and name
>> those relations.
>
> Joe Darcy has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Add discussion of numerical equality.
> - Fix typo.
Marked as reviewed by psandoz (Reviewer).
src/java.base/share/classes/java/lang/Double.java line 166:
> 164: * equivalence relation for {@code double} values {@code a} and {@code
> b} is
> 165: * implemented by the expression
> 166: * {@code Double.doubleTo}Raw{@code LongBits(a)
> == Double.doubleTo}Raw{@code LongBits(b)}
Place in a code snippet since it's hard to read in source otherwise? Snippets
allow for highlighting e.g.
/**
* {@snippet :
* Double.doubleToRawLongBits(a) == Double.doubleToRawLongBits(b) // @highlight
substring="Raw"
* }
*/
-
PR: https://git.openjdk.org/jdk/pull/10498
Re: RFR: JDK-8294539: Augment discussion of equivalence relations on floating-point values [v2]
On Tue, 4 Oct 2022 05:26:08 GMT, Joe Darcy wrote:
>> src/java.base/share/classes/java/lang/Double.java line 166:
>>
>>> 164: * equivalence relation for {@code double} values {@code a} and {@code
>>> b} is
>>> 165: * implemented by the expression
>>> 166: * {@code Double.doubleTo}Raw{@code
>>> LongBits(a) == Double.doubleTo}Raw{@code
>>> LongBits(b)}
>>
>> Place in a code snippet since it's hard to read in source otherwise?
>> Snippets allow for highlighting e.g.
>>
>> /**
>> * {@snippet :
>> * Double.doubleToRawLongBits(a) == Double.doubleToRawLongBits(b) //
>> @highlight substring="Raw"
>> * }
>> */
>
> Before sending out the PR, I used a snippet, but it formatted the code using
> too much vertical space for the definition list. I didn't look to see if
> there was a "compact" styling option that could be used.
Ok, i am not aware of a compact style.
-
PR: https://git.openjdk.org/jdk/pull/10498
Re: RFR: JDK-8294539: Augment discussion of equivalence relations on floating-point values [v5]
On Tue, 4 Oct 2022 19:56:21 GMT, Joe Darcy wrote: >> While the floating-point == operation is *not* an equivalence relation, >> there are useful equivalence relations that can be defined over >> floating-point values. Text is added to java.lang.Double to discuss and name >> those relations. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Appease jcheck; reflow paragraphs. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10498
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v6]
On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Make memory session a pure lifetime abstraction
src/java.base/share/classes/java/lang/foreign/Arena.java line 37:
> 35: * This session is created with the arena, and is closed when the arena
> is {@linkplain #close() closed}.
> 36: * Furthermore, all the native segments {@linkplain #allocate(long, long)
> allocated} by the arena are associated
> 37: * with that session.
I think we can simplify the wording by saying an arena has a session:
Suggestion:
* An arena is a {@linkplain AutoCloseable closeable} segment allocator that
has a {@link #session() memory session}.
* The arena's session is created when the arena is created, and is closed when
the arena is {@linkplain #close() closed}.
* All native segments {@linkplain #allocate(long, long) allocated} by the
arena are associated
* with its session.
src/java.base/share/classes/java/lang/foreign/Arena.java line 65:
> 63: * The {@link MemorySegment#address()} of the returned memory segment
> is the starting address of the
> 64: * allocated off-heap memory region backing the segment. Moreover,
> the {@linkplain MemorySegment#address() address}
> 65: * of the returned segment is aligned according the provided
> alignment constraint.
Suggestion:
/**
* Creates a native memory segment with the given size (in bytes) and
alignment constraint (in bytes).
* The returned segment is associated with the arena's memory session.
* The segment's {@link MemorySegment#address() address} is the starting
address of the
* allocated off-heap memory region backing the segment, and the address is
* aligned according the provided alignment constraint.
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v6]
On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Make memory session a pure lifetime abstraction src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 357: > 355: > 356: /** > 357: * Creates an access var handle that can be used to access a memory > segment at the layout selected by the given layout path, Suggestion: * Creates a var handle that can be used to access a memory segment at the layout selected by the given layout path, - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v6]
On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Make memory session a pure lifetime abstraction
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 104:
> 102: * Every memory segment is associated with a {@linkplain MemorySession
> memory session}. This ensures that access operations
> 103: * on a memory segment cannot occur when the region of memory which
> backs the memory segment is no longer available
> 104: * (e.g. after the memory session associated with the accessed memory
> segment is no longer {@linkplain MemorySession#isAlive() alive}.
Missing close brace:
Suggestion:
* (e.g., after the memory session associated with the accessed memory segment
is no longer {@linkplain MemorySession#isAlive() alive}).
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v6]
On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Make memory session a pure lifetime abstraction src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 312: > 310: * > 311: * > 312: * Heap segment can only be accessed using a layout whose alignment is > smaller or equal to the Suggestion: * Heap segments can only be accessed using a layout whose alignment is smaller or equal to the - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v6]
On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Make memory session a pure lifetime abstraction src/java.base/share/classes/java/lang/foreign/MemorySession.java line 83: > 81: * MemorySegment segment = MemorySegment.allocateNative(100, > MemorySession.implicit()); > 82: * ... > 83: * segment = null; // the segment session is unreacheable here and > becomes available for implicit close Typo: Suggestion: * segment = null; // the segment session is unreachable here and becomes available for implicit close - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v6]
On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Make memory session a pure lifetime abstraction src/java.base/share/classes/java/lang/foreign/ValueLayout.java line 329: > 327: /** > 328: * Returns an unbounded address layout with the same > carrier, alignment constraint, name and order as this address layout, > 329: * but with the specified pointee layout. An unbounded address > layouts allow raw addresses to be accessed Suggestion: * but with the specified pointee layout. An unbounded address layout allow raw addresses to be accessed - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v6]
On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Make memory session a pure lifetime abstraction src/java.base/share/classes/java/lang/foreign/package-info.java line 103: > 101: * the memory session associated with the segment being accessed has not > been closed prematurely. > 102: * We call this guarantee temporal safety. Together, spatial > and temporal safety ensure that each memory access > 103: * operation either succeeds - and accesses a valid location of the > region of memory backing the memory segment - or fails. Suggestion: * operation either succeeds - and accesses a valid location within the region of memory backing the memory segment - or fails. - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v17]
On Wed, 9 Nov 2022 13:24:54 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak Arena::close javadoc
src/java.base/share/classes/java/lang/foreign/Arena.java line 101:
> 99: * @throws IllegalArgumentException if {@code bytesSize < 0}, {@code
> alignmentBytes <= 0}, or if {@code alignmentBytes}
> 100: * is not a power of 2.
> 101: * @throws IllegalStateException if the session associated with this
> arena is not {@linkplain MemorySession#isAlive() alive}.
Suggestion:
* @throws IllegalStateException if arena's session is not {@linkplain
MemorySession#isAlive() alive}.
src/java.base/share/classes/java/lang/foreign/Arena.java line 121:
> 119: * segments associated with that memory session are also released.
> 120: * @throws IllegalStateException if the session associated with this
> arena is not {@linkplain MemorySession#isAlive() alive}.
> 121: * @throws IllegalStateException if this session is {@linkplain
> MemorySession#whileAlive(Runnable) kept alive} by another client.
Suggestion:
* @throws IllegalStateException if the arena's session is not {@linkplain
MemorySession#isAlive() alive}.
* @throws IllegalStateException if the arena's session is {@linkplain
MemorySession#whileAlive(Runnable) kept alive}.
Note i removed "by another client". I wanted to say "by another thread", but
then there is the case of calling close from within the Runnable passed to
whileAlive, so i wanted to say "by another caller". But, i think this can all
be implied and we don't need to say anything.
src/java.base/share/classes/java/lang/foreign/MemorySession.java line 66:
> 64: * is not critical, or in unstructured cases where the boundaries of the
> lifetime associated with a memory session
> 65: * cannot be easily determined. As shown in the example above, a memory
> session that is managed implicitly cannot end
> 66: * if a program references to one or more segments associated with that
> session. This means that memory segments associated
Suggestion:
* if a program references one or more segments associated with that session.
This means that memory segments associated
src/java.base/share/classes/java/lang/foreign/MemorySession.java line 89:
> 87:
> 88: /**
> 89: * {@return {@code true} if the provided thread can access and/or
> obtain segments associated with this memory session}
Is the following accurate and more concise?
Suggestion:
* {@return {@code true} if the provided thread can access and/or associate
segments with this memory session}
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Tue, 15 Nov 2022 18:47:39 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix typo in SegmentScope javadoc
src/java.base/share/classes/java/lang/foreign/Arena.java line 132:
> 130: * and all the memory segments associated with it can no longer be
> accessed. Furthermore, any off-heap region of memory backing the
> 131: * segments associated with that scope are also released.
> 132: * @throws IllegalStateException if the arena has already been
> {@linkplain #close() closed}.
JavaDoc was pointing to itself.
Suggestion:
* @throws IllegalStateException if the arena has already been closed.
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 109:
> 107: * Finally, access operations on a memory segment are subject to the
> thread-confinement checks enforced by the associated
> 108: * scope; that is, if the segment is the {@linkplain
> SegmentScope#global() global scope} or an {@linkplain SegmentScope#auto()
> automatic scope},
> 109: * it can be accessed by multiple threads. If the segment is associatd
> with an arena scope, then it can only be
Typo:
Suggestion:
* it can be accessed by multiple threads. If the segment is associated with an
arena scope, then it can only be
src/java.base/share/classes/java/lang/foreign/SegmentScope.java line 10:
> 8: * A segment scope controls access to a memory segment.
> 9: *
> 10: * A memory segment can only be accessed while its scope is {@linkplain
> #isAlive() alive}. Moreoever,
Typo:
Suggestion:
* A memory segment can only be accessed while its scope is {@linkplain
#isAlive() alive}. Moreover,
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v10]
On Wed, 16 Nov 2022 16:55:24 GMT, Andrew Haley wrote: >> JEP 429 implementation. > > Andrew Haley has updated the pull request incrementally with two additional > commits since the last revision: > > - Javadoc changes. > - ProblemList.txt cleanup src/hotspot/share/utilities/exceptions.cpp line 166: > 164: // Remove the ScopedValue cache in case we got a virtual machine > 165: // Error while we were trying to manipulate ScopedValue bindings. > 166: thread->set_scopedValueCache(NULL); I am see this pattern repeat quite often: thread->set_scopedValueCache(NULL); oop threadObj = thread->vthread(); assert(threadOop != NULL, "must be"); // <--- sometimes java_lang_Thread::clear_scopedValueBindings(threadObj); Encapsulate in a method on the `JavaThread` class? - PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v10]
On Wed, 16 Nov 2022 16:55:24 GMT, Andrew Haley wrote:
>> JEP 429 implementation.
>
> Andrew Haley has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Javadoc changes.
> - ProblemList.txt cleanup
src/java.base/share/classes/java/lang/Thread.java line 744:
> 742:
> 743: // special value to mean a new thread
> 744: this.scopedValueBindings = Thread.class;
Perhaps:
static final Object NEW_THREAD_BINDINGS = Thread.class;
...
this.scopedValueBindings = NEW_THREAD_BINDINGS;
?
src/java.base/share/classes/java/lang/Thread.java line 1614:
> 1612: }
> 1613:
> 1614: @Hidden
Should we document that the name and signature are special (same of other
relevant methods not already documented).
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 49:
> 47: *
> 48: * {@code ScopedValue} defines the {@link #where(ScopedValue, Object,
> Runnable)}
> 49: * method to set the value of a {@code ScopedValue} for the bouned period
> of execution by
Suggestion:
* method to set the value of a {@code ScopedValue} for the bounded period of
execution by
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 241:
> 239: }
> 240:
> 241: static final class EmptySnapshot extends Snapshot {
We could make `Snapshot` final have a static final field?
static final Snapshot EMPTY_SNAPSHOT = new Snapshot();
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 391:
> 389: * JVM_FindScopedValueBindings().
> 390: */
> 391: private R runWith(Snapshot newSnapshot, Callable op)
> throws Exception {
Missing `@Hidden` and `@ForceInline` ? like with the other `runWith` method
accepting `Runnable`?
(I was gonna suggest changing the name to `callWith`, but then reread the docs
and VM code. Its convenient to have the same names.)
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 672:
> 670:return EmptySnapshot.getInstance();
> 671: }
> 672: if (bindings == null) {
Suggestion:
if (bindings == NEW_THREAD_BINDINGS) {
// This must be a new thread
return Snapshot.EMPTY_SNAPSHOT;
} else if (bindings == null) {
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 720:
> 718: // is invoked, we record the result of the lookup in this per-thread
> cache
> 719: // for fast access in future.
> 720: private static class Cache {
Make the class final and remove the qualifier on all the methods.
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 824:
> 822: }
> 823:
> 824: public static void invalidate() {
Is this method used?
-
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v25]
On Wed, 23 Nov 2022 18:39:19 GMT, Andrew Haley wrote:
>> JEP 429 implementation.
>
> Andrew Haley has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 52 commits:
>
> - Merge master
> - javadoc
> - Feedback from reviewers
> - Update src/hotspot/share/classfile/vmSymbols.hpp
> - Merge branch 'JDK-828' of https://github.com/theRealAph/jdk into
> JDK-828
> - Update src/java.base/share/classes/java/lang/VirtualThread.java
>
>Co-authored-by: Alan Bateman
> - Update src/java.base/share/classes/java/lang/Thread.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Update src/hotspot/cpu/aarch64/aarch64.ad
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Feedback from reviewers
> - Remove incorrect assertion.
> - ... and 42 more: https://git.openjdk.org/jdk/compare/2afb4c33...30f150e1
Looks good (just some last minor comments). I did not focus on the tests, nor
too closely at all of the HotSpot changes.
Something for future investigation perhaps (if not already thought about):
consider using a persistent map, a Hash Array Mapped Trie (HAMT), for storing
scoped keys and values, which could potentially remove the need for the cache
of replace the cache when many values are in scope. The HAMT's structural
sharing properties, wide-branching factor, and `Integer.bitCount` being
intrinsic all make for an efficient implementation.
src/hotspot/share/classfile/vmSymbols.hpp line 401:
> 399: template(daemon_name, "daemon")
> \
> 400: template(run_method_name, "run")
> \
> 401: template(call_method_name, "call")
> \
Is this used?
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 209:
> 207: final int bitmask;
> 208:
> 209: private static final Object NIL = new Object();
Suggestion:
static final Object NO_VALUE = new Object();
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 212:
> 210:
> 211: static final Snapshot EMPTY_SNAPSHOT = new Snapshot();
> 212: Snapshot(Carrier bindings, Snapshot prev) {
Suggestion:
static final Snapshot EMPTY_SNAPSHOT = new Snapshot();
Snapshot(Carrier bindings, Snapshot prev) {
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 218:
> 216: }
> 217:
> 218: protected Snapshot() {
Suggestion:
Snapshot() {
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 464:
> 462: * Calls a value-returning operation with a {@code ScopedValue}
> bound to a value
> 463: * in the current thread. When the operation completes (normally or
> with an
> 464: * exception), the {@code ScopedValue} will revert to being unbound,
> or rervert to
Suggestion:
* exception), the {@code ScopedValue} will revert to being unbound, or
revert to
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v25]
On Thu, 24 Nov 2022 09:27:04 GMT, Andrew Haley wrote: >> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java >> line 209: >> >>> 207: final int bitmask; >>> 208: >>> 209: private static final Object NIL = new Object(); >> >> Suggestion: >> >> static final Object NO_VALUE = new Object(); > > It not very important, but I'm going to push back (very gently) on this one. > "nil: noun. nothing; naught; zero. adjective. having no value or existence." > That is the exact literal meaning of this sentinel. Also, "nil" has been used > with this meaning in programming languages for 60 years. What is your > objection to it here? I agree its not very important, please feel free to ignore my suggestion. My thinking was to prefer something more explicit in the code when reading, since i felt the use of the term nil was more idiomatic in other languages than Java. - PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v31]
On Wed, 23 Nov 2022 17:33:06 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> * remove unused Scoped interface
> * re-add trusting of final fields in layout class implementations
> * Fix BulkOps benchmark, which had alignment issues
Marked as reviewed by psandoz (Reviewer).
src/java.base/share/classes/jdk/internal/foreign/FunctionDescriptorImpl.java
line 57:
> 55: * {@return the return layout (if any) associated with this function
> descriptor}
> 56: */
> 57: public final Optional returnLayout() {
No need for `final` since class is final.
Suggestion:
public Optional returnLayout() {
src/java.base/share/classes/jdk/internal/foreign/SlicingAllocator.java line 33:
> 31: public final class SlicingAllocator implements SegmentAllocator {
> 32:
> 33: public static final long DEFAULT_BLOCK_SIZE = 4 * 1024;
Not used.
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]
On Mon, 28 Nov 2022 10:47:47 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> ~~try-with-resources~~ *try-finally* friendly access method was added. This
>> made is more easy to add guarding and did not risk lambda capturing. Also,
>> introducing lambdas in certain fundamental JDK classes might incur bootstrap
>> problems.
>>
>> The aforementioned ~~TwR~~ TF is using a ~~"session acquisition" that is not
>> used explicitly in the try block itself~~ session used in the *finally*
>> block. ~~This raises a warning that is suppressed using
>> `@SuppressWarnings("try")`. In the future, we might be able to remove these
>> suppressions by using the reserved variable name `_`.~~
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Re-introduce yet another address vairable
The approach looks good, and almost the least intrusive (see comment).
src/java.base/share/classes/java/nio/Buffer.java line 838:
> 836:
> 837: @Override
> 838: public void releaseSession(Buffer buffer,
> MemorySessionImpl scope) {
I prefer methods that do not expose the scope implementation so access is
limited to just to the acquire/release methods, but i am unsure of the
performance implications. These methods might not reliably inline, and we may
need to ensure that first (which is also separately a good thing). I think it
requires that the shared secret fields are stable and that there is only one
implementation of `JavaNioAccess`, which there is, but we can enforce via
sealing. Something to consider as a further iteration.
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v17]
On Tue, 29 Nov 2022 09:40:58 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> ~~try-with-resources~~ *try-finally* friendly access method was added. This
>> made is more easy to add guarding and did not risk lambda capturing. Also,
>> introducing lambdas in certain fundamental JDK classes might incur bootstrap
>> problems.
>>
>> The aforementioned ~~TwR~~ TF is using a ~~"session acquisition" that is not
>> used explicitly in the try block itself~~ session used in the *finally*
>> block. ~~This raises a warning that is suppressed using
>> `@SuppressWarnings("try")`. In the future, we might be able to remove these
>> suppressions by using the reserved variable name `_`.~~
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove session exposure
Marked as reviewed by psandoz (Reviewer).
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: JDK-8277074: Qualified exported types show up in JavaDoc
On Tue, 15 Nov 2022 12:15:42 GMT, Hannes Wallnöfer wrote: > Please review a simple change to hide internal classes in generated > documentation by adding doc comments containing a `@hidden` tag. I verified > the fix by making sure `grep -r jdk.internal` returns no matches in the > generated documentation tree. Changes for the Vector API are good. - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.org/jdk/pull/11163
Re: RFR: 8296896: Change virtual Thread.yield to use external submit
On Tue, 6 Dec 2022 10:16:38 GMT, Alan Bateman wrote: > The implementation of Thread.yield for virtual threads is currently a "lazy > submit". This means the task for the virtual thread is queued to the > carrier/worker local queue without signalling other threads. This behavior > can be surprising/unfair when there are tasks in the submission queues, say > when a platform thread has started or unparked a virtual thread. > > Ron, Doug Lea, Viktor Klang and I have discussed this topic and propose to > change Thread.yield to use "external submit" when the local task queue is > empty, and to push to the local queue when not empty. The change improves the > fairness but will of course increase the chances that repeated Thread.yield > will bounce between carriers. test/jdk/java/lang/Thread/virtual/YieldQueuing.java line 43: > 41: > 42: /** > 43: * Test Thread.yield submits the task for the current virtyal thread > to a scheduler Suggestion: * Test Thread.yield submits the task for the current virtual thread to a scheduler - PR: https://git.openjdk.org/jdk/pull/11533
Re: [jdk20] RFR: JDK-8277074: Qualified exported types show up in JavaDoc
On Mon, 19 Dec 2022 16:28:17 GMT, Hannes Wallnöfer wrote: > This `noreg-doc` PR has been moved over from the mainline repository, the > original PR is openjdk/jdk#11163. > > The purpose is to hide internal classes in generated documentation by adding > doc comments containing a @hidden tag. I verified the fix by making sure the > internal vector and event classes no longer show up in the generated > documentation tree. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk20/pull/56
Re: RFR: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle
On Thu, 19 Jan 2023 19:14:38 GMT, Mandy Chung wrote: > `VarHandles.getStaticFieldFromBaseAndOffset` maps a base/offset/fieldType to > a static `Field`. It's fragile to assume that the location of a static > field returned by `Unsafe.staticFieldBase` is a Class object.This changes > the VarHandle implementation for static fields (i.e. `FieldStaticReadOnly` > and `FieldStaticReadWrite` classes) to include the receiver type in addition > to the base and offset. src/java.base/share/classes/java/lang/invoke/VarHandles.java line 187: > 185: // Required by instance static field handles > 186: static Field getStaticFieldFromBaseAndOffset(Class receiverType, > 187: Object base, `base` is now unused. - PR: https://git.openjdk.org/jdk/pull/12100
Re: RFR: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle [v2]
On Thu, 19 Jan 2023 23:34:04 GMT, Mandy Chung wrote: >> `VarHandles.getStaticFieldFromBaseAndOffset` maps a base/offset/fieldType to >> a static `Field`. It's fragile to assume that the location of a static >> field returned by `Unsafe.staticFieldBase` is a Class object.This >> changes the VarHandle implementation for static fields (i.e. >> `FieldStaticReadOnly` and `FieldStaticReadWrite` classes) to include the >> receiver type in addition to the base and offset. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > remove the base parameter which is unused Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12100
Re: RFR: 8300693: Lower the compile threshold and reduce the iterations of warmup loop in VarHandles tests [v2]
On Fri, 20 Jan 2023 17:45:46 GMT, Mandy Chung wrote: >> `java/lang/invoke/VarHandles` tests run with C1, C2 and tiered compilations >> and the test cases are executed in the warm up loop with 2 iterations to >> verify C1, C2 intrinsics. Default Tier4CompileThreshold is 15000. >> >> This PR proposes to scale the compile threshold to 0.1 such that the warm up >> loop can be reduced to 2000 iterations. This will speed up the test >> execution time. >> >> >> Before: >> make test-only TEST=open/test/jdk/java/lang/invoke/VarHandles 341.06s user >> 14.65s system 563% cpu 1:03.14 total >> >> After: >> make test-only TEST=open/test/jdk/java/lang/invoke/VarHandles 234.38s user >> 13.08s system 535% cpu 46.218 total > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > move the comment to @comment Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12104
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module
On Thu, 26 Jan 2023 21:03:59 GMT, Mandy Chung wrote:
> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a
> class in the runtime package of the Lookup's lookup class via
> `Lookup::defineClass`. The classes that are injected have the same access
> as other members in the module and can access private members of all types in
> the module via reflection.
>
> However, changing `Lookup.defineClass` to require full privilege access
> (`PRIVATE` + `MODULE`) is an incompatible change that would break existing
> frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject
> auxiliary classes in a module. A module authorizes the framework by opening
> a package for it to access and `Lookup::defineClass` was the supported
> replacement for `setAccessible` on `ClassLoader::defineClass` hack in JDK 9.
>
>
> This PR proposes to keep existing behavior and provide better documentation
> to help developers to beware of the permissions given out when opening a
> package to another module. A class injected in a module has the same
> privilege as other module members.
src/java.base/share/classes/java/lang/Module.java line 607:
> 605: * {@link java.lang.invoke.MethodHandles.Lookup Lookup} object that
> can be used to
> 606: * {@link java.lang.invoke.MethodHandles.Lookup#defineClass(byte[])
> inject classes}
> 607: * in package {@code p}.
Suggestion:
* A package {@code p} opened to module {@code M} means that code in
* {@code M} is allowed to do deep reflection on all types in the package.
* Further, if {@code M} reads this module it can obtain a
* {@link java.lang.invoke.MethodHandles.Lookup Lookup} object that is
allowed to
* {@link java.lang.invoke.MethodHandles.Lookup#defineClass(byte[]) define
classes}
* in package {@code p}.
Trying to reuse existing terms. I am presuming deep reflection implies on all
members and setAccessible so there is no need to mention it?
Also i don't see "inject" used in existing text, so just reuse "define"?
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 240:
> 238: * of {@code targetClass}. Extreme caution should be taken when
> opening a package
> 239: * to another module. The injected classes have the same full
> privilege
> 240: * access as other members in the target module.
Suggestion:
* The {@code Lookup} object returned by this method is allowed to
* {@linkplain Lookup#defineClass(byte[]) define classes} in the runtime
package
* of {@code targetClass}. Extreme caution should be taken when opening a
package
* to another module as such defined classes have the same full privilege
* access as other members in the target module.
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 883:
> 881: * of {@code T}. Extreme caution should be taken when opening a
> package
> 882: * to another module. The injected classes have the same full
> privilege
> 883: * access as other members in the target module.
Suggestion:
*
* The {@code Lookup} object returned by {@code privateLookupIn} is allowed
to
* {@linkplain Lookup#defineClass(byte[]) define classes} in the runtime
package
* of {@code T}. Extreme caution should be taken when opening a package
* to another module as such defined classes have the same full privilege
* access as other members in the target module.
You mention "target module" but i don't think i it is defined for the Lookup
class JavaDoc. In this case are we referring to module M2?
-
PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v2]
On Thu, 26 Jan 2023 22:27:36 GMT, Mandy Chung wrote: >> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a >> class in the runtime package of the Lookup's lookup class via >> `Lookup::defineClass`. The classes that are injected have the same access >> as other members in the module and can access private members of all types >> in the module via reflection. >> >> However, changing `Lookup.defineClass` to require full privilege access >> (`PRIVATE` + `MODULE`) is an incompatible change that would break existing >> frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject >> auxiliary classes in a module. A module authorizes the framework by >> opening a package for it to access and `Lookup::defineClass` was the >> supported replacement for `setAccessible` on `ClassLoader::defineClass` hack >> in JDK 9. >> >> This PR proposes to keep existing behavior and provide better documentation >> to help developers to beware of the permissions given out when opening a >> package to another module. A class injected in a module has the same >> privilege as other module members. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback Looks good. All changes seem informational explaining the existing behavior so no CSR? - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8301190: [vectorapi] The typeChar of LaneType is incorrect when default locale is tr
On Thu, 26 Jan 2023 21:56:22 GMT, Glavo wrote: > When the default Locale is tr, the letter `i` will be converted to `İ` > (U+0130) by `toUpperCase()`. This causes the assertion to fail. Looks good. I am grateful the assert was in place. - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.org/jdk/pull/12238
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v4]
On Fri, 27 Jan 2023 18:20:42 GMT, Mandy Chung wrote: >> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a >> class in the runtime package of the Lookup's lookup class via >> `Lookup::defineClass`. The classes that are injected have the same access >> as other members in the module and can access private members of all types >> in the module via reflection. >> >> However, changing `Lookup.defineClass` to require full privilege access >> (`PRIVATE` + `MODULE`) is an incompatible change that would break existing >> frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject >> auxiliary classes in a module. A module authorizes the framework by >> opening a package for it to access and `Lookup::defineClass` was the >> supported replacement for `setAccessible` on `ClassLoader::defineClass` hack >> in JDK 9. >> >> This PR proposes to keep existing behavior and provide better documentation >> to help developers to beware of the permissions given out when opening a >> package to another module. A class injected in a module has the same >> privilege as other module members. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > fine tune wording Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8293198: [vectorapi] Improve the implementation of VectorMask.indexInRange() [v2]
On Fri, 3 Feb 2023 07:13:10 GMT, Xiaohong Gong wrote:
>> The Vector API `"indexInRange(int offset, int limit)"` is used
>> to compute a vector mask whose lanes are set to true if the
>> index of the lane is inside the range specified by the `"offset"`
>> and `"limit"` arguments, otherwise the lanes are set to false.
>>
>> There are two special cases for this API:
>> 1) If `"offset >= 0 && offset >= limit"`, all the lanes of the
>> generated mask are false.
>> 2) If` "offset >= 0 && limit - offset >= vlength"`, all the
>> lanes of the generated mask are true. Note that `"vlength"` is
>> the number of vector lanes.
>>
>> For such special cases, we can simply use `"maskAll(false|true)"`
>> to implement the API. Otherwise, the original comparison with
>> `"iota" `vector is needed. And for further optimization, we have
>> optimal instruction supported by SVE (i.e. whilelo [1]), which
>> can implement the API directly if the `"offset >= 0"`.
>>
>> As a summary, to optimize the API, we can use the if-else branches
>> to handle the specific cases in java level and intrinsify the
>> remaining case by C2 compiler:
>>
>>
>> public VectorMask indexInRange(int offset, int limit) {
>> if (offset < 0) {
>> return this.and(indexInRange0Helper(offset, limit));
>> } else if (offset >= limit) {
>> return this.and(vectorSpecies().maskAll(false));
>> } else if (limit - offset >= length()) {
>> return this.and(vectorSpecies().maskAll(true));
>> }
>> return this.and(indexInRange0(offset, limit));
>> }
>>
>>
>> The last part (i.e. `"indexInRange0"`) in the above implementation
>> is expected to be intrinsified by C2 compiler if the necessary IRs
>> are supported. Otherwise, it will fall back to the original API
>> implementation (i.e. `"indexInRange0Helper"`). Regarding to the
>> intrinsifaction, the compiler will generate `"VectorMaskGen"` IR
>> with "limit - offset" as the input if the current platform supports
>> it. Otherwise, it generates `"VectorLoadConst + VectorMaskCmp"` based
>> on `"iota < limit - offset"`.
>>
>> For the following java code which uses `"indexInRange"`:
>>
>>
>> static final VectorSpecies SPECIES =
>>DoubleVector.SPECIES_PREFERRED;
>> static final int LENGTH = 1027;
>>
>> public static double[] da;
>> public static double[] db;
>> public static double[] dc;
>>
>> private static void func() {
>> for (int i = 0; i < LENGTH; i += SPECIES.length()) {
>> var m = SPECIES.indexInRange(i, LENGTH);
>> var av = DoubleVector.fromArray(SPECIES, da, i, m);
>> av.lanewise(VectorOperators.NEG).intoArray(dc, i, m);
>> }
>> }
>>
>>
>> The core code generated with SVE 256-bit vector size is:
>>
>>
>> ptrue p2.d ; maskAll(true)
>> ...
>> LOOP:
>> ...
>> sub w11, w13, w14 ; limit - offset
>> cmp w14, w13
>> b.csLABEL-1 ; if (offset >= limit) => uncommon-trap
>> cmp w11, #0x4
>> b.ltLABEL-2 ; if (limit - offset < vlength)
>> mov p1.b, p2.b
>> LABEL-3:
>> ld1d{z16.d}, p1/z, [x10] ; load vector masked
>> ...
>> cmp w14, w29
>> b.ccLOOP
>> ...
>> LABEL-2:
>> whilelo p1.d, x16, x10; VectorMaskGen
>> ...
>> b LABEL-3
>> ...
>> LABEL-1:
>> uncommon-trap
>>
>>
>> Please note that if the array size `LENGTH` is aligned with
>> the vector size 256 (i.e. `LENGTH = 1024`), the branch "LABEL-2"
>> will be optimized out by compiler and it becomes another
>> uncommon-trap.
>>
>> For NEON, the main CFG is the same with above. But the compiler
>> intrinsification is different. Here is the code:
>>
>>
>> sub x10, x10, x12 ; limit - offset
>> scvtf d16, x10
>> dup v16.2d, v16.d[0] ; replicateD
>>
>> mov x8, #0xd8d0
>> movkx8, #0x84cb, lsl #16
>> movkx8, #0x, lsl #32
>> ldr q17, [x8], #0 ; load the "iota" const vector
>> fcmgt v18.2d, v16.2d, v17.2d ; mask = iota < limit - offset
>>
>>
>> Here is the performance data of the new added benchmark on an ARM
>> SVE 256-bit platform:
>>
>>
>> Benchmark (size) BeforeAfter Units
>> IndexInRangeBenchmark.byteIndexInRange 1024 11203.697 41404.431 ops/ms
>> IndexInRangeBenchmark.byteIndexInRange 1027 2365.920 8747.004 ops/ms
>> IndexInRangeBenchmark.doubleIndexInRange 1024 1227.505 6092.194 ops/ms
>> IndexInRangeBenchmark.doubleIndexInRange 1027 351.215 1156.683 ops/ms
>> IndexInRangeBenchmark.floatIndexInRange 1024 1468.876 11032.580 ops/ms
>> IndexInRangeBenchmark.floatIndexInRange 1027 699.645 2439.671 ops/ms
>> IndexInRangeBenchmark.intIndexInRange1024 2842.187 11903.544 ops/ms
>> IndexInRangeBenchmark.intIndexInRange1027 689.866 2547.424 ops/ms
>> IndexInRangeBenchmark.longIndexInRange 1024 1394.135 5902.973 ops/ms
>> IndexInRangeBenchmark.longIndexInRange 102
Re: RFR: 8293198: [vectorapi] Improve the implementation of VectorMask.indexInRange() [v3]
On Tue, 7 Feb 2023 09:51:19 GMT, Xiaohong Gong wrote:
>> The Vector API `"indexInRange(int offset, int limit)"` is used
>> to compute a vector mask whose lanes are set to true if the
>> index of the lane is inside the range specified by the `"offset"`
>> and `"limit"` arguments, otherwise the lanes are set to false.
>>
>> There are two special cases for this API:
>> 1) If `"offset >= 0 && offset >= limit"`, all the lanes of the
>> generated mask are false.
>> 2) If` "offset >= 0 && limit - offset >= vlength"`, all the
>> lanes of the generated mask are true. Note that `"vlength"` is
>> the number of vector lanes.
>>
>> For such special cases, we can simply use `"maskAll(false|true)"`
>> to implement the API. Otherwise, the original comparison with
>> `"iota" `vector is needed. And for further optimization, we have
>> optimal instruction supported by SVE (i.e. whilelo [1]), which
>> can implement the API directly if the `"offset >= 0"`.
>>
>> As a summary, to optimize the API, we can use the if-else branches
>> to handle the specific cases in java level and intrinsify the
>> remaining case by C2 compiler:
>>
>>
>> public VectorMask indexInRange(int offset, int limit) {
>> if (offset < 0) {
>> return this.and(indexInRange0Helper(offset, limit));
>> } else if (offset >= limit) {
>> return this.and(vectorSpecies().maskAll(false));
>> } else if (limit - offset >= length()) {
>> return this.and(vectorSpecies().maskAll(true));
>> }
>> return this.and(indexInRange0(offset, limit));
>> }
>>
>>
>> The last part (i.e. `"indexInRange0"`) in the above implementation
>> is expected to be intrinsified by C2 compiler if the necessary IRs
>> are supported. Otherwise, it will fall back to the original API
>> implementation (i.e. `"indexInRange0Helper"`). Regarding to the
>> intrinsifaction, the compiler will generate `"VectorMaskGen"` IR
>> with "limit - offset" as the input if the current platform supports
>> it. Otherwise, it generates `"VectorLoadConst + VectorMaskCmp"` based
>> on `"iota < limit - offset"`.
>>
>> For the following java code which uses `"indexInRange"`:
>>
>>
>> static final VectorSpecies SPECIES =
>>DoubleVector.SPECIES_PREFERRED;
>> static final int LENGTH = 1027;
>>
>> public static double[] da;
>> public static double[] db;
>> public static double[] dc;
>>
>> private static void func() {
>> for (int i = 0; i < LENGTH; i += SPECIES.length()) {
>> var m = SPECIES.indexInRange(i, LENGTH);
>> var av = DoubleVector.fromArray(SPECIES, da, i, m);
>> av.lanewise(VectorOperators.NEG).intoArray(dc, i, m);
>> }
>> }
>>
>>
>> The core code generated with SVE 256-bit vector size is:
>>
>>
>> ptrue p2.d ; maskAll(true)
>> ...
>> LOOP:
>> ...
>> sub w11, w13, w14 ; limit - offset
>> cmp w14, w13
>> b.csLABEL-1 ; if (offset >= limit) => uncommon-trap
>> cmp w11, #0x4
>> b.ltLABEL-2 ; if (limit - offset < vlength)
>> mov p1.b, p2.b
>> LABEL-3:
>> ld1d{z16.d}, p1/z, [x10] ; load vector masked
>> ...
>> cmp w14, w29
>> b.ccLOOP
>> ...
>> LABEL-2:
>> whilelo p1.d, x16, x10; VectorMaskGen
>> ...
>> b LABEL-3
>> ...
>> LABEL-1:
>> uncommon-trap
>>
>>
>> Please note that if the array size `LENGTH` is aligned with
>> the vector size 256 (i.e. `LENGTH = 1024`), the branch "LABEL-2"
>> will be optimized out by compiler and it becomes another
>> uncommon-trap.
>>
>> For NEON, the main CFG is the same with above. But the compiler
>> intrinsification is different. Here is the code:
>>
>>
>> sub x10, x10, x12 ; limit - offset
>> scvtf d16, x10
>> dup v16.2d, v16.d[0] ; replicateD
>>
>> mov x8, #0xd8d0
>> movkx8, #0x84cb, lsl #16
>> movkx8, #0x, lsl #32
>> ldr q17, [x8], #0 ; load the "iota" const vector
>> fcmgt v18.2d, v16.2d, v17.2d ; mask = iota < limit - offset
>>
>>
>> Here is the performance data of the new added benchmark on an ARM
>> SVE 256-bit platform:
>>
>>
>> Benchmark (size) BeforeAfter Units
>> IndexInRangeBenchmark.byteIndexInRange 1024 11203.697 41404.431 ops/ms
>> IndexInRangeBenchmark.byteIndexInRange 1027 2365.920 8747.004 ops/ms
>> IndexInRangeBenchmark.doubleIndexInRange 1024 1227.505 6092.194 ops/ms
>> IndexInRangeBenchmark.doubleIndexInRange 1027 351.215 1156.683 ops/ms
>> IndexInRangeBenchmark.floatIndexInRange 1024 1468.876 11032.580 ops/ms
>> IndexInRangeBenchmark.floatIndexInRange 1027 699.645 2439.671 ops/ms
>> IndexInRangeBenchmark.intIndexInRange1024 2842.187 11903.544 ops/ms
>> IndexInRangeBenchmark.intIndexInRange1027 689.866 2547.424 ops/ms
>> IndexInRangeBenchmark.longIndexInRange 1024 1394.135 5902.973 ops/ms
>> IndexInRangeBenchmark.longIndexInRange 102
Re: RFR: 8302260: VarHandle.describeConstable() fails to return a nominal descriptor for static public fields
On Mon, 13 Feb 2023 19:35:52 GMT, Mandy Chung wrote: > I overlooked in the fix for JDK-8297757 that it should have passed the > declaring class of the static fields rather than the reference class passed > to `Lookup::findStaticVarHandle`. Doh! I think the previous fix may have also result in an InternalError in some cases? since the field may not be found from the declaring class in `VarHandles::getStaticFieldFromBaseAndOffset`? - PR: https://git.openjdk.org/jdk/pull/12543
Re: RFR: 8302260: VarHandle.describeConstable() fails to return a nominal descriptor for static public fields
On Mon, 13 Feb 2023 19:35:52 GMT, Mandy Chung wrote: > I overlooked in the fix for JDK-8297757 that it should have passed the > declaring class of the static fields rather than the reference class passed > to `Lookup::findStaticVarHandle`. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12543
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask
On Tue, 31 Jan 2023 10:57:58 GMT, Viktor Klang wrote: > I noticed when looking at the code that there was no real need to use a CHM > to perform the tracking of activation in an ordered fashion on > ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations > and indirection. src/java.base/share/classes/java/util/stream/ForEachOps.java line 431: > 429: // leftChild and rightChild were just created and not > fork():ed > 430: // yet so no need for a volatile write > 431: NEXT.set(leftChild, rightChild); Make `next` a plain field and shuffle up the assignment (`leftChild.next = rightChild`) to occur immediately after construction of the right child task? (FWIW `addToPendingCount` operates on a volatile field of `CountedCompleter`). - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask
On Thu, 16 Feb 2023 19:44:35 GMT, Viktor Klang wrote: >> src/java.base/share/classes/java/util/stream/ForEachOps.java line 431: >> >>> 429: // leftChild and rightChild were just created and not >>> fork():ed >>> 430: // yet so no need for a volatile write >>> 431: NEXT.set(leftChild, rightChild); >> >> Make `next` a plain field and shuffle up the assignment (`leftChild.next = >> rightChild`) to occur immediately after construction of the right child >> task? (FWIW `addToPendingCount` operates on a volatile field of >> `CountedCompleter`). > > @PaulSandoz I'm usually a bit weary of piggybacking if it is not done on the > same object, as future reorderings of the code might break that assumption. I > wouldn't want to break anything silently so I made a rather conservative > change. On which side should I err? :) Since as you have said the left and right nodes have yet to be "published" (outside of their little nest) I think it should be fine to move it above and next to the constructions. (Also since `addToPendingCount` has volatile write semantics, via it's getAndSet, the plain write should not float below that call if that really matters, and its hard to see the code radically changing in this regard unless there is a major rewrite than i guess it all has to be carefully rethought.) - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v2]
On Fri, 17 Feb 2023 11:39:21 GMT, Viktor Klang wrote: >> I noticed when looking at the code that there was no real need to use a CHM >> to perform the tracking of activation in an ordered fashion on >> ForEachOrderedTask, but instead a VarHandle can be used, reducing >> allocations and indirection. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Write the initial value of the next reference without using the VarHandle That's a nice find, looks good. (Update the year in the copyright header.) - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v5]
On Tue, 28 Feb 2023 11:09:37 GMT, Viktor Klang wrote: >> I noticed when looking at the code that there was no real need to use a CHM >> to perform the tracking of activation in an ordered fashion on >> ForEachOrderedTask, but instead a VarHandle can be used, reducing >> allocations and indirection. > > Viktor Klang has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Updating copyright header of ForEachOps.java and removing unnecessary > suppression of an unchecked cast. src/java.base/share/classes/java/util/stream/ForEachOps.java line 513: > 511: // of right subtree (if any, which can be this task's right > sibling) > 512: // > 513: var leftDescendant = (ForEachOrderedTask T>)NEXT.getAndSet(this, (ForEachOrderedTask)null); The reference cast on the argument is not required. `VarHandle`'s by default have `MethodHandle` invoke semantics (but without the throwing Throwable): https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html#invoke VarHandle's have been engineered so such reference casts on the arguments can be optimized away. This makes them much easier to use than MethodHandles. - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v3]
On Tue, 28 Feb 2023 23:08:29 GMT, Eirik Bjorsnos wrote:
>> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set
>> of benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark
>> serves as an example of how vectorization can be useful also in the area of
>> text processing. It takes advantage of the fact that ASCII and Latin-1 were
>> designed to optimize case-twiddling operations.
>>
>> The code came about during the work on #12632, where vectorization was
>> deemed out of scope.
>>
>> Benchmark results:
>>
>>
>> Benchmark (size) Mode Cnt Score Error
>> Units
>> EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493
>> ns/op
>
> Eirik Bjorsnos has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Uppercase Thorn is 0xDE
> - Update 'a' to 'A' and 'z' to 'Z' in comments
test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
line 67:
> 65: public void scalar(Blackhole blackhole) {
> 66: blackhole.consume(scalarEqualsIgnoreCase(a, b, len));
> 67: }
If you like there is no need to explicitly use a black hole, instead declare
the benchmark method to return `boolean` and return the result of the call. JMH
will do the right thing. I find that a little more concise and easier to read.
-
PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v7]
On Wed, 1 Mar 2023 14:28:55 GMT, Eirik Bjorsnos wrote:
>> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set
>> of benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark
>> serves as an example of how vectorization can be useful also in the area of
>> text processing. It takes advantage of the fact that ASCII and Latin-1 were
>> designed to optimize case-twiddling operations.
>>
>> The code came about during the work on #12632, where vectorization was
>> deemed out of scope.
>>
>> Benchmark results:
>>
>>
>> Benchmark (size) Mode Cnt Score Error
>> Units
>> EqualsIgnoreCaseBenchmark.scalar 16 avgt 1521.575 ± 0.365
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 32 avgt 1543.199 ± 2.786
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 64 avgt 1571.389 ± 0.891
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 146.827 ± 1.115
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1062.759 ± 9.331
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.483 ± 0.121
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1510.538 ± 0.125
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.182 ± 0.177
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1519.192 ± 0.421
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized1024 avgt 1592.521 ± 3.494
>> ns/op
>
> Eirik Bjorsnos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use GT, LT, EQ operations since they seem to outperform GE, LE, NE.
Benchmark looks good.
I think you need to look at the generated code and perf data to get more
insight into the differences you are observing.
Sometimes array alignment can add variance to the results (which may not be the
case here), and to rule that out i sometimes run vector tests with the HotSpot
option `-XX:ObjectAlignmentInBytes` e.g. `-XX:ObjectAlignmentInBytes=32`.
test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
line 111:
> 109: .or(letter.and(upperA.eq(upperB)));
> 110:
> 111: if(equalsIgnoreCase.allTrue()) {
Suggestion:
if (equalsIgnoreCase.allTrue()) {
-
PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v8]
On Wed, 1 Mar 2023 17:48:46 GMT, Eirik Bjorsnos wrote: >> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set >> of benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark >> serves as an example of how vectorization can be useful also in the area of >> text processing. It takes advantage of the fact that ASCII and Latin-1 were >> designed to optimize case-twiddling operations. >> >> The code came about during the work on #12632, where vectorization was >> deemed out of scope. >> >> Benchmark results: >> >> >> Benchmark (size) Mode Cnt Score Error >> Units >> EqualsIgnoreCaseBenchmark.scalar 16 avgt 1521.575 ± 0.365 >> ns/op >> EqualsIgnoreCaseBenchmark.scalar 32 avgt 1543.199 ± 2.786 >> ns/op >> EqualsIgnoreCaseBenchmark.scalar 64 avgt 1571.389 ± 0.891 >> ns/op >> EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 146.827 ± 1.115 >> ns/op >> EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1062.759 ± 9.331 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.483 ± 0.121 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1510.538 ± 0.125 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.182 ± 0.177 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1519.192 ± 0.421 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized1024 avgt 1592.521 ± 3.494 >> ns/op > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Add whitespace after if Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - renamed all remaining ConcreteXyzEntry to XyzEntryImpl > - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to > AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry > - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl > - ConcreteEntry renamed to AbstractPoolEntry src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java line 111: > 109: Set dependencies = cm.elementStream() > 110: .filter(ce -> ce instanceof > MethodModel) > 111: .flatMap(ce -> ((MethodModel) > ce).elementStream()) You could potentially collapse this into a single `flatMap` and avoid the cast: .flatMap(ce -> ce instanceof MethodMethod mm ? mm.elementStream() : Stream.empty()) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
> - ConcreteEntry renamed to AbstractPoolEntry
src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
line 48:
> 46: default ConstantDesc constantValue() {
> 47: return asSymbol();
> 48: }
It seems possible to use this pattern consistently for `ConstantDynamicEntry`,
`MethodHandleEntry`, and `MethodTypeEntry`?
At first i thought why not make `asSymbol` a covariant override of
`constantValue`, but its not the same thing, since there are constant values
(subtypes of `ConstantValueEntry`) that are not symbols.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
> - ConcreteEntry renamed to AbstractPoolEntry
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java
line 376:
> 374: } else if (o instanceof Utf8Entry u) {
> 375: return equalsString(u.stringValue());
> 376: }
Dead branch? since there is only one concrete implementation of `Utf8Entry`.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - renamed all remaining ConcreteXyzEntry to XyzEntryImpl > - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to > AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry > - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl > - ConcreteEntry renamed to AbstractPoolEntry src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPool.java line 49: > 47: import static jdk.internal.classfile.Classfile.TAG_STRING; > 48: import static jdk.internal.classfile.Classfile.TAG_UTF8; > 49: Unused imports, perhaps sweep through all classes (i think it can be done over multiple packages with IntelliJ). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
> - ConcreteEntry renamed to AbstractPoolEntry
src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
line 71:
> 69: * @return the combined {@link List}
> 70: */
> 71: static List adding(List base,
> List additions) {
This and all the other following static methods seem more like implementation
details rather than part of the public API?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - renamed all remaining ConcreteXyzEntry to XyzEntryImpl > - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to > AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry > - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl > - ConcreteEntry renamed to AbstractPoolEntry src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java line 222: > 220: * @param typeEntry the member field or method descriptor > 221: */ > 222: NameAndTypeEntry natEntry(Utf8Entry nameEntry, Utf8Entry typeEntry); I would be inclined to rename more literally as `nameAndTypeEntry`, which is consistent with the naming convention in all other cases in this interface AFAICT (edit: almost i also see `bsm`) . (FWIW i keep reading nat as "not a type"!) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
> - ConcreteEntry renamed to AbstractPoolEntry
src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
line 167:
> 165: buf.patchInt(pos + 2, 4, attrLen - 6);
> 166: buf.patchInt(pos + 6, 2, bsmSize);
> 167: return true;
The if and else branch return true, factor out at the end of the method?
src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
line 339:
> 337: }
> 338:
> 339: private AbstractPoolEntry.Utf8EntryImpl tryFindUtf8(int hash,
> String target) {
Unused type variable `T`
src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
line 488:
> 486: return methodHandleEntry(refKind, reference);
> 487: }
> 488: return internalAdd(new
> AbstractPoolEntry.MethodHandleEntryImpl(this, size, hash, refKind,
> (AbstractPoolEntry.AbstractMemberRefEntry) reference), hash);
Break the long line (same for two following methods).
src/java.base/share/classes/jdk/internal/classfile/impl/TemporaryConstantPool.java
line 56:
> 54: public final class TemporaryConstantPool implements ConstantPoolBuilder {
> 55:
> 56: private TemporaryConstantPool() {};
Suggestion:
private TemporaryConstantPool() {}
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
> - ConcreteEntry renamed to AbstractPoolEntry
src/java.base/share/classes/jdk/internal/classfile/ClassReader.java line 270:
> 268: int classReaderOffset,
> 269: int length);
> 270: }
Suggestion:
}
src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
line 90:
> 88: private BootstrapMethodsAttribute bootstrapMethodsAttribute;
> 89:
> 90: @SuppressWarnings("unchecked")
Is this needed?
src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
line 115:
> 113:
> 114: // 4
> 115: case TAG_CONSTANTDYNAMIC, TAG_FIELDREF, TAG_FLOAT,
> TAG_INTEGER, TAG_INTERFACEMETHODREF, TAG_INVOKEDYNAMIC, TAG_METHODREF,
> TAG_NAMEANDTYPE -> p += 4;
Break the line
src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
line 132:
> 130: this.cp = new PoolEntry[constantPoolCount];
> 131:
> 132: p = metadataStart;
Redundant assignment (see line 127).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with four additional > commits since the last revision: > > - renamed all remaining ConcreteXyzEntry to XyzEntryImpl > - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to > AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry > - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl > - ConcreteEntry renamed to AbstractPoolEntry src/java.base/share/classes/jdk/internal/classfile/ClassReader.java line 144: > 142: * constant pool size, or zero > 143: * @throws ClassCastException if the index does not correspond to > 144: * a module entry Throwing `ClassCastException` is certainly convenient from an implementation perspective, but I think it is better to throw `IllegalArgumentException`. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
> - ConcreteEntry renamed to AbstractPoolEntry
src/java.base/share/classes/jdk/internal/classfile/attribute/CodeAttribute.java
line 56:
> 54: * @param label a marker for a position within this {@code
> CodeAttribute}
> 55: * @return position of the {@code Label} in the {@code codeArray}
> 56: */
Suggestion:
/**
* {@return the position of the {@code Label} in the {@code codeArray}}
* @param label a marker for a position within this {@code CodeAttribute}
*/
Throws IAE if the label is not positioned in the code array?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v13]
On Wed, 8 Feb 2023 11:00:14 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/attribute/StackMapTableAttribute.java
>> line 68:
>>
>>> 66: * A simple stack value.
>>> 67: */
>>> 68: public enum SimpleVerificationTypeInfo implements
>>> VerificationTypeInfo {
>>
>> I note that in this class we have made the decision to model all the innards
>> (XYZInfo) as nested classes - while in all the other cases XYZInfo are
>> toplevel types. Moving forward, we should pick something consistent.
>
> Every case has been considered individually, evaluated on use cases and pros
> and cons have been weighted. Unified approach across the whole API would be
> nice, however not so simple and not the highest priority.
I had the same observation as Maurizio.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 13:50:07 GMT, Adam Sotona wrote: >> Actually, we also have a ClassfileVersion class, so that could be a better >> place for version numbers? > > There were several iterations of "where to store numeric constants". > It is hard to find them when spread across many classes and duplicities > appear instantly. > Dedicated "Constants" would be another bikeshed with conflicting name. > Co-location in Classfile was the latest decission as it is not just a central > bikeshed, but it also reflect connection with classfile specification. > Please raise that discussion at classfile-api-dev at openjdk.org if necessary. At least for versions its easier to have a singular location because every six months the code needs to be updated. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
> - ConcreteEntry renamed to AbstractPoolEntry
src/java.base/share/classes/jdk/internal/classfile/ClassModel.java line 86:
> 84: * {@snippet lang=java :
> 85: * Classfile.build(thisClass(), ConstantPoolBuilder.of(this),
> 86: * b -> b.transform(this, transform);
Suggestion:
* Classfile.build(thisClass(), ConstantPoolBuilder.of(this),
* b -> b.transform(this, transform));
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v4]
On Tue, 28 Feb 2023 10:53:14 GMT, Viktor Klang wrote: >> src/java.base/share/classes/java/util/stream/ForEachOps.java line 513: >> >>> 511: // of right subtree (if any, which can be this task's >>> right sibling) >>> 512: // >>> 513: var leftDescendant = (ForEachOrderedTask>> T>)NEXT.getAndSet(this, null); >> >> Casting the `null` is required for the resolved method descriptor to be >> `(ForEachOrderedTask, ForEachOrderedTask)ForEachOrderedTask` instead of >> `(ForEachOrderedTask, Object)ForEachOrderedTask`, which prevents unnecessary >> type conversion `LambdaForm`s from being introduced and allows >> [`VarHandle::withInvokeExactBehavior`] to be used: >> Suggestion: >> >> var leftDescendant = (ForEachOrderedTask) >> NEXT.getAndSet(this, (ForEachOrderedTask) null); >> >> >> [`VarHandle::withInvokeExactBehavior`]: >> https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html#withInvokeExactBehavior() > > @ExE-Boss Ah, sorry, it was meant to be there. :) VarHandles are more optimized than MethodHandles when the signature at the call site does not match the signature of the handle. For referenced casts of arguments VarHandles avoid the `asType` conversion that would occur with method handles. This makes it much easier to write/read. VarHandle::withInvokeExactBehavior was introduced to deal with cases where an exact signature match is required. Sometimes this is useful to avoid performance potholes resulting from unintended primitive casts/conversions, where VarHandles otherwise resort to `asType` conversion. - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: 8294982: Implementation of Classfile API [v13]
On Thu, 2 Mar 2023 14:29:13 GMT, Adam Sotona wrote: >> I had the same observation as Maurizio. > > I've extracted `StackMapFrameInfo` to the top level and moved > `VerificationTypeInfo`, however it is still nested. > Let me know if you think we should really avoid all nested or if info inside > info is OK. > Thanks. That seems reasonable (top level `StackMapFrameInfo` with nesting for less important stuff.) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java
line 157:
> 155: if (!(ce instanceof
> MethodModel mm
> 156: &&
> mm.methodName().stringValue().startsWith("debug")))
> 157: classBuilder.with(ce);
Indentation is confusing (even though its less concise its probably better to
use braces for all the examples):
Suggestion:
classBuilder.with(ce);
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java
line 98:
> 96: @Override
> 97: public void accept(CodeBuilder cob, CodeElement coe) {
> 98: switch (coe) {
Missing case for`CharacterRange` instruction? I am not sure it makes any sense,
if so perhaps add a comment as to why.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
line 42:
> 40: * the setting of the {@link Classfile.Option#processDebug(boolean)}
> option.
> 41: */
> 42: public sealed interface CharacterRange extends PseudoInstruction
This and some other instructions with unbounded implementations do not have
static `of` factory methods. Is that intended?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > StackMapFrameInfo extracted to top level from StackMapTableAttribute src/java.base/share/classes/jdk/internal/classfile/components/snippet-files/PackageSnippets.java line 171: > 169: > 170: //store stacked method > parameters into locals > 171: var storeStack = new > LinkedList(); FWIW you can use an ArrayDeque + push + forEach, in the spirit of highlighting Deque over LinkedList for stack-based usage (since this is an example with visibility). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > StackMapFrameInfo extracted to top level from StackMapTableAttribute src/java.base/share/classes/jdk/internal/classfile/components/snippet-files/PackageSnippets.java line 149: > 147: var instrumentorCodeMap = instrumentor.methods().stream() > 148: > .filter(instrumentedMethodsFilter) > 149: > .collect(Collectors.toMap(mm -> mm.methodName().stringValue() + > mm.methodType().stringValue(), mm -> mm.code().orElse(null))); Should we be filtering out abstract methods before the `collect` and/or replace with: mm -> mm.code().orElseThrow() ? - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java
line 41:
> 39: * Models a constant-load instruction in the {@code code} array of a
> {@code
> 40: * Code} attribute, including "intrinsic constant" instructions (e.g.,
> {@code
> 41: * aload_0}), "argument constant" instructions (e.g., {@code bipush}),
> and "load
Suggestion:
* iconst_0}), "argument constant" instructions (e.g., {@code bipush}), and
"load
Otherwise we can refer to `aconst_null`
src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java
line 60:
> 58: /**
> 59: * Models an "intrinsic constant" instruction (e.g., {@code
> 60: * aload_0}).
Suggestion:
* iconst_0}).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Wed, 15 Feb 2023 14:12:21 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java >> line 63: >> >>> 61: * aload_0}). >>> 62: */ >>> 63: sealed interface IntrinsicConstantInstruction extends >>> ConstantInstruction >> >> I'm not super sure of the fine-grained distinction here. The constant pool >> variant is interesting (as you can ask for the associated constant entry) - >> but the distinction between intrinsics vs. argument seems rather weak. > > They significantly differ in instruction formats and instruction format > distinction is critical for some use cases. I think this distinction is appropriate for the level of modeling. CodeBuilder::constantInstruction(ConstantDesc value) is very useful in selecting the most appropriate specific constant instruction. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/ExceptionCatch.java
line 77:
> 75: static ExceptionCatch of(Label handler, Label tryStart, Label tryEnd,
> 76: Optional catchTypeEntry) {
> 77: return new AbstractPseudoInstruction.ExceptionCatchImpl(handler,
> tryStart, tryEnd, catchTypeEntry);
Suggestion:
return new AbstractPseudoInstruction.ExceptionCatchImpl(handler,
tryStart, tryEnd, catchTypeEntry.orElse(null));
Then you only need one constructor for `ExceptionCatchImpl` and you don't need
to disambiguate a call using a cast for a final argument that is `null` .
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/NewMultiArrayInstruction.java
line 60:
> 58: static NewMultiArrayInstruction of(ClassEntry arrayTypeEntry,
> 59:int dimensions) {
> 60: return new
> AbstractInstruction.UnboundNewMultidimensionalArrayInstruction(arrayTypeEntry,
> dimensions);
Should we validate that the dimensionality of `arrayType` is greater than or
equal to `dimensions`?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/LoadInstruction.java
line 66:
> 64: * @param slot the local varaible slot to load from
> 65: */
> 66: static LoadInstruction of(Opcode op, int slot) {
Should you validate that `slot` is compatible with `op`? (same for the
StoreInstruction?)
src/java.base/share/classes/jdk/internal/classfile/instruction/NewObjectInstruction.java
line 38:
> 36: * of a {@link CodeModel}.
> 37: */
> 38: public sealed interface NewObjectInstruction extends Instruction
Should we add a helper method on `CodeBuilder` that does the new + dup + invoke
special dance?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
line 156:
> 154: @Override
> 155: public String toString() {
> 156: return String.format("Store[OP=%s, slot=%d]", this.opcode(),
> slot());
A suggestion. I believe the `toString` output for bound and unbound
instructions should be identical and it can composed solely from methods on the
public instruction interface. There are some differences e.g. `Increment` and
`Inc` for the unbound and bound increment instruction respectively.
If those assumptions are correct i recommend placing a static `toString` method
on all unbound instructions that also have bound instructions, accepting the
public instruction interface as an argument. Then the overridden
`Object::toString` method defers to those. Thereby there is no duplication.
(Alas we cannot override `toString` on the instruction interface itself).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 93:
> 91: * directly; they are passed to handlers by methods such as {@link
> 92: * MethodBuilder#withCode(Consumer)} or to code transforms. The elements
> of a
> 93: * code can be specified abstractly (by passing a {@link CodeElement} to
> {@link
Suggestion:
* code can be specified abstractly, by passing a {@link CodeElement} to {@link
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 164:
> 162: * generate the code.
> 163: * @return this builder
> 164: */
Suggestion:
/**
* Apply a transform to the code built by a handler, directing results to
this builder.
*
* @param transform the transform to apply to the code built by the handler
* @param handler the handler that receives a {@linkplain CodeBuilder} to
* build the code.
* @return this builder
*/
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 165:
> 163: * @return this builder
> 164: */
> 165: default CodeBuilder transforming(CodeTransform transform,
> Consumer handler) {
The functionality of this method, `transforming`, and
`ClassfileBuilder::transform`, are in effect equivalent in their transforming:
adding the results of transformed code to the builder. They differ in the
source of code elements.
The latter's behaviour can be implemented using the former, with a consumer
that passes all elements of a code model to the builder e.g. `builder ->
model.forEach(builder::with)`.
The difference in naming initially confused me. To me this suggests the method
names should be the same? (perhaps with the transformer being consistently the
last argument?).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/CodeTransform.java line 91:
> 89: @Override
> 90: default ResolvedTransform resolve(CodeBuilder builder) {
> 91: return new TransformImpl.CodeTransformImpl(e -> accept(builder,
> e),
Make `CodeTransformImpl` generic in the class file element, rename to say
`ResolvedTransformImpl` and reuse for the other `XxxTransform`?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/impl/TransformImpl.java line
63:
> 61: private static final Runnable NOTHING = () -> { };
> 62:
> 63: interface FakeClassTransform extends ClassTransform {
Rename to `UnresolvedXxxTransform`? I think that is a better name, since it
could appear in stack traces.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractDirectBuilder.java
line 34:
> 32: * AbstractDirectBuilder
> 33: */
> 34: public class AbstractDirectBuilder {
Type variable `B` is unused.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/impl/NonterminalCodeBuilder.java
line 46:
> 44: this.terminal = switch (parent) {
> 45: case ChainedCodeBuilder cb -> cb.terminal;
> 46: case BlockCodeBuilderImpl cb -> cb.terminal;
Suggestion:
case NonterminalCodeBuilder cb -> cb.terminal;
?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/TypeKind.java line 45:
> 43: DoubleType("double", "D", 7),
> 44: /** a reference type */
> 45: ReferenceType("reference type", "A", -1),
Suggestion:
ReferenceType("reference type", "L", -1),
?
Otherwise `TypeKind.fromDescriptor(TypeKind.ReferenceType.descriptor())` will
fail.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 15:50:33 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/instruction/NewMultiArrayInstruction.java
>> line 60:
>>
>>> 58: static NewMultiArrayInstruction of(ClassEntry arrayTypeEntry,
>>> 59:int dimensions) {
>>> 60: return new
>>> AbstractInstruction.UnboundNewMultidimensionalArrayInstruction(arrayTypeEntry,
>>> dimensions);
>>
>> Should we validate that the dimensionality of `arrayType` is greater than or
>> equal to `dimensions`?
>
> Architectural decision is to do not provide much of validation in favour of
> performance, however it might be re-visited in cases like this. Please raise
> the validation topic at classfile-api-dev at openjdk.org, thanks.
Ok.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
line 683:
> 681: public UnboundInstruction(Opcode op, int size) {
> 682: super(op, size);
> 683: }
Unused?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/AnnotationReader.java
line 99:
> 97: }
> 98:
> 99: public static List>
> readParameterAnnotations(ClassReader classReader, int p, boolean isVisible) {
Parameter `isVisible` is unused, but method is called with true/false values.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
line 73:
> 71: if (a.attributeMapper() == am)
> 72: iterator.remove();
> 73: }
Suggestion:
attributes.removeIf(a -> a.attributeMappter() == am);
But presumably use an inner class instead. I can understand because of that if
you want to keep the existing code instead.
src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
line 76:
> 74: }
> 75:
> 76: List> attributes() {
Unused
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/BootstrapMethodEntryImpl.java
line 83:
> 81: }
> 82: else
> 83: return false;
Suggestion:
return obj instanceof BootstrapMethodEntry e
&& e.bootstrapMethod().equals(handle)
&& e.arguments().equals(arguments);
? I am uncertain about the use of reference equality, since it indicates an
instance is only equal to itself?
src/java.base/share/classes/jdk/internal/classfile/impl/BootstrapMethodEntryImpl.java
line 91:
> 89: for (LoadableConstantEntry a : arguments) {
> 90: hash = 31 * hash + a.hashCode();
> 91: }
Suggestion:
hash = 31 * hash + arguments.hashCode();
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with three additional > commits since the last revision: > > - fixed AccessFlags javadoc > - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform > - removed obsolete generic parameter from AbstractDirectBuilder src/java.base/share/classes/jdk/internal/classfile/impl/BufferedMethodBuilder.java line 54: > 52: private final List elements = new ArrayList<>(); > 53: private final SplitConstantPool constantPool; > 54: private final ClassEntry thisClass; Unused. Can be removed as can the associated constructor parameter. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with three additional > commits since the last revision: > > - fixed AccessFlags javadoc > - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform > - removed obsolete generic parameter from AbstractDirectBuilder src/java.base/share/classes/jdk/internal/classfile/impl/ClassPrinterImpl.java line 570: > 568: fieldToTree(f, verbosity > 569: .with(new ListNodeImpl(BLOCK, "methods", > clm.methods().stream().map(mm -> > 570: (Node)methodToTree(mm, verbosity; Suggestion: methodToTree(mm, verbosity; (Note as i go through the code i am looking at the IDEs report of problems and making note as comments. Likely be more productive for you to do that directly.) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/CodeImpl.java line 52:
> 50: static final Instruction[] SINGLETON_INSTRUCTIONS = new
> Instruction[256];
> 51:
> 52: static {
Can we loop through all `Opcode` values filter for `sizeIfFixed == 1` and
switch on the kind? If so that would avoid the need for explicit lists and
simplify the code.
src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line 176:
> 174: }
> 175:
> 176: public static long nextPowerOfTwo( long x ) {
If you like you can change the implementation to be:
x = -1 >>> Long.numberOfLeadingZeros(x - 1);
return x + 1;
See `ConcurrentHashMap`.
src/java.base/share/classes/jdk/internal/classfile/impl/Util.java line 84:
> 82: }
> 83: }
> 84: }
Suggestion:
loop: for (int i = 1; i < type.length(); ++i) {
switch (type.charAt(i)) {
case '[':
bs.set(i);
while (type.charAt(++i) == '[')
;
if (type.charAt(i) == 'L') {
while (type.charAt(++i) != ';')
;
}
break;
case ')':
break loop;
default:
bs.set(i);
if (type.charAt(i) == 'L') {
while (type.charAt(++i) != ';')
;
}
}
}
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/Util.java line 161:
> 159: var desc = cd.descriptorString();
> 160: return switch (desc.charAt(0)) {
> 161: case '[' -> desc;
Is this correct? Arrays don't have an internal name.
-
PR: https://git.openjdk.org/jdk/pull/10982
