Re: RFR: 8307466: java.time.Instant calculation bug in until and between methods [v2]
> The implementation of java.time.Instant.until(I2, ChronoUnit) in some cases > did not correctly borrow or carry from the nanos to the seconds when > computing using ChronoUnit.MILLIS or ChronoUnit.MICROS. > The errant computation was introduced by > [JDK-8273369](https://bugs.openjdk.org/browse/JDK-8273369). Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Slight perf improvement using int instead of long for local nanosDiff - Changes: - all: https://git.openjdk.org/jdk/pull/13846/files - new: https://git.openjdk.org/jdk/pull/13846/files/8deb8df7..46d4918a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13846=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13846=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13846.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13846/head:pull/13846 PR: https://git.openjdk.org/jdk/pull/13846
Re: RFR: 8307575: Migrate the serialization constructor accessors to Method Handles [v2]
> Apparently method handle linking doesn't impose extra checks on constructor > invocation, so the special logic for the serialization constructor to call > superclass constructor in MagicAccessorImpl can be removed altogether with > old core reflection implementation. > > Serialization and sun.reflect.ReflectionFactory tests pass. May be worth to > think about the long-term treatment of > ReflectionFactory.newConstructorForSerialization, as creating partial object > is inherently unsafe, and behavior of > `newConstructorForSerialization(ArrayList.class, > String.class.getDeclaredConstructor(String.class))` etc. (which is accepted > for now) may have unpredictable side effects. > > #1830 has a similar patch; this one doesn't touch proxies and updates to the > new post-JEP 416 reflection implementation. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Remove invalid assertion (via sun.reflect.ReflectionFactory) - Changes: - all: https://git.openjdk.org/jdk/pull/13853/files - new: https://git.openjdk.org/jdk/pull/13853/files/64a8f518..3a0393a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13853=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13853=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13853.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13853/head:pull/13853 PR: https://git.openjdk.org/jdk/pull/13853
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
> As John Rose has pointed out in this issue, the current j.l.r.Proxy based > implementation of MethodHandleProxies.asInterface has a few issues: > 1. Exposes too much information via Proxy supertype (and WrapperInstance > interface) > 2. Does not allow future expansion to support SAM[^1] abstract classes > 3. Slow (in fact, very slow) > > This patch addresses all 3 problems: > 1. It updates the WrapperInstance methods to take an `Empty` to avoid method > clashes > 2. This patch obtains already generated classes from a ClassValue by the > requested interface type; the ClassValue can later be updated to compute > implementation generation for abstract classes as well. > 3. This patch's faster than old implementation in general. > > > Benchmark Mode Cnt > Score Error Units > MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 > 1.483 ±0.025 ns/op > MethodHandleProxiesAsIFInstance.baselineComputeavgt 15 > 0.264 ±0.006 ns/op > MethodHandleProxiesAsIFInstance.testCall avgt 15 > 1.773 ±0.040 ns/op > MethodHandleProxiesAsIFInstance.testCreate avgt 15 >16.754 ±0.411 ns/op > MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >19.609 ±1.598 ns/op > MethodHandleProxiesAsIFInstanceCall.callDoable avgt 15 > 0.424 ±0.024 ns/op > MethodHandleProxiesAsIFInstanceCall.callHandle avgt 15 > 1.936 ±0.008 ns/op > MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 15 > 1.766 ±0.014 ns/op > MethodHandleProxiesAsIFInstanceCall.callLambda avgt 15 > 0.414 ±0.005 ns/op > MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 15 > 0.271 ±0.006 ns/op > MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 15 > 0.263 ±0.004 ns/op > MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 15 > 0.266 ±0.005 ns/op > MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 15 > 0.262 ±0.003 ns/op > MethodHandleProxiesAsIFInstanceCall.direct avgt 15 > 0.264 ±0.005 ns/op > MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 15 >18.000 ±0.181 ns/op > MethodHandleProxiesAsIFInstanceCreate.createCallLambda avgt 15 > 17624.673 ± 2404.853 ns/op > MethodHandleProxiesAsIFInstanceCreate.createInterfaceInstance avgt 15 >17.554 ±0.748 ns/op > MethodHandleProxiesAsIFInstanceCreate.createLambda avgt 15 > 16860.341 ± 1270.982 ns/op > MethodHandleProxiesSuppl.testInstanceTargetavgt 15 > 0.405 ±0.006 ns/op > MethodHandleProxiesSuppl.testInstanceType avgt 15 > 0.343 ±0.005 ns/op > MethodHandleProxiesSuppl.testIsWrapperInstance avgt 15 > 0.375 ±0.021 ns/op > > > Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's > no longer applicable. > > [^1]: single abstract method Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Remove assertion, no longer true with teleport definition in MHP - Changes: - all: https://git.openjdk.org/jdk/pull/13197/files - new: https://git.openjdk.org/jdk/pull/13197/files/836d9d0b..bd21e68e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13197=15 - incr: https://webrevs.openjdk.org/?repo=jdk=13197=14-15 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13197.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13197/head:pull/13197 PR: https://git.openjdk.org/jdk/pull/13197
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v15]
On Sun, 7 May 2023 04:26:34 GMT, Chen Liang wrote: >> As John Rose has pointed out in this issue, the current j.l.r.Proxy based >> implementation of MethodHandleProxies.asInterface has a few issues: >> 1. Exposes too much information via Proxy supertype (and WrapperInstance >> interface) >> 2. Does not allow future expansion to support SAM[^1] abstract classes >> 3. Slow (in fact, very slow) >> >> This patch addresses all 3 problems: >> 1. It updates the WrapperInstance methods to take an `Empty` to avoid method >> clashes >> 2. This patch obtains already generated classes from a ClassValue by the >> requested interface type; the ClassValue can later be updated to compute >> implementation generation for abstract classes as well. >> 3. This patch's faster than old implementation in general. >> >> >> Benchmark Mode Cnt >> Score Error Units >> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 >> 1.483 ±0.025 ns/op >> MethodHandleProxiesAsIFInstance.baselineComputeavgt 15 >> 0.264 ±0.006 ns/op >> MethodHandleProxiesAsIFInstance.testCall avgt 15 >> 1.773 ±0.040 ns/op >> MethodHandleProxiesAsIFInstance.testCreate avgt 15 >> 16.754 ±0.411 ns/op >> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >> 19.609 ±1.598 ns/op >> MethodHandleProxiesAsIFInstanceCall.callDoable avgt 15 >> 0.424 ±0.024 ns/op >> MethodHandleProxiesAsIFInstanceCall.callHandle avgt 15 >> 1.936 ±0.008 ns/op >> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 15 >> 1.766 ±0.014 ns/op >> MethodHandleProxiesAsIFInstanceCall.callLambda avgt 15 >> 0.414 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 15 >> 0.271 ±0.006 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 15 >> 0.263 ±0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 15 >> 0.266 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 15 >> 0.262 ±0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.direct avgt 15 >> 0.264 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 15 >> 18.000 ±0.181 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallLambda avgt 15 >> 17624.673 ± 2404.853 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createInterfaceInstance avgt 15 >> 17.554 ±0.748 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createLambda avgt 15 >> 16860.341 ± 1270.982 ns/op >> MethodHandleProxiesSuppl.testInstanceTargetavgt 15 >> 0.405 ±0.006 ns/op >> MethodHandleProxiesSuppl.testInstanceType avgt 15 >> 0.343 ±0.005 ns/op >> MethodHandleProxiesSuppl.testIsWrapperInstance avgt 15 >> 0.375 ±0.021 ns/op >> >> >> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's >> no longer applicable. >> >> [^1]: single abstract method > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Fix tabs, and tests about modules and constructor access Moved the implementation back to shared-class. The dedicated-class implementation will probably be offered from another API instead. The test failure is caused by lookup class definer defining classes in a different module, which breaks the `assert(Reflection::is_same_class_package(lookup_k, ik))` assertion. Since I know little to none C++, might need someone's help to further tweak that part. - PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1537436920
Re: RFR: 8304913: Use OperatingSystem, Architecture, and Version in jlink [v3]
On Fri, 5 May 2023 13:59:37 GMT, Roger Riggs wrote:
>> Refactor the Platform class of jlink to use jdk.internal.util
>> OperatingSystem and Architecture instead of os.name and os.arch.
>> They are direct replacements for the Platform enums except for UNKNOWN; its
>> use is refactored to report errors via exceptions.
>>
>> Neither os.name nor os.arch should be assumed to be changeable;
>> one test case is removed because it assumes os.name can be changed on the
>> command line.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Source code cleanup suggested by reviewers
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Platform.java line 64:
> 62: */
> 63: public boolean is64Bit() {
> 64: return Architecture.is64bit();
`Platform::is64Bit` tests if the target architecture is 64-bit but
`Architecture.is64bit()` tests the runtime architecture.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13585#discussion_r1186816949
Re: RFR: 8304913: Use OperatingSystem, Architecture, and Version in jlink [v3]
On Fri, 5 May 2023 13:59:37 GMT, Roger Riggs wrote: >> Refactor the Platform class of jlink to use jdk.internal.util >> OperatingSystem and Architecture instead of os.name and os.arch. >> They are direct replacements for the Platform enums except for UNKNOWN; its >> use is refactored to report errors via exceptions. >> >> Neither os.name nor os.arch should be assumed to be changeable; >> one test case is removed because it assumes os.name can be changed on the >> command line. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Source code cleanup suggested by reviewers I suggest to remove `Platform::is64Bit` and simply call `Architecture::is64bit`. Otherwise, looks fine. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13585#pullrequestreview-1415865569
