Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Dean Long]

On Fri, 9 Apr 2021 16:54:51 GMT, Ioi Lam  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> LGTM. Just a small nit.

@iklam
I thought the fingerprint code was also used by CDS.

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: jpackage ea-17 --mac-entitlements

[Michael Hall]

> 
> OK, probable user error. I eliminated my entitlement changes and it worked.
> 
Related to the same functionality that I am trying to add, I should, or have 
to, make changes to my Info.plist
This being the addition of NSAppleEventsUsageDescription. 
If I remember correctly in prior discussion concerning application specific 
environment variables provision was made for the user to include their own 
custom Info.plist in the Resources directory? Is that correct? Does the file 
name stay the same?
 
If of interest what I am trying to do is pretty much as described here…
Authorization by AEDeterminePermissionToAutomateTarget waits infinit time
https://developer.apple.com/forums/thread/666528 

Except that hangs intermittently and rarely while mine always hangs.

I want to authorize automation AppleEvents for my own application. This 
consistently hangs on the above call (AEDeterminePermissionToAutomateTarget) 
done in JNI.

2442 AEDeterminePermissionToAutomateTarget  (in AE) + 1313  [0x7fff3a2d8f2c]
+   2442 
_dispatch_semaphore_wait_slow  (in libdispatch.dylib) + 98  [0x7fff72fb4fbf]
+ 2442 
_dispatch_sema4_wait  (in libdispatch.dylib) + 16  [0x7fff72fb4aed]
+   2442 
semaphore_wait_trap  (in libsystem_kernel.dylib) + 10  [0x7fff7314ee36]

I have succeeded in getting the same native code to run command line from 
Terminal. I got it to run once from my application when the application was 
launched command line, it showed a dialog, and granted permission. However, the 
dialog application showed as Terminal and not mine.. So now the functionality 
actually works if command line launched but doesn’t if double click launched. 
With the problem that it hangs if I try the permit double click launched.

So it seems Terminal must be doing something different or have different 
authorization than I do. One difference was the entitlement that I originally 
was crashing with on this list thread.

Also if of interest I determined that difference by using the Taccy 
application. 
https://eclecticlight.co/tag/taccy/ 
For Terminal this shows 

App signature check:
/System/Applications/Utilities/Terminal.app: accepted
source=Apple System
origin=Software Signing

For my application…

App signature check:
⛔️ spctl error 1
/Users/mjh/HalfPipe/HalfPipe_jpkg/outputdir/HalfPipe.app: a sealed resource is 
missing or invalid

I believe this relates to…
JDK-8263156 : [macos]: OS X application signing concerns - a sealed resource is 
missing or invalid
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8263156 


Although I believe 
codesign -v --verbose=4 outputdir/HalfPipe.app 
This verify would be sufficient to reproduce that error for any(?) jpackage 
Developer signed application.

I’m not sure what your policies are for using 3rd party tools but Taccy seems 
to give you a nice graphical view of application signing/entitlement related.

Sorry for the length,
Thanks

RFR: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"

[Alex Menkov]

The test actually failed starting from jdk13, but the error is masked by 
JDK-8264667 (and shell part of the test didn't verify result of the java 
execution)
The fix:
- updates JvmtiClassFileReconstituter to add attributes in the same order as 
javac does
- makes the test java instead of shell
- added workaround for JDK-8264667
- test code is ready to ignore order of attributes

-

Commit messages:
 - fixed spaces
 - JDK-8262002

Changes: https://git.openjdk.java.net/jdk/pull/3426/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3426=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262002
  Stats: 220 lines in 4 files changed: 113 ins; 96 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3426.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3426/head:pull/3426

PR: https://git.openjdk.java.net/jdk/pull/3426

Re: RFR: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"

[Alex Menkov]

On Sat, 10 Apr 2021 01:10:37 GMT, Alex Menkov  wrote:

> The test actually failed starting from jdk13, but the error is masked by 
> JDK-8264667 (and shell part of the test didn't verify result of the java 
> execution)
> The fix:
> - updates JvmtiClassFileReconstituter to add attributes in the same order as 
> javac does
> - makes the test java instead of shell
> - added workaround for JDK-8264667
> - test code is ready to ignore order of attributes

label remove core-libs

-

PR: https://git.openjdk.java.net/jdk/pull/3426

Re: RFR: 8264208: Console charset API [v2]

[Bernd Eckenfels]

Hello,

I like the API, it is useful, however not enough to replace the defaultCharset 
once the Change to UTF8 is done. You still need a way to query the platforms 
file encoding (especially on Windows).

Also I wonder if the Javadoc needs to discuss platform aspects of console, 
especially System.out and LANG on unix vs. windows.

Gruss
Bernd
--
http://bernd.eckenfels.net

Von: security-dev  im Auftrag von Naoto 
Sato 
Gesendet: Friday, April 9, 2021 11:06:00 PM
An: core-libs-dev@openjdk.java.net ; 
security-...@openjdk.java.net 
Betreff: Re: RFR: 8264208: Console charset API [v2]

> Please review the changes for the subject issue.  This has been suggested in 
> a recent discussion thread for the JEP 400 
> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>  A CSR has also been drafted, and comments are welcome 
> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Reflected the review comments.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3419/files
  - new: https://git.openjdk.java.net/jdk/pull/3419/files/d6db04bb..8fd8f6e6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=00-01

  Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3419.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: jpackage ea-17 --mac-entitlements

[Michael Hall]

> On Apr 9, 2021, at 6:59 PM, Michael Hall  wrote:
> 
> Should bug reports be written against this yet?
> 
> I codesign generated a entitlements file and then tried adding…
> 
>com.apple.private.tcc.allow-prompting
>
> 
> I added this to my invocation…
> 
> --mac-entitlements “entitlements.xml"
> 
> And the app crashed…
> 
> System Integrity Protection: enabled
> 
> Crashed Thread:0
> 
> Exception Type:EXC_CRASH (Code Signature Invalid)
> Exception Codes:   0x, 0x
> Exception Note:EXC_CORPSE_NOTIFY
> 
> Termination Reason:Namespace CODESIGNING, Code 0x1
> 
> This is new to me and user error is very possible. It does seem to possibly 
> be necessary for something I am trying to do.  If this is still new work in 
> progress and you would prefer bug reports to be deferred until later that’s 
> fine. If you would like the bug reports written now I can do that.

OK, probable user error. I eliminated my entitlement changes and it worked.

jpackage ea-17 --mac-entitlements

[Michael Hall]

Should bug reports be written against this yet?

I codesign generated a entitlements file and then tried adding…

com.apple.private.tcc.allow-prompting


I added this to my invocation…

--mac-entitlements “entitlements.xml"

And the app crashed…

System Integrity Protection: enabled

Crashed Thread:0

Exception Type:EXC_CRASH (Code Signature Invalid)
Exception Codes:   0x, 0x
Exception Note:EXC_CORPSE_NOTIFY

Termination Reason:Namespace CODESIGNING, Code 0x1

This is new to me and user error is very possible. It does seem to possibly be 
necessary for something I am trying to do.  If this is still new work in 
progress and you would prefer bug reports to be deferred until later that’s 
fine. If you would like the bug reports written now I can do that.

Re: [External] : Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[John Rose]

On Apr 9, 2021, at 4:00 PM, John Rose 
mailto:john.r.r...@oracle.com>> wrote:

The MH combinator for lookupswitch can use a data-driven
reverse lookup in a (frozen/stable) int[] array, using binary
search.  The bytecode emitter can render such a thing as
an internal lookupswitch, if that seems desirable.  But
the stable array with binary search scales to other types
besides int, so it’s the right primitive.

This may be confusing on a couple of points.
First, the mapping function I’m talking about is not
a MH combinator, but rather a MH factory, which takes
a non-MH argument, probably an unsorted array or List
of items of any type.  It then DTRT (internal hash map
or tree map or flat binary search or flat table lookup or
lookupswitch or any combination thereof) to get
an algorithm to classify the array or List elements
into a compact enumeration [0,N).

Second, when the input array or List is of int (or
Integer) then it *might* be a lookupswitch internally,
and I’m abusing the terminology to call this particular
case a lookupswitch.  But it’s really just a classifier
factory, whose output is a MH of type K → [0,N) for
some K.  The output might also be ToIntFunction
for all I care; that can be inter-converted with a MH.

Re: [External] : Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[John Rose]

On Apr 9, 2021, at 11:15 AM, fo...@univ-mlv.fr wrote:
> 
> - Mail original -
>> De: "John Rose" 
>> À: "Remi Forax" 
>> Cc: "Jorn Vernee" , "core-libs-dev" 
>> 
>> Envoyé: Vendredi 9 Avril 2021 20:01:18
>> Objet: Re: RFR: 8263087: Add a MethodHandle combinator that switches over a 
>> set of MethodHandles
> 
> Hi John,
> 
>> On Apr 9, 2021, at 9:55 AM, Remi Forax  wrote:
>>> 
>>> I think the combinator should be lookupswitch which is more general than
>>> tableswitch with a special case when generating the bytecode to generate a
>>> tableswitch instead of a lookupswitch if the indexes are subsequent.
>> 
>> We can get there in the simpler steps Jorn has outlined.
> 
> I fail to see how it can work.

If you have a fixed set of N cases it is always valid
to number them compactly in [0,N).  If there is
another association of keys in some set K to cases,
then you simply build a mapping K → [0,N).  That
works for enums, lookupswitch, strings, and patterns.
The mapping functions would be:
  - Enum::ordinal
  - a lookupswitch of cases `case i: return n`, n in [0,N]
  - some perfect hash, composed with lookupswitch
  - some decision tree that outputs compact case numbers

In the second case, C2 will inline the lookupswitch and
tableswitch together and do branch-to-branch tensioning.
The result will be the same as if the intermediate tableswitch
had not been used.

The MH combinator for lookupswitch can use a data-driven
reverse lookup in a (frozen/stable) int[] array, using binary
search.  The bytecode emitter can render such a thing as
an internal lookupswitch, if that seems desirable.  But
the stable array with binary search scales to other types
besides int, so it’s the right primitive.

The SwitchBootstraps class is the place to match a
static decision tree or decision chain (of N cases) of
an arbitrary shape to compact case labels in [0,N).
Then they all feed to Jorn’s primitive.

> 
>> 
>> The combinator is much simpler if the case numbers are implicit in [0,N). 
>> Then
>> it’s natural to filter on the [0,N) input as a separately factored choice.
> 
> An array of MethodHandles + a default method handle is simpler than an array 
> of sorted ints + an array of MethodHandles + a default method, but not much 
> simpler.

Simpler by the complexity of the sorting, which is a sharp edge.
The type “sorted int array without duplicates and unaliased
or frozen” is pretty involved.  Easy to make mistakes with it.

> 
>> That also scales to pattern-switch.
> 
> yes, for all the switches, pattern-switch, enum-switch but not for the string 
> switch which requires a lookup switch.

Nope; see above.

> Can you outline how to use the tableswitch combinator in the case of a switch 
> on strings ?

Above; reduce to perfect hash plus lookupswitch
producing compact int values to feed to a tableswitch.

Summary:  Switches only need one case label domain:  [0,N).
Everything else is case label mappings.

— John

Re: RFR: 8264806: Remove the experimental JIT compiler

[Vladimir Kozlov]

On Fri, 9 Apr 2021 17:35:11 GMT, Vladimir Kozlov  wrote:

> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to 
> Java-based JIT compiler (Graal) from JDK:
> 
> - `jdk.internal.vm.compiler` — the Graal compiler 
> - `jdk.internal.vm.compiler.management` — Graal's `MBean`
> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests
> 
> Remove Graal related code in makefiles.
> 
> Note, next two `module-info.java` files are preserved so that the JVMCI 
> module `jdk.internal.vm.ci` continues to build:
> src/jdk.internal.vm.compiler/share/classes/module-info.java
> src/jdk.internal.vm.compiler.management/share/classes/module-info.java
> 
> @AlanBateman suggested that we can avoid it by using Module API to export 
> packages at runtime . It requires changes in GraalVM's JVMCI too so I will 
> file followup RFE to implement it.
> 
> Tested hs-tier1-4

Thankyou, @erikj79, for review. I restored code as you asked.
For some reasons incremental webrev shows update only in Cdiffs.

-

PR: https://git.openjdk.java.net/jdk/pull/3421

Re: RFR: 8264806: Remove the experimental JIT compiler [v2]

[Vladimir Kozlov]

> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to 
> Java-based JIT compiler (Graal) from JDK:
> 
> - `jdk.internal.vm.compiler` — the Graal compiler 
> - `jdk.internal.vm.compiler.management` — Graal's `MBean`
> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests
> 
> Remove Graal related code in makefiles.
> 
> Note, next two `module-info.java` files are preserved so that the JVMCI 
> module `jdk.internal.vm.ci` continues to build:
> src/jdk.internal.vm.compiler/share/classes/module-info.java
> src/jdk.internal.vm.compiler.management/share/classes/module-info.java
> 
> @AlanBateman suggested that we can avoid it by using Module API to export 
> packages at runtime . It requires changes in GraalVM's JVMCI too so I will 
> file followup RFE to implement it.
> 
> Tested hs-tier1-4

Vladimir Kozlov 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:

 - Restore Graal Builder image makefile
 - Merge latest changes from branch 'JDK-8264805' into JDK-8264806
 - 8264806: Remove the experimental JIT compiler

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3421/files
  - new: https://git.openjdk.java.net/jdk/pull/3421/files/8ff9b599..a246aaa6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3421=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3421=00-01

  Stats: 102 lines in 12 files changed: 66 ins; 27 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3421.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3421/head:pull/3421

PR: https://git.openjdk.java.net/jdk/pull/3421

Re: RFR: 8264208: Console charset API [v2]

[Joe Wang]

On Fri, 9 Apr 2021 21:02:26 GMT, Naoto Sato  wrote:

>> src/java.base/share/classes/java/io/Console.java line 404:
>> 
>>> 402:  *
>>> 403:  * @return A {@code Charset} object used in this {@code Console}.
>>> 404:  * @since 17
>> 
>> A couple of minor comments: 
>> May replace {@code Charset} with @link;
>> Add "the" to the sentence: The returned charset corresponds to "the" input...
>> @return: javadoc guide suggests "do not capitalize and do not end with a 
>> period" when writing a phrase. But I guess for consistency in this class, 
>> it's okay to capitalize.
>
> Thanks, Joe. Modified as suggested.

Thanks. Looks good to me.

-

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8264208: Console charset API [v2]

[Joe Wang]

On Fri, 9 Apr 2021 21:06:00 GMT, Naoto Sato  wrote:

>> Please review the changes for the subject issue.  This has been suggested in 
>> a recent discussion thread for the JEP 400 
>> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>>  A CSR has also been drafted, and comments are welcome 
>> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reflected the review comments.

Marked as reviewed by joehw (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v5]

[Igor Veresov]

On Fri, 9 Apr 2021 21:59:04 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comments

Marked as reviewed by iveresov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v5]

[Vladimir Kozlov]

> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to 
> Ahead-of-Time Compiler from JDK: 
> 
> - `jdk.aot` module — the `jaotc` tool 
> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
> - related HotSpot code guarded by `#if INCLUDE_AOT` 
> 
> Additionally, remove tests as well as code in makefiles related to AoT 
> compilation.
> 
> Tested hs-tier1-4

Vladimir Kozlov has updated the pull request incrementally with one additional 
commit since the last revision:

  Address review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3398/files
  - new: https://git.openjdk.java.net/jdk/pull/3398/files/6cce0f6c..71a166c1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3398=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3398=03-04

  Stats: 36 lines in 9 files changed: 0 ins; 27 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3398.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3398/head:pull/3398

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8263763: Synthetic constructor parameters of enum are not considered for annotation indices [v2]

[Joe Darcy]

On Fri, 19 Mar 2021 15:20:00 GMT, Rafael Winterhalter 
 wrote:

>> 8263763: The constructor of an enumeration prefixes with two synthetic 
>> arguments for constant name and ordinal index. For this reason, the 
>> constructor annotations need to be shifted two indices to the right, such 
>> that the annotation indices match with the parameter indices.
>
> Rafael Winterhalter 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.

Marked as reviewed by darcy (Reviewer).

src/java.base/share/classes/java/lang/reflect/Constructor.java line 612:

> 610: Class declaringClass = getDeclaringClass();
> 611: if (
> 612: declaringClass.isEnum()

Please format as
if (declaringClass.isEnum()) {
...

test/jdk/java/lang/annotation/EnumConstructorAnnotation.java line 39:

> 37: public static void main(String[] args) {
> 38: Constructor c = SampleEnum.class.getDeclaredConstructors()[0];
> 39: Annotation[] a = c.getParameters()[2].getAnnotations();

The value of c.getParameters().getAnnotations() can be checked for consistency 
against c.getParameterAnnotations(). (The type java.lang.reflect.Parameter was 
added several releases after annotations were added.)

-

PR: https://git.openjdk.java.net/jdk/pull/3082

Re: RFR: 8264208: Console charset API [v2]

[Naoto Sato]

> Please review the changes for the subject issue.  This has been suggested in 
> a recent discussion thread for the JEP 400 
> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>  A CSR has also been drafted, and comments are welcome 
> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Reflected the review comments.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3419/files
  - new: https://git.openjdk.java.net/jdk/pull/3419/files/d6db04bb..8fd8f6e6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=00-01

  Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3419.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8264208: Console charset API [v2]

[Naoto Sato]

On Fri, 9 Apr 2021 19:25:02 GMT, Joe Wang  wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Reflected the review comments.
>
> src/java.base/share/classes/java/io/Console.java line 404:
> 
>> 402:  *
>> 403:  * @return A {@code Charset} object used in this {@code Console}.
>> 404:  * @since 17
> 
> A couple of minor comments: 
> May replace {@code Charset} with @link;
> Add "the" to the sentence: The returned charset corresponds to "the" input...
> @return: javadoc guide suggests "do not capitalize and do not end with a 
> period" when writing a phrase. But I guess for consistency in this class, 
> it's okay to capitalize.

Thanks, Joe. Modified as suggested.

-

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Mandy Chung]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

I reviewed the module-loader-map and non-hotspot non-AOT tests.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[Jorn Vernee]

On Fri, 9 Apr 2021 18:27:07 GMT, Jorn Vernee  wrote:

>> This patch adds a `tableSwitch` combinator that can be used to switch over a 
>> set of method handles given an index, with a fallback in case the index is 
>> out of bounds, much like the `tableswitch` bytecode. Here is a description 
>> of how it works (copied from the javadoc):
>> 
>>  Creates a table switch method handle, which can be used to switch over 
>> a set of target
>>  method handles, based on a given target index, called selector.
>> 
>>  For a selector value of {@code n}, where {@code n} falls in the range 
>> {@code [0, N)},
>>  and where {@code N} is the number of target method handles, the table 
>> switch method
>>  handle will invoke the n-th target method handle from the list of 
>> target method handles.
>> 
>>  For a selector value that does not fall in the range {@code [0, N)}, 
>> the table switch
>>  method handle will invoke the given fallback method handle.
>> 
>>  All method handles passed to this method must have the same type, with 
>> the additional
>>  requirement that the leading parameter be of type {@code int}. The 
>> leading parameter
>>  represents the selector.
>> 
>>  Any trailing parameters present in the type will appear on the returned 
>> table switch
>>  method handle as well. Any arguments assigned to these parameters will 
>> be forwarded,
>>  together with the selector value, to the selected method handle when 
>> invoking it.
>> 
>> The combinator does not support specifying the starting index, so the switch 
>> cases always run from 0 to however many target handles are specified. A 
>> starting index can be added manually with another combination step that 
>> filters the input index by adding or subtracting a constant from it, which 
>> does not affect performance. One of the reasons for not supporting a 
>> starting index is that it allows for more lambda form sharing, but also 
>> simplifies the implementation somewhat. I guess an open question is if a 
>> convenience overload should be added for that case?
>> 
>> Lookup switch can also be simulated by filtering the input through an 
>> injection function that translates it into a case index, which has also 
>> proven to have the ability to have comparable performance to, or even better 
>> performance than, a bytecode-native `lookupswitch` instruction. I plan to 
>> add such an injection function to the runtime libraries in the future as 
>> well. Maybe at that point it could be evaluated if it's worth it to add a 
>> lookup switch combinator as well, but I don't see an immediate need to 
>> include it in this patch.
>> 
>> The current bytecode intrinsification generates a call for each switch case, 
>> which guarantees full inlining of the target method handles. Alternatively 
>> we could only have 1 callsite at the end of the switch, where each case just 
>> loads the target method handle, but currently this does not allow for 
>> inlining of the handles, since they are not constant.
>> 
>> Maybe a future C2 optimization could look at the receiver input for 
>> invokeBasic call sites, and if the input is a phi node, clone the call for 
>> each constant input of the phi. I believe that would allow simplifying the 
>> bytecode without giving up on inlining.
>> 
>> Some numbers from the added benchmarks:
>> Benchmark(numCases)  (offset)  
>> (sorted)  Mode  Cnt   Score   Error  Units
>> MethodHandlesTableSwitchConstant.testSwitch   5 0   
>> N/A  avgt   30   4.186 � 0.054  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch   5   150   
>> N/A  avgt   30   4.164 � 0.057  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  10 0   
>> N/A  avgt   30   4.124 � 0.023  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  10   150   
>> N/A  avgt   30   4.126 � 0.025  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  25 0   
>> N/A  avgt   30   4.137 � 0.042  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  25   150   
>> N/A  avgt   30   4.113 � 0.016  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  50 0   
>> N/A  avgt   30   4.118 � 0.028  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  50   150   
>> N/A  avgt   30   4.127 � 0.019  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 100 0   
>> N/A  avgt   30   4.116 � 0.013  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 100   150   
>> N/A  avgt   30   4.121 � 0.020  ms/op
>> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5 0   
>> N/A  avgt   30   4.113 � 0.009  ms/op
>> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5   150   
>> N/A  avgt   30   4.149 � 0.041  ms/op
>> 

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[Rémi Forax]

On Fri, 9 Apr 2021 19:57:10 GMT, Jorn Vernee  wrote:

>>> yes, for all the switches, pattern-switch, enum-switch but not for the 
>>> string switch which requires a lookup switch.
>> Can you outline how to use the tableswitch combinator in the case of a 
>> switch on strings ?
>> 
>> Jan Lahoda has made several good examples of that: 
>> https://github.com/lahodaj/jdk/blob/switch-bootstrap/src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
>>  i.e. several filtering strategies for translating a String into a table 
>> index (which can then be fed to `tableswitch`)
>> 
>> I ran some benchmarks:
>> 
>> ![results](http://cr.openjdk.java.net/~jvernee/StringSwitch_10.svg)
>> 
>> Here, 'legacy' is what C2 does with `lookupswitch`.
>> 
>> Maybe it's worth it to include such an example & benchmark in this patch as 
>> well (show off how to emulate `lookupswitch`)
>
> I've uploaded a benchmark that simulates a lookup switch using the 
> tableSwitch combinator as well, using a HashMap lookup as a filter: 
> https://github.com/openjdk/jdk/commit/a7157eb0ef1b7190aabfb2689539801c6692bbae
> 
> For that particular key set (the same as from the graph above), HashMap is 
> faster:
> 
> Benchmark  Mode  Cnt   Score   
> Error  Units
> MethodHandlesEmulateLookupSwitch.emulatedLookupSwitch  avgt   30  19.450 � 
> 0.079  ms/op
> MethodHandlesEmulateLookupSwitch.nativeLookupSwitchavgt   30  25.370 � 
> 0.159  ms/op
> 
> But, I've found it really depends on the key set. However, this is mostly to 
> demonstrate that emulation can have competitive performance with native 
> `lookupswitch`. e.g. to get constant folding for constant inputs another 
> filter has to be used, since C2 can not see through the HashMap lookups.

Ok, let restart from the beginning,
you have two strategy to de-sugar a switch,
- if what you do after the case do not mutate any variables, you can desugar 
each case to a method more or less like a lambda (it's not exactly like a 
lambda because there is no capture) and you have an indy in front that will 
call the right method handles
- you have a front end, with an indy that associate the object to an int and a 
backend which is tableswitch in the bytecode

The first strategy is an optimization, it will get you good performance by 
example if you compare a switch on a hirerachy on types and the equivalent 
method call. But you can not use that strategy for all switch, it's more an 
optimization.
The second strategy let you encode any switches.

The tests above are using the first strategy, which I think is not what we 
should implement by default given that it doesn't work will all cases. In the 
particular case of a switch on string, javac generates two switches, the front 
one and the back one, if we want to compare, we should implement the second 
strategy, so indy or the equivalent constant handle should take a String as 
parameter and return an int.

On the test themselves, for the hash, the Map should be directly bound, it will 
be more efficient, the asm version doesn't not appear in the graphics and there 
is a missing strategy that is using a MutableCallSite to switch from the a 
cascade of guards using the real values (store the String value even if it goes 
to `default`) and then switch to a lookup switch which i've found is the 
optimal strategy in real code (it works a lot like a bi-morphic cache but on 
string values instead of on classes).

-

PR: https://git.openjdk.java.net/jdk/pull/3401

Re: 8214761: Bug in parallel Kahan summation implementation

[Dalibor Topic]

Hi Anirvan,

You can contact contact Oracle’s open-source administrators at 
opensource_ww_...@oracle.com, who run the OCA app.


We don't use the github usernames from the OCA app for OpenJDK pull 
requests, so you should not be running into issues with that for 
contributions to OpenJDK. If you do, please let me know off-list which 
pull request is involved.


cheers,
dalibor topic

On 03.04.2021 22:11, Anirvan Sarkar wrote:

Hi Pavel,

I think I have the same issue as Chris.

The new OCA contributors list [1] is incorrectly linking the user names to
GitHub as compared to the old contributors list [2].
It seems the new OCA contributors list is assuming that the GitHub username
of past contributors is the same as the username of the project, when they
had signed the OCA in the past.

When I had signed OCA a few years back for the OpenJFX project, I had
provided my username as *anirvan.sarkar*.
This was my username on java.net / javafx-jira.kenai.com (the old JIRA for
JavaFX project).
But my GitHub username is *AnirvanSarkar*, not *anirvan.sarkar* which user
does not exist on GitHub.

[1] : https://oca.opensource.oracle.com/?ojr=contrib-list
[2] : https://oca.opensource.oracle.com/?ojr=old-contrib-list


On Sun, 4 Apr 2021 at 03:35, Pavel Rappo  wrote:


Hey Chris,

I don't know exactly what triggers removal of the "oca" and "oca-verify"
labels. The only OCA entry for Chris Dennis I could find [1] had a
different GitHub username. Did you mistype it or it belongs to another
person? Mind you, that person's GitHub page is 404.

-Pavel

[1] https://oca.opensource.oracle.com/?ojr=contrib-list


On 3 Apr 2021, at 16:12, Chris Dennis  wrote:

A gentle prod. Am I misunderstanding procedure here?

From: Chris Dennis 
Date: Monday, March 22, 2021 at 2:28 PM
To: core-libs-dev 
Subject: 8214761: Bug in parallel Kahan summation implementation
I created a PR for 8214761: https://github.com/openjdk/jdk/pull/2988 -

but have been stuck waiting on OCA signatory status to be confirmed. Did
something get lost in the shuffle or do I just need to be more patient.


Thanks,

Chris







--
 Dalibor Topic
Consulting Product Manager
Phone: +494089091214 , Mobile: +491737185961
, Video: dalibor.to...@oracle.com


Oracle Global Services Germany GmbH
Hauptverwaltung: Riesstr. 25, D-80992 München
Registergericht: Amtsgericht München, HRB 246209
Geschäftsführer: Ralf Herrmann

Re: RFR: 8264208: Console charset API

[Joe Wang]

On Fri, 9 Apr 2021 16:47:55 GMT, Naoto Sato  wrote:

> Please review the changes for the subject issue.  This has been suggested in 
> a recent discussion thread for the JEP 400 
> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>  A CSR has also been drafted, and comments are welcome 
> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].

Marked as reviewed by joehw (Reviewer).

src/java.base/share/classes/java/io/Console.java line 404:

> 402:  *
> 403:  * @return A {@code Charset} object used in this {@code Console}.
> 404:  * @since 17

A couple of minor comments: 
May replace {@code Charset} with @link;
Add "the" to the sentence: The returned charset corresponds to "the" input...
@return: javadoc guide suggests "do not capitalize and do not end with a 
period" when writing a phrase. But I guess for consistency in this class, it's 
okay to capitalize.

-

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8264806: Remove the experimental JIT compiler

[Erik Joelsson]

On Fri, 9 Apr 2021 17:35:11 GMT, Vladimir Kozlov  wrote:

> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to 
> Java-based JIT compiler (Graal) from JDK:
> 
> - `jdk.internal.vm.compiler` — the Graal compiler 
> - `jdk.internal.vm.compiler.management` — Graal's `MBean`
> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests
> 
> Remove Graal related code in makefiles.
> 
> Note, next two `module-info.java` files are preserved so that the JVMCI 
> module `jdk.internal.vm.ci` continues to build:
> src/jdk.internal.vm.compiler/share/classes/module-info.java
> src/jdk.internal.vm.compiler.management/share/classes/module-info.java
> 
> @AlanBateman suggested that we can avoid it by using Module API to export 
> packages at runtime . It requires changes in GraalVM's JVMCI too so I will 
> file followup RFE to implement it.
> 
> Tested hs-tier1-4

make/GraalBuilderImage.gmk line 1:

> 1: #

This file should not be removed. This outout image is this makefile produces is 
used as input to the separate GraalVM build.

make/Main.gmk line 444:

> 442: ))
> 443: 
> 444: $(eval $(call SetupTarget, graal-builder-image, \

Same as above, this needs to stay.

make/autoconf/spec.gmk.in line 895:

> 893: STATIC_LIBS_IMAGE_DIR := $(IMAGES_OUTPUTDIR)/$(STATIC_LIBS_IMAGE_SUBDIR)
> 894: 
> 895: # Graal builder image

Like above, this needs to stay.

-

PR: https://git.openjdk.java.net/jdk/pull/3421

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 16:54:35 GMT, Ioi Lam  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/oops/methodCounters.cpp line 77:
> 
>> 75: }
>> 76: 
>> 77: void MethodCounters::metaspace_pointers_do(MetaspaceClosure* it) {
> 
> MethodCounters::metaspace_pointers_do can be removed altogether (also need to 
> remove the declaration in methodCounter.hpp).

Done.

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 16:34:58 GMT, Igor Veresov  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/jvmci/jvmciCodeInstaller.cpp line 1184:
> 
>> 1182:   }
>> 1183: } else if 
>> (jvmci_env()->isa_HotSpotMetaspaceConstantImpl(constant)) {
>> 1184:   if (!_immutable_pic_compilation) {
> 
> All _immutable_pic_compilation mentions can be removed as well. It is true 
> only for AOT compiles produced by Graal. It's never going to be used without 
> AOT.

Done. I removed _immutable_pic_compilation in JVMCI in Hotspot.

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 16:30:41 GMT, Igor Veresov  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/oops/instanceKlass.hpp line 257:
> 
>> 255: _misc_declares_nonstatic_concrete_methods = 1 << 6,  // directly 
>> declares non-static, concrete methods
>> 256: _misc_has_been_redefined  = 1 << 7,  // class has 
>> been redefined
>> 257: _unused   = 1 << 8,  //
> 
> Any particular reason we don't want to remove this gap?

Less changes. We don't get any benefits from removing it. It is opposite - if 
we need a new value we will use it without changing following values again.

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 08:32:59 GMT, Aleksey Shipilev  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/code/compiledIC.cpp line 715:
> 
>> 713: tty->print("interpreted");
>> 714:   } else {
>> 715: tty->print("unknown");
> 
> We can probably split this cleanup into the minor issue, your call. The 
> benefit of separate issue: backportability.

Reverted and filed https://bugs.openjdk.java.net/browse/JDK-8265013

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[Jorn Vernee]

On Thu, 8 Apr 2021 18:51:21 GMT, Jorn Vernee  wrote:

> This patch adds a `tableSwitch` combinator that can be used to switch over a 
> set of method handles given an index, with a fallback in case the index is 
> out of bounds, much like the `tableswitch` bytecode. Here is a description of 
> how it works (copied from the javadoc):
> 
>  Creates a table switch method handle, which can be used to switch over a 
> set of target
>  method handles, based on a given target index, called selector.
> 
>  For a selector value of {@code n}, where {@code n} falls in the range 
> {@code [0, N)},
>  and where {@code N} is the number of target method handles, the table 
> switch method
>  handle will invoke the n-th target method handle from the list of target 
> method handles.
> 
>  For a selector value that does not fall in the range {@code [0, N)}, the 
> table switch
>  method handle will invoke the given fallback method handle.
> 
>  All method handles passed to this method must have the same type, with 
> the additional
>  requirement that the leading parameter be of type {@code int}. The 
> leading parameter
>  represents the selector.
> 
>  Any trailing parameters present in the type will appear on the returned 
> table switch
>  method handle as well. Any arguments assigned to these parameters will 
> be forwarded,
>  together with the selector value, to the selected method handle when 
> invoking it.
> 
> The combinator does not support specifying the starting index, so the switch 
> cases always run from 0 to however many target handles are specified. A 
> starting index can be added manually with another combination step that 
> filters the input index by adding or subtracting a constant from it, which 
> does not affect performance. One of the reasons for not supporting a starting 
> index is that it allows for more lambda form sharing, but also simplifies the 
> implementation somewhat. I guess an open question is if a convenience 
> overload should be added for that case?
> 
> Lookup switch can also be simulated by filtering the input through an 
> injection function that translates it into a case index, which has also 
> proven to have the ability to have comparable performance to, or even better 
> performance than, a bytecode-native `lookupswitch` instruction. I plan to add 
> such an injection function to the runtime libraries in the future as well. 
> Maybe at that point it could be evaluated if it's worth it to add a lookup 
> switch combinator as well, but I don't see an immediate need to include it in 
> this patch.
> 
> The current bytecode intrinsification generates a call for each switch case, 
> which guarantees full inlining of the target method handles. Alternatively we 
> could only have 1 callsite at the end of the switch, where each case just 
> loads the target method handle, but currently this does not allow for 
> inlining of the handles, since they are not constant.
> 
> Maybe a future C2 optimization could look at the receiver input for 
> invokeBasic call sites, and if the input is a phi node, clone the call for 
> each constant input of the phi. I believe that would allow simplifying the 
> bytecode without giving up on inlining.
> 
> Some numbers from the added benchmarks:
> Benchmark(numCases)  (offset)  
> (sorted)  Mode  Cnt   Score   Error  Units
> MethodHandlesTableSwitchConstant.testSwitch   5 0   
> N/A  avgt   30   4.186 � 0.054  ms/op
> MethodHandlesTableSwitchConstant.testSwitch   5   150   
> N/A  avgt   30   4.164 � 0.057  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  10 0   
> N/A  avgt   30   4.124 � 0.023  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  10   150   
> N/A  avgt   30   4.126 � 0.025  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  25 0   
> N/A  avgt   30   4.137 � 0.042  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  25   150   
> N/A  avgt   30   4.113 � 0.016  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  50 0   
> N/A  avgt   30   4.118 � 0.028  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  50   150   
> N/A  avgt   30   4.127 � 0.019  ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 0   
> N/A  avgt   30   4.116 � 0.013  ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100   150   
> N/A  avgt   30   4.121 � 0.020  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5 0   
> N/A  avgt   30   4.113 � 0.009  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5   150   
> N/A  avgt   30   4.149 � 0.041  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  10 0   
> N/A  avgt   30   4.121 � 0.026  ms/op
> 

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 03:03:33 GMT, David Holmes  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/memory/heap.hpp line 174:
> 
>> 172:   bool contains(const void* p) const { return low() <= p && 
>> p < high(); }
>> 173:   bool contains_blob(const CodeBlob* blob) const {
>> 174: const void* start = (void*)blob;
> 
> start seems redundant now

Done:
   bool contains_blob(const CodeBlob* blob) const {
-const void* start = (void*)blob;
-return contains(start);
+return contains((void*)blob);
   }

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 08:29:21 GMT, Aleksey Shipilev  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> src/hotspot/cpu/x86/globalDefinitions_x86.hpp line 73:
> 
>> 71: 
>> 72: #if INCLUDE_JVMCI
>> 73: #define COMPRESSED_CLASS_POINTERS_DEPENDS_ON_COMPRESSED_OOPS 
>> (EnableJVMCI)
> 
> Minor nit: can probably drop parentheses here.

done

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[forax]

- Mail original -
> De: "John Rose" 
> À: "Remi Forax" 
> Cc: "Jorn Vernee" , "core-libs-dev" 
> 
> Envoyé: Vendredi 9 Avril 2021 20:01:18
> Objet: Re: RFR: 8263087: Add a MethodHandle combinator that switches over a 
> set of MethodHandles

Hi John,

> On Apr 9, 2021, at 9:55 AM, Remi Forax  wrote:
>> 
>> I think the combinator should be lookupswitch which is more general than
>> tableswitch with a special case when generating the bytecode to generate a
>> tableswitch instead of a lookupswitch if the indexes are subsequent.
> 
> We can get there in the simpler steps Jorn has outlined.

I fail to see how it can work.

> 
> The combinator is much simpler if the case numbers are implicit in [0,N). Then
> it’s natural to filter on the [0,N) input as a separately factored choice.

An array of MethodHandles + a default method handle is simpler than an array of 
sorted ints + an array of MethodHandles + a default method, but not much 
simpler.

> That also scales to pattern-switch.

yes, for all the switches, pattern-switch, enum-switch but not for the string 
switch which requires a lookup switch.
Can you outline how to use the tableswitch combinator in the case of a switch 
on strings ?

> 
> I agree with the choice to have N call sites. It’s possible to build the one
> call site version on top using constant combinators but not vice versa.

yes,
Rémi

Integrated: 8264765: BreakIterator sees bogus sentence boundary in parenthesized “i.e.” phrase

[Naoto Sato]

On Thu, 8 Apr 2021 18:19:20 GMT, Naoto Sato  wrote:

> Please review the fix to the subject issue. It is not actually related to 
> "parenthesized", but period-comma sequence was regarded as a break on a 
> backward traverse.

This pull request has now been integrated.

Changeset: 9ebc497b
Author:Naoto Sato 
URL:   https://git.openjdk.java.net/jdk/commit/9ebc497b
Stats: 19 lines in 2 files changed: 14 ins; 0 del; 5 mod

8264765: BreakIterator sees bogus sentence boundary in parenthesized “i.e.” 
phrase

Reviewed-by: joehw

-

PR: https://git.openjdk.java.net/jdk/pull/3400

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[John Rose]

On Apr 9, 2021, at 9:55 AM, Remi Forax  wrote:
> 
> I think the combinator should be lookupswitch which is more general than 
> tableswitch with a special case when generating the bytecode to generate a 
> tableswitch instead of a lookupswitch if the indexes are subsequent.

We can get there in the simpler steps Jorn has outlined.

The combinator is much simpler if the case numbers are implicit in [0,N). Then 
it’s natural to filter on the [0,N) input as a separately factored choice. That 
also scales to pattern-switch. 

I agree with the choice to have N call sites. It’s possible to build the one 
call site version on top using constant combinators but not vice versa. 

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 02:44:23 GMT, David Holmes  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> src/hotspot/cpu/x86/compiledIC_x86.cpp line 134:
> 
>> 132: #ifdef ASSERT
>> 133:   CodeBlob *cb = CodeCache::find_blob_unsafe((address) _call);
>> 134:   assert(cb, "sanity");
> 
> Nit: implied boolean - use "cb != NULL"

done

-

PR: https://git.openjdk.java.net/jdk/pull/3398

RFR: 8264806: Remove the experimental JIT compiler

[Vladimir Kozlov]

As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to 
Java-based JIT compiler (Graal) from JDK:

- `jdk.internal.vm.compiler` — the Graal compiler 
- `jdk.internal.vm.compiler.management` — Graal's `MBean`
- `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests

Remove Graal related code in makefiles.

Note, next two `module-info.java` files are preserved so that the JVMCI module 
`jdk.internal.vm.ci` continues to build:
src/jdk.internal.vm.compiler/share/classes/module-info.java
src/jdk.internal.vm.compiler.management/share/classes/module-info.java

@AlanBateman suggested that we can avoid it by using Module API to export 
packages at runtime . It requires changes in GraalVM's JVMCI too so I will file 
followup RFE to implement it.

Tested hs-tier1-4

-

Depends on: https://git.openjdk.java.net/jdk/pull/3398

Commit messages:
 - 8264806: Remove the experimental JIT compiler

Changes: https://git.openjdk.java.net/jdk/pull/3421/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3421=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264806
  Stats: 441620 lines in 2886 files changed: 0 ins; 441604 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3421.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3421/head:pull/3421

PR: https://git.openjdk.java.net/jdk/pull/3421

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 17:09:58 GMT, Vladimir Kozlov  wrote:

>> Hi Vladimir,
>> 
>> This looks good to me - I went through all the files.
>> 
>> It was nice to see how nicely compartmentalized the AOT feature was - that 
>> made checking the changes quite simple. The one exception was the 
>> fingerprinting code, which for some reason was AOT-only but not marked that 
>> way, so I'm not sure what the story is there. ??
>> 
>> I was also surprised to see some of the flags were not marked experimental, 
>> so there will need to a CSR request to remove those without going through 
>> the normal deprecation process.
>> 
>> Thanks,
>> David
>
>> Hi Vladimir,
>> 
>> This looks good to me - I went through all the files.
>> 
>> It was nice to see how nicely compartmentalized the AOT feature was - that 
>> made checking the changes quite simple. The one exception was the 
>> fingerprinting code, which for some reason was AOT-only but not marked that 
>> way, so I'm not sure what the story is there. ??
>> 
>> I was also surprised to see some of the flags were not marked experimental, 
>> so there will need to a CSR request to remove those without going through 
>> the normal deprecation process.
>> 
>> Thanks,
>> David
> 
> Thank you, David.
> We thought that we could use fingerprint code for other cases that is why we 
> did not put it under `#if INCLUDE_AOT`.
> I filed CSR for AOT product flags removal: 
> https://bugs.openjdk.java.net/browse/JDK-8265000

Thank you, Igor Ignatyev, Igor Veresov, Ioi, Aleksey and Andrew for reviews.
I will update changes based on your comments.

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[Jorn Vernee]

On 09/04/2021 18:54, Remi Forax wrote:

- Mail original -

De: "Jorn Vernee" 
À: "core-libs-dev" 
Envoyé: Vendredi 9 Avril 2021 12:51:53
Objet: RFR: 8263087: Add a MethodHandle combinator that switches over a set of 
MethodHandles
This patch adds a `tableSwitch` combinator that can be used to switch over a set
of method handles given an index, with a fallback in case the index is out of
bounds, much like the `tableswitch` bytecode.

The combinator does not support specifying the starting index, so the switch
cases always run from 0 to however many target handles are specified. A
starting index can be added manually with another combination step that filters
the input index by adding or subtracting a constant from it, which does not
affect performance. One of the reasons for not supporting a starting index is
that it allows for more lambda form sharing, but also simplifies the
implementation somewhat. I guess an open question is if a convenience overload
should be added for that case?

I think the combinator should be lookupswitch which is more general than 
tableswitch with a special case when generating the bytecode to generate a 
tableswitch instead of a lookupswitch if the indexes are subsequent.
One of the bigger downsides I see in supporting lookupswitch directly is 
that the lambda form and intrinsified bytecode become dependent on the 
key set, which allows for less sharing. Something that is not/less of a 
problem with tableswitch + filter function, because the filter function 
could potentially be the same for any key set (where the key set is 
bound to the filter function instead).



Lookup switch can also be simulated by filtering the input through an injection
function that translates it into a case index, which has also proven to have
the ability to have comparable performance to, or even better performance than,
a bytecode-native `lookupswitch` instruction. I plan to add such an injection
function to the runtime libraries in the future as well. Maybe at that point it
could be evaluated if it's worth it to add a lookup switch combinator as well,
but I don't see an immediate need to include it in this patch.


As i said in the bug when we discuss about that the filtering function,
i believe that the filtering function for emulating lookupswitch is 
lookupswitch itself.
Right, but lookupswitch also ties us into C2's optimization strategy for 
lookupswitch. Having the ability to specify the filter function allows 
picking a better one for the particular use-case. For instance for 
switches with a large-ish number of cases (15+) it's faster to use a 
HashMap lookup as a filtering function (according to my benchmarking), 
with comparinble results to native lookupswitch if the filter function 
uses a tree of if/else.


Though, I'm not saying that it's not worth it to add a lookupswitch 
combinator as well, to me it seems like tableswitch is the more 
flexible/minimal primitive, because it doesn't force the use of a 
particular lookup strategy.


WRT picking the translation strategy based on the set of keys; I'm note 
super keen on that. Since the MethodHandle combinators are a low-level 
API, I ended up adopting a simple 'what you see is what you get' 
philosophy as much as possible, with the possibility of building other 
use-cases on top. i.e. a tableSwitch combinator that reliably translates 
into the tableswitch bytecode, a lookupSwitch combinator that reliably 
translates into the lookupswitch bytecode, and an exception if I get the 
key set wrong, rather than silently switching strategies to one or the 
other.



The current bytecode intrinsification generates a call for each switch case,
which guarantees full inlining of the target method handles. Alternatively we
could only have 1 callsite at the end of the switch, where each case just loads
the target method handle, but currently this does not allow for inlining of the
handles, since they are not constant.

This scheme also allows to never JIT compile a branch which is never used.


Yes, that's a good point, thanks.

Thanks for the input,
Jorn




Maybe a future C2 optimization could look at the receiver input for invokeBasic
call sites, and if the input is a phi node, clone the call for each constant
input of the phi. I believe that would allow simplifying the bytecode without
giving up on inlining.

Some numbers from the added benchmarks:
Benchmark(numCases)  (offset)  (sorted)
Mode  Cnt   Score   Error  Units
MethodHandlesTableSwitchConstant.testSwitch   5 0   N/A
avgt   30   4.186 � 0.054  ms/op
MethodHandlesTableSwitchConstant.testSwitch   5   150   N/A
avgt   30   4.164 � 0.057  ms/op
MethodHandlesTableSwitchConstant.testSwitch  10 0   N/A
avgt   30   4.124 � 0.023  ms/op
MethodHandlesTableSwitchConstant.testSwitch  10   150   N/A
avgt   30   4.126 � 0.025  ms/op

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Vladimir Kozlov]

On Fri, 9 Apr 2021 04:32:14 GMT, David Holmes  wrote:

> Hi Vladimir,
> 
> This looks good to me - I went through all the files.
> 
> It was nice to see how nicely compartmentalized the AOT feature was - that 
> made checking the changes quite simple. The one exception was the 
> fingerprinting code, which for some reason was AOT-only but not marked that 
> way, so I'm not sure what the story is there. ??
> 
> I was also surprised to see some of the flags were not marked experimental, 
> so there will need to a CSR request to remove those without going through the 
> normal deprecation process.
> 
> Thanks,
> David

Thank you, David.
We thought that we could use fingerprint code for other cases that is why we 
did not put it under `#if INCLUDE_AOT`.
I filed CSR for AOT product flags removal: 
https://bugs.openjdk.java.net/browse/JDK-8265000

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

[Marcin Grzejszczak]

I'm decorating existing user or framework code. If they have custom behavior, i 
want to respect that instead of override it.

Get Outlook for iOS

From: fo...@univ-mlv.fr 
Sent: Friday, April 9, 2021 6:33:10 PM
To: Marcin Grzejszczak 
Cc: core-libs-dev 
Subject: Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16




De: "Marcin Grzejszczak" 
À: "Remi Forax" 
Cc: "core-libs-dev" 
Envoyé: Vendredi 9 Avril 2021 17:41:28
Objet: Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16
That's the thing, I'm not using an agent. We're doing the wrapping at runtime 
by instrumenting spring beans.

Apart the fact that decorateTask is a good example of why it's better to use 
delegation than using the template method pattern.
Why do you need to call decorateTask() directly, and not execute/submit/invoke 
with your wrapped Runnable ?

Rémi


Pobierz aplikację Outlook dla systemu 
iOS

Od: Remi Forax 
Wysłane: Friday, April 9, 2021 5:18:11 PM
Do: Marcin Grzejszczak 
DW: core-libs-dev 
Temat: Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

- Mail original -
> De: "Marcin Grzejszczak" 
> À: "core-libs-dev" 
> Envoyé: Vendredi 9 Avril 2021 16:29:32
> Objet: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

> Hi!
>
> I'm the lead of Spring Cloud Sleuth [1], a project dedicated to working with
> distributed tracing. We're propagating the tracing context e.g. through
> threads. That means that when a user spawns a new thread we need to pass the
> context from the old thread to the new one. Example - if the user uses an
> Executor or an ExecutorService (e.g. via calling the execute(Runnable r)
> method) then we need to wrap the Runnable in its trace representation. That
> means that we retrieve the context from Thread A , pass it in the constructor
> of the TraceRunnable and then restore it once the run method is called in
> Thread B.
>
> The problem in Sleuth that we have with JDK16 is that we can't use reflection 
> to
> ensure that we're wrapping all methods of any Executors [2]. In other words we
> want to create a proxy around an existing Executor and wrap all methods.
> Currently, we're using reflection cause Executor implementations such as
> `ScheduledThreadPoolExecutor` have quite a few protected methods that we can't
> access. What we would like to achieve is a delegation mechanism that we can
> wrap all objects that the given Executor is using within their API (such as
> Runnables, Callables, other Executors) in their trace representation and then
> delegate calls for all methods to the wrapped object. That would also mean the
> delegation to currently protected methods.
>
> If there's another way to achieve this other than opening the
> java.util.concurrent API then I would very much like to use it. Currently with
> JDK16 it's not possible to instrument that code so context propagation might 
> be
> buggy when dealing with executors.

I'm not sure if you are using an agent or not, if you are using an agent, you 
can redefine a module
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F16%2Fdocs%2Fapi%2Fjava.instrument%2Fjava%2Flang%2Finstrument%2FInstrumentation.html%23redefineModuledata=04%7C01%7Cmgrzejszczak%40vmware.com%7Cc45688f3edb8423b9cae08d8fb6ab28a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637535782975891768%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000sdata=RHHLZTpU5RMNx7JHwkgG79%2FHWlXsXYOG8bBw9WaYQCU%3Dreserved=0(java.lang.Module,java.util.Set,java.util.Map,java.util.Map,java.util.Set,java.util.Map)

regards,
Rémi

>
> Marcin Grzejszczak
> Staff Engineer, Spring Cloud

Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results

[Brian Burkhalter]

On Fri, 9 Apr 2021 16:39:45 GMT, Brian Burkhalter  wrote:

>> Hello,
>> 
>> here's some background information for those that didn't follow the mailing 
>> list for the last couple of years.
>> 
>> Some enjoyable properties of the novel algorithm:
>> * No intermediate objects are instantiated.
>> * Loop-free core algorithm.
>> * Only int and long arithmetic.
>> * No divisions at all.
>> * 17.7x speedup (jmh) 
>> [1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-April/065921.html).
>> * Randomized, yet reproducible deep diving tests (jtreg).
>> * Clear, unambiguous spec.
>> * All floats have been tested to fully meet the spec.
>> * Fully documented in 
>> [2](https://drive.google.com/file/d/1luHhyQF9zKlM8yJ1nebU0OgVYhfC6CBN/view) 
>> and/or in comments.
>> 
>> See 
>> [3](https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-September/062580.html)
>>  for some (invented) Q The last Q deals with your investment in time 
>> for an informed review.
>> 
>> 
>> Greetings
>> Raffaello
>
> @rgiulietti Please issue the `/csr/` command here [1]. Speaking of which, 
> does the CSR [2] need to be updated?
> 
> [1] 
> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/csr
> [2] https://bugs.openjdk.java.net/browse/JDK-8202555

> @bplb No, the CSR [1] (https://bugs.openjdk.java.net/browse/JDK-8202555) 
> needs no updates.

OK, good. I wonder whether it should be moved back to `Draft` until someone 
else other than me has reviewed it?

-

PR: https://git.openjdk.java.net/jdk/pull/3402

RFR: 8264208: Console charset API

[Naoto Sato]

Please review the changes for the subject issue.  This has been suggested in a 
recent discussion thread for the JEP 400 
[[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
 A CSR has also been drafted, and comments are welcome 
[[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].

-

Commit messages:
 - 8264208: Console charset API

Changes: https://git.openjdk.java.net/jdk/pull/3419/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3419=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264208
  Stats: 202 lines in 9 files changed: 174 ins; 17 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3419.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Ioi Lam]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

LGTM. Just a small nit.

src/hotspot/share/oops/methodCounters.cpp line 77:

> 75: }
> 76: 
> 77: void MethodCounters::metaspace_pointers_do(MetaspaceClosure* it) {

MethodCounters::metaspace_pointers_do can be removed altogether (also need to 
remove the declaration in methodCounter.hpp).

-

Marked as reviewed by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[Remi Forax]

- Mail original -
> De: "Jorn Vernee" 
> À: "core-libs-dev" 
> Envoyé: Vendredi 9 Avril 2021 12:51:53
> Objet: RFR: 8263087: Add a MethodHandle combinator that switches over a set 
> of MethodHandles

> This patch adds a `tableSwitch` combinator that can be used to switch over a 
> set
> of method handles given an index, with a fallback in case the index is out of
> bounds, much like the `tableswitch` bytecode.
> 
> The combinator does not support specifying the starting index, so the switch
> cases always run from 0 to however many target handles are specified. A
> starting index can be added manually with another combination step that 
> filters
> the input index by adding or subtracting a constant from it, which does not
> affect performance. One of the reasons for not supporting a starting index is
> that it allows for more lambda form sharing, but also simplifies the
> implementation somewhat. I guess an open question is if a convenience overload
> should be added for that case?

I think the combinator should be lookupswitch which is more general than 
tableswitch with a special case when generating the bytecode to generate a 
tableswitch instead of a lookupswitch if the indexes are subsequent.

> 
> Lookup switch can also be simulated by filtering the input through an 
> injection
> function that translates it into a case index, which has also proven to have
> the ability to have comparable performance to, or even better performance 
> than,
> a bytecode-native `lookupswitch` instruction. I plan to add such an injection
> function to the runtime libraries in the future as well. Maybe at that point 
> it
> could be evaluated if it's worth it to add a lookup switch combinator as well,
> but I don't see an immediate need to include it in this patch.
> 

As i said in the bug when we discuss about that the filtering function,
i believe that the filtering function for emulating lookupswitch is 
lookupswitch itself.

> The current bytecode intrinsification generates a call for each switch case,
> which guarantees full inlining of the target method handles. Alternatively we
> could only have 1 callsite at the end of the switch, where each case just 
> loads
> the target method handle, but currently this does not allow for inlining of 
> the
> handles, since they are not constant.

This scheme also allows to never JIT compile a branch which is never used.

> 
> Maybe a future C2 optimization could look at the receiver input for 
> invokeBasic
> call sites, and if the input is a phi node, clone the call for each constant
> input of the phi. I believe that would allow simplifying the bytecode without
> giving up on inlining.
> 
> Some numbers from the added benchmarks:
> Benchmark(numCases)  (offset)  
> (sorted)
> Mode  Cnt   Score   Error  Units
> MethodHandlesTableSwitchConstant.testSwitch   5 0   
> N/A
> avgt   30   4.186 � 0.054  ms/op
> MethodHandlesTableSwitchConstant.testSwitch   5   150   
> N/A
> avgt   30   4.164 � 0.057  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  10 0   
> N/A
> avgt   30   4.124 � 0.023  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  10   150   
> N/A
> avgt   30   4.126 � 0.025  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  25 0   
> N/A
> avgt   30   4.137 � 0.042  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  25   150   
> N/A
> avgt   30   4.113 � 0.016  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  50 0   
> N/A
> avgt   30   4.118 � 0.028  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  50   150   
> N/A
> avgt   30   4.127 � 0.019  ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 0   
> N/A
> avgt   30   4.116 � 0.013  ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100   150   
> N/A
> avgt   30   4.121 � 0.020  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5 0   
> N/A
> avgt   30   4.113 � 0.009  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5   150   
> N/A
> avgt   30   4.149 � 0.041  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  10 0   
> N/A
> avgt   30   4.121 � 0.026  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  10   150   
> N/A
> avgt   30   4.113 � 0.021  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  25 0   
> N/A
> avgt   30   4.129 � 0.028  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  25   150   
> N/A
> avgt   30   4.105 � 0.019  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  50 0   
> N/A
> avgt   30   4.097 � 0.021  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  50   150   
> N/A
> avgt   30   4.131 � 0.037  

Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results

[Raffaello Giulietti]

On Fri, 9 Apr 2021 16:44:18 GMT, Raffaello Giulietti 
 wrote:

>> @rgiulietti Please issue the `/csr/` command here [1]. Speaking of which, 
>> does the CSR [2] need to be updated?
>> 
>> [1] 
>> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/csr
>> [2] https://bugs.openjdk.java.net/browse/JDK-8202555
>
> @bplb No, the CSR [1] (https://bugs.openjdk.java.net/browse/JDK-8202555) 
> needs no updates.

The langtools/tier1 test tools/javac/sym/ElementStructureTest.java fails on all 
4 platforms supported by the automatic GH actions.
Does anybody have a clue? Is this something I should be worried about?

Thanks

-

PR: https://git.openjdk.java.net/jdk/pull/3402

Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results

[Raffaello Giulietti]

On Fri, 9 Apr 2021 16:39:45 GMT, Brian Burkhalter  wrote:

>> Hello,
>> 
>> here's some background information for those that didn't follow the mailing 
>> list for the last couple of years.
>> 
>> Some enjoyable properties of the novel algorithm:
>> * No intermediate objects are instantiated.
>> * Loop-free core algorithm.
>> * Only int and long arithmetic.
>> * No divisions at all.
>> * 17.7x speedup (jmh) 
>> [1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-April/065921.html).
>> * Randomized, yet reproducible deep diving tests (jtreg).
>> * Clear, unambiguous spec.
>> * All floats have been tested to fully meet the spec.
>> * Fully documented in 
>> [2](https://drive.google.com/file/d/1luHhyQF9zKlM8yJ1nebU0OgVYhfC6CBN/view) 
>> and/or in comments.
>> 
>> See 
>> [3](https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-September/062580.html)
>>  for some (invented) Q The last Q deals with your investment in time 
>> for an informed review.
>> 
>> 
>> Greetings
>> Raffaello
>
> @rgiulietti Please issue the `/csr/` command here [1]. Speaking of which, 
> does the CSR [2] need to be updated?
> 
> [1] 
> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/csr
> [2] https://bugs.openjdk.java.net/browse/JDK-8202555

@bplb No, the CSR [1] (https://bugs.openjdk.java.net/browse/JDK-8202555) needs 
no updates.

-

PR: https://git.openjdk.java.net/jdk/pull/3402

Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results

[Brian Burkhalter]

On Fri, 9 Apr 2021 16:33:43 GMT, Raffaello Giulietti 
 wrote:

>> Forgot to add that other changes in the code are the use of switch 
>> expressions and the use of instanceof patterns.
>
> Hello,
> 
> here's some background information for those that didn't follow the mailing 
> list for the last couple of years.
> 
> Some enjoyable properties of the novel algorithm:
> * No intermediate objects are instantiated.
> * Loop-free core algorithm.
> * Only int and long arithmetic.
> * No divisions at all.
> * 17.7x speedup (jmh) 
> [1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-April/065921.html).
> * Randomized, yet reproducible deep diving tests (jtreg).
> * Clear, unambiguous spec.
> * All floats have been tested to fully meet the spec.
> * Fully documented in 
> [2](https://drive.google.com/file/d/1luHhyQF9zKlM8yJ1nebU0OgVYhfC6CBN/view) 
> and/or in comments.
> 
> See 
> [3](https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-September/062580.html)
>  for some (invented) Q The last Q deals with your investment in time for 
> an informed review.
> 
> 
> Greetings
> Raffaello

@rgiulietti Please issue the `/csr/` command here [1]. Speaking of which, does 
the CSR [2] need to be updated?

[1] 
https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/csr
[2] https://bugs.openjdk.java.net/browse/JDK-8202555

-

PR: https://git.openjdk.java.net/jdk/pull/3402

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Igor Veresov]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp line 1184:

> 1182:   }
> 1183: } else if (jvmci_env()->isa_HotSpotMetaspaceConstantImpl(constant)) 
> {
> 1184:   if (!_immutable_pic_compilation) {

All _immutable_pic_compilation mentions can be removed as well. It is true only 
for AOT compiles produced by Graal. It's never going to be used without AOT.

src/hotspot/share/oops/instanceKlass.hpp line 257:

> 255: _misc_declares_nonstatic_concrete_methods = 1 << 6,  // directly 
> declares non-static, concrete methods
> 256: _misc_has_been_redefined  = 1 << 7,  // class has 
> been redefined
> 257: _unused   = 1 << 8,  //

Any particular reason we don't want to remove this gap?

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results

[Raffaello Giulietti]

On Thu, 8 Apr 2021 21:20:34 GMT, Raffaello Giulietti 
 wrote:

>> Hello,
>> 
>> here's a PR for a patch submitted on March 2020 
>> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was 
>> a thing.
>> 
>> The patch has been edited to adhere to OpenJDK code conventions about 
>> multi-line (block) comments. Nothing in the code proper has changed, except 
>> for the addition of redundant but clarifying parentheses in some expressions.
>> 
>> 
>> Greetings
>> Raffaello
>
> Forgot to add that other changes in the code are the use of switch 
> expressions and the use of instanceof patterns.

Hello,

here's some background information for those that didn't follow the mailing 
list for the last couple of years.

Some enjoyable properties of the novel algorithm:
* No intermediate objects are instantiated.
* Loop-free core algorithm.
* Only int and long arithmetic.
* No divisions at all.
* 17.7x speedup (jmh) 
[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-April/065921.html).
* Randomized, yet reproducible deep diving tests (jtreg).
* Clear, unambiguous spec.
* All floats have been tested to fully meet the spec.
* Fully documented in 
[2](https://drive.google.com/file/d/1luHhyQF9zKlM8yJ1nebU0OgVYhfC6CBN/view) 
and/or in comments.

See 
[3](https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-September/062580.html)
 for some (invented) Q The last Q deals with your investment in time for 
an informed review.


Greetings
Raffaello

-

PR: https://git.openjdk.java.net/jdk/pull/3402

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Roger Riggs]

On Fri, 9 Apr 2021 15:47:35 GMT, Conor Cleary  wrote:

>> src/java.naming/share/classes/com/sun/jndi/ldap/LdapPoolManager.java line 
>> 413:
>> 
>>> 411: return AccessController.doPrivileged(
>>> 412: (PrivilegedAction) () -> Long.getLong(propName, 
>>> defVal).longValue()
>>> 413: );
>> 
>> And GetIntegerAction here. Though it only supports an int value.
>
> Thanks for the suggestion Roger, I think the `privilegedGetProperty(prop, 
> default)` for the `getProperty()` method looks great. 
> 
> WRT to using it for `getInt()` and `getLong()`, I think its reasonable to use 
> other means for these methods in the interest of consistency due to, as you 
> pointed out, only `int` being supported. Would you think? Or would it be 
> better to use the same means in all 3 methods?

Its a slippery slope that might require a bit more investigation to check the 
expected value sizes to see if a change to the number of bits in the value for 
each property might break something.  Technical debt can take you down a rabbit 
hole. Quickest to just convert to the lamba as you proposed.

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

[forax]

> De: "Marcin Grzejszczak" 
> À: "Remi Forax" 
> Cc: "core-libs-dev" 
> Envoyé: Vendredi 9 Avril 2021 17:41:28
> Objet: Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

> That's the thing, I'm not using an agent. We're doing the wrapping at runtime 
> by
> instrumenting spring beans.

Apart the fact that decorateTask is a good example of why it's better to use 
delegation than using the template method pattern. 
Why do you need to call decorateTask() directly, and not execute/submit/invoke 
with your wrapped Runnable ? 

Rémi 

> Pobierz aplikację [ https://aka.ms/o0ukef |
> Outlook dla systemu iOS ]

> Od: Remi Forax 
> Wysłane: Friday, April 9, 2021 5:18:11 PM
> Do: Marcin Grzejszczak 
> DW: core-libs-dev 
> Temat: Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16
> - Mail original -
> > De: "Marcin Grzejszczak" 
> > À: "core-libs-dev" 
> > Envoyé: Vendredi 9 Avril 2021 16:29:32
> > Objet: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

> > Hi!

> > I'm the lead of Spring Cloud Sleuth [1], a project dedicated to working with
> > distributed tracing. We're propagating the tracing context e.g. through
> > threads. That means that when a user spawns a new thread we need to pass the
> > context from the old thread to the new one. Example - if the user uses an
> > Executor or an ExecutorService (e.g. via calling the execute(Runnable r)
> > method) then we need to wrap the Runnable in its trace representation. That
> > means that we retrieve the context from Thread A , pass it in the 
> > constructor
> > of the TraceRunnable and then restore it once the run method is called in
> > Thread B.

> > The problem in Sleuth that we have with JDK16 is that we can't use 
> > reflection to
> > ensure that we're wrapping all methods of any Executors [2]. In other words 
> > we
> > want to create a proxy around an existing Executor and wrap all methods.
> > Currently, we're using reflection cause Executor implementations such as
> > `ScheduledThreadPoolExecutor` have quite a few protected methods that we 
> > can't
> > access. What we would like to achieve is a delegation mechanism that we can
> > wrap all objects that the given Executor is using within their API (such as
> > Runnables, Callables, other Executors) in their trace representation and 
> > then
> > delegate calls for all methods to the wrapped object. That would also mean 
> > the
> > delegation to currently protected methods.

> > If there's another way to achieve this other than opening the
> > java.util.concurrent API then I would very much like to use it. Currently 
> > with
> > JDK16 it's not possible to instrument that code so context propagation 
> > might be
> > buggy when dealing with executors.

> I'm not sure if you are using an agent or not, if you are using an agent, you
> can redefine a module
> [
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F16%2Fdocs%2Fapi%2Fjava.instrument%2Fjava%2Flang%2Finstrument%2FInstrumentation.html%23redefineModule=04%7C01%7Cmgrzejszczak%40vmware.com%7Cc45688f3edb8423b9cae08d8fb6ab28a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637535782975891768%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000=RHHLZTpU5RMNx7JHwkgG79%2FHWlXsXYOG8bBw9WaYQCU%3D=0(java.lang.Module,java.util.Set,java.util.Map,java.util.Map,java.util.Set,java.util.Map
> |
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F16%2Fdocs%2Fapi%2Fjava.instrument%2Fjava%2Flang%2Finstrument%2FInstrumentation.html%23redefineModuledata=04%7C01%7Cmgrzejszczak%40vmware.com%7Cc45688f3edb8423b9cae08d8fb6ab28a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637535782975891768%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000sdata=RHHLZTpU5RMNx7JHwkgG79%2FHWlXsXYOG8bBw9WaYQCU%3Dreserved=0(java.lang.Module,java.util.Set,java.util.Map,java.util.Map,java.util.Set,java.util.Map
> ] )

> regards,
> Rémi


> > Marcin Grzejszczak
> > Staff Engineer, Spring Cloud

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Conor Cleary]

On Fri, 9 Apr 2021 13:46:46 GMT, Roger Riggs  wrote:

>> ### Description
>> This fix is part of a previous effort to both cleanup/modernise JNDI code, 
>> the details of which can be seen in 
>> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number 
>> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where 
>> only a single object unique to the requirements of the method is used. The 
>> issues these occurrences of AICs cause are highlighted below.
>> 
>> - AICs, in the cases concerned with this fix, are used where only one 
>> operation is required. While AICs can be useful for more complex 
>> implementations (using interfaces, multiple methods needed, local fields 
>> etc.), Lambdas are better suited here as they result in a more readable and 
>> concise solution.
>> 
>> ### Fixes
>> - Where applicable, occurrences of AICs were replaced with lambdas to 
>> address the issues above resulting primarily in more readable/concise code.
>
> src/java.naming/share/classes/com/sun/jndi/ldap/LdapPoolManager.java line 413:
> 
>> 411: return AccessController.doPrivileged(
>> 412: (PrivilegedAction) () -> Long.getLong(propName, 
>> defVal).longValue()
>> 413: );
> 
> And GetIntegerAction here. Though it only supports an int value.

Thanks for the suggestion Roger, I think the `privilegedGetProperty(prop, 
default)` for the `getProperty()` method looks great. 

WRT to using it for `getInt()` and `getLong()`, I think its reasonable to use 
other means for these methods in the interest of consistency due to, as you 
pointed out, only `int` being supported. Would you think? Or would it be better 
to use the same means in all 3 methods?

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Conor Cleary]

On Fri, 9 Apr 2021 14:01:32 GMT, Roger Riggs  wrote:

>> That is a very neat alternative yes. Approaching the problem like that 
>> especially improves the readability 
>> [JdkLDAP.java](https://github.com/openjdk/jdk/pull/3416/files#diff-bf4c67da93cf2b9196508db2d57f7e01bc884f2268f5bfd43a9f01dfd55e4955)
>>  in my opinion. 
>> I don't think casting has any major performance hits for these fixes, so is 
>> your suggestion mostly for readability's sake?
>
> Right, not a performance problem.  Just simpler to use an existing method to 
> read a privileged property.
> And there will be one less doPriv.

I think I may change the PR to reflect what Alan suggested. Its more readable 
than the lambda-with-cast solution in that an action is created _and then_ an 
action is carried out as supposed to fitting it all into one call. Its also as 
concise.

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

[Marcin Grzejszczak]

That's the thing, I'm not using an agent. We're doing the wrapping at runtime 
by instrumenting spring beans.

Pobierz aplikację Outlook dla systemu iOS

Od: Remi Forax 
Wysłane: Friday, April 9, 2021 5:18:11 PM
Do: Marcin Grzejszczak 
DW: core-libs-dev 
Temat: Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

- Mail original -
> De: "Marcin Grzejszczak" 
> À: "core-libs-dev" 
> Envoyé: Vendredi 9 Avril 2021 16:29:32
> Objet: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

> Hi!
>
> I'm the lead of Spring Cloud Sleuth [1], a project dedicated to working with
> distributed tracing. We're propagating the tracing context e.g. through
> threads. That means that when a user spawns a new thread we need to pass the
> context from the old thread to the new one. Example - if the user uses an
> Executor or an ExecutorService (e.g. via calling the execute(Runnable r)
> method) then we need to wrap the Runnable in its trace representation. That
> means that we retrieve the context from Thread A , pass it in the constructor
> of the TraceRunnable and then restore it once the run method is called in
> Thread B.
>
> The problem in Sleuth that we have with JDK16 is that we can't use reflection 
> to
> ensure that we're wrapping all methods of any Executors [2]. In other words we
> want to create a proxy around an existing Executor and wrap all methods.
> Currently, we're using reflection cause Executor implementations such as
> `ScheduledThreadPoolExecutor` have quite a few protected methods that we can't
> access. What we would like to achieve is a delegation mechanism that we can
> wrap all objects that the given Executor is using within their API (such as
> Runnables, Callables, other Executors) in their trace representation and then
> delegate calls for all methods to the wrapped object. That would also mean the
> delegation to currently protected methods.
>
> If there's another way to achieve this other than opening the
> java.util.concurrent API then I would very much like to use it. Currently with
> JDK16 it's not possible to instrument that code so context propagation might 
> be
> buggy when dealing with executors.

I'm not sure if you are using an agent or not, if you are using an agent, you 
can redefine a module
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F16%2Fdocs%2Fapi%2Fjava.instrument%2Fjava%2Flang%2Finstrument%2FInstrumentation.html%23redefineModuledata=04%7C01%7Cmgrzejszczak%40vmware.com%7Cc45688f3edb8423b9cae08d8fb6ab28a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637535782975891768%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000sdata=RHHLZTpU5RMNx7JHwkgG79%2FHWlXsXYOG8bBw9WaYQCU%3Dreserved=0(java.lang.Module,java.util.Set,java.util.Map,java.util.Map,java.util.Set,java.util.Map)

regards,
Rémi

>
> Marcin Grzejszczak
> Staff Engineer, Spring Cloud

Re: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

[Remi Forax]

- Mail original -
> De: "Marcin Grzejszczak" 
> À: "core-libs-dev" 
> Envoyé: Vendredi 9 Avril 2021 16:29:32
> Objet: Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

> Hi!
> 
> I'm the lead of Spring Cloud Sleuth [1], a project dedicated to working with
> distributed tracing. We're propagating the tracing context e.g. through
> threads. That means that when a user spawns a new thread we need to pass the
> context from the old thread to the new one. Example - if the user uses an
> Executor or an ExecutorService (e.g. via calling the execute(Runnable r)
> method) then we need to wrap the Runnable in its trace representation. That
> means that we retrieve the context from Thread A , pass it in the constructor
> of the TraceRunnable and then restore it once the run method is called in
> Thread B.
> 
> The problem in Sleuth that we have with JDK16 is that we can't use reflection 
> to
> ensure that we're wrapping all methods of any Executors [2]. In other words we
> want to create a proxy around an existing Executor and wrap all methods.
> Currently, we're using reflection cause Executor implementations such as
> `ScheduledThreadPoolExecutor` have quite a few protected methods that we can't
> access. What we would like to achieve is a delegation mechanism that we can
> wrap all objects that the given Executor is using within their API (such as
> Runnables, Callables, other Executors) in their trace representation and then
> delegate calls for all methods to the wrapped object. That would also mean the
> delegation to currently protected methods.
> 
> If there's another way to achieve this other than opening the
> java.util.concurrent API then I would very much like to use it. Currently with
> JDK16 it's not possible to instrument that code so context propagation might 
> be
> buggy when dealing with executors.

I'm not sure if you are using an agent or not, if you are using an agent, you 
can redefine a module
https://docs.oracle.com/en/java/javase/16/docs/api/java.instrument/java/lang/instrument/Instrumentation.html#redefineModule(java.lang.Module,java.util.Set,java.util.Map,java.util.Map,java.util.Set,java.util.Map)

regards,
Rémi

> 
> Marcin Grzejszczak
> Staff Engineer, Spring Cloud

Re: RFR: 8261301: StringWriter.flush() is NOOP but documentation does not indicate it

[Brian Burkhalter]

On Thu, 8 Apr 2021 13:46:00 GMT, Roger Riggs  wrote:

>> The specification of the method `flush()` in the `java.io` classes 
>> `CharArrayWriter` and `StringWriter` is not explicit about the fact that the 
>> method has no effect. This request proposes to add to the specification of 
>> each flush() method the sentence
>> The {@code flush} method of {@code } does nothing.
>> The corresponding CSR is JDK-8264867.
>
> In the CSR, the first sentence of the Summary should be context free.
> It is used standalone in other contexts to describe the change.  Perhaps 
> change to:
> "Clarify the `flush` methods in java.io classes CharArrayWriter and 
> StringWriter do nothing."

@RogerRiggs I updated the CSR as you suggested, thanks!

-

PR: https://git.openjdk.java.net/jdk/pull/3383

Instrumenting Executors - issues in Spring Cloud Sleuth and JDK16

[Marcin Grzejszczak]

Hi!

I'm the lead of Spring Cloud Sleuth [1], a project dedicated to working with 
distributed tracing. We're propagating the tracing context e.g. through 
threads. That means that when a user spawns a new thread we need to pass the 
context from the old thread to the new one. Example - if the user uses an 
Executor or an ExecutorService (e.g. via calling the execute(Runnable r)​ 
method) then we need to wrap the Runnable​ in its trace representation. That 
means that we retrieve the context from Thread A , pass it in the constructor 
of the TraceRunnable​ and then restore it once the run​ method is called in 
Thread B.

The problem in Sleuth that we have with JDK16 is that we can't use reflection 
to ensure that we're wrapping all methods of any Executors [2]. In other words 
we want to create a proxy around an existing Executor and wrap all methods. 
Currently, we're using reflection cause Executor implementations such as 
`ScheduledThreadPoolExecutor` have quite a few protected methods that we can't 
access. What we would like to achieve is a delegation mechanism that we can 
wrap all objects that the given Executor is using within their API (such as 
Runnables, Callables, other Executors) in their trace representation and then 
delegate calls for all methods to the wrapped object. That would also mean the 
delegation to currently protected methods.

If there's another way to achieve this other than opening the 
java.util.concurrent API then I would very much like to use it. Currently with 
JDK16 it's not possible to instrument that code so context propagation might be 
buggy when dealing with executors.

[1] - https://spring.io/projects/spring-cloud-sleuth
Spring Cloud Sleuth
Adds trace and span ids to the Slf4J MDC, so you can extract all the logs from 
a given trace or span in a log aggregator. Instruments common ingress and 
egress points from Spring applications (servlet filter, rest template, 
scheduled actions, message channels, feign client).
spring.io



[2] - https://github.com/spring-cloud/spring-cloud-sleuth/issues/1897
[https://avatars.githubusercontent.com/u/7815877?s=400=4]
InaccessibleObjectException when calling getScheduledThreadPoolExecutor · Issue 
#1897 · 
spring-cloud/spring-cloud-sleuth
Describe the bug When calling the method 
LazyTraceThreadPoolTaskScheduler.getScheduledThreadPoolExecutor(), a new 
LazyTraceScheduledThreadPoolExecutor is created. The constructor of the pool 
execut...
github.com





Marcin Grzejszczak
Staff Engineer, Spring Cloud

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Roger Riggs]

On Fri, 9 Apr 2021 13:51:30 GMT, Conor Cleary  wrote:

>> An alternative here would be to use 
>> sun.security.action.privilegedGetProperty(prop, default).
>> The package is already exported from java.base to java.desktop, etc.
>
> That is a very neat alternative yes. Approaching the problem like that 
> especially improves the readability 
> [JdkLDAP.java](https://github.com/openjdk/jdk/pull/3416/files#diff-bf4c67da93cf2b9196508db2d57f7e01bc884f2268f5bfd43a9f01dfd55e4955)
>  in my opinion. 
> I don't think casting has any major performance hits for these fixes, so is 
> your suggestion mostly for readability's sake?

Right, not a performance problem.  Just simpler to use an existing method to 
read a privileged property.
And there will be one less doPriv.

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Conor Cleary]

On Fri, 9 Apr 2021 13:45:03 GMT, Roger Riggs  wrote:

>> src/java.naming/share/classes/com/sun/jndi/ldap/LdapPoolManager.java line 
>> 401:
>> 
>>> 399: return AccessController.doPrivileged(
>>> 400: (PrivilegedAction) () -> 
>>> System.getProperty(propName, defVal)
>>> 401: );
>> 
>> If you want to avoid the cast then you could create the PrivilegedAction 
>> explicitly, e.g.
>> 
>> PrivilegedAction pa = () -> System.getProperty(propName, defVal);
>> return AccessController.doPrivileged(pa);
>
> An alternative here would be to use 
> sun.security.action.privilegedGetProperty(prop, default).
> The package is already exported from java.base to java.desktop, etc.

That is a very neat alternative yes. Approaching the problem like that 
especially improves the readability 
[JdkLDAP.java](https://github.com/openjdk/jdk/pull/3416/files#diff-bf4c67da93cf2b9196508db2d57f7e01bc884f2268f5bfd43a9f01dfd55e4955)
 in my opinion. 
I don't think casting has any major performance hits for these fixes, so is 
your suggestion mostly for readability's sake?

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Roger Riggs]

On Fri, 9 Apr 2021 13:15:16 GMT, Conor Cleary  wrote:

> ### Description
> This fix is part of a previous effort to both cleanup/modernise JNDI code, 
> the details of which can be seen in 
> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number 
> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where 
> only a single object unique to the requirements of the method is used. The 
> issues these occurrences of AICs cause are highlighted below.
> 
> - AICs, in the cases concerned with this fix, are used where only one 
> operation is required. While AICs can be useful for more complex 
> implementations (using interfaces, multiple methods needed, local fields 
> etc.), Lambdas are better suited here as they result in a more readable and 
> concise solution.
> 
> ### Fixes
> - Where applicable, occurrences of AICs were replaced with lambdas to address 
> the issues above resulting primarily in more readable/concise code.

Looks good.

A general reminder about one important difference between an anonymous inner 
class and a lambda.
An anonymous has *identity* but a lambda does not; or only accidentally.
None of the uses here require identity.

src/java.naming/share/classes/com/sun/jndi/ldap/LdapPoolManager.java line 413:

> 411: return AccessController.doPrivileged(
> 412: (PrivilegedAction) () -> Long.getLong(propName, 
> defVal).longValue()
> 413: );

And GetIntegerAction here. Though it only supports an int value.

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Roger Riggs]

On Fri, 9 Apr 2021 13:30:27 GMT, Alan Bateman  wrote:

>> ### Description
>> This fix is part of a previous effort to both cleanup/modernise JNDI code, 
>> the details of which can be seen in 
>> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number 
>> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where 
>> only a single object unique to the requirements of the method is used. The 
>> issues these occurrences of AICs cause are highlighted below.
>> 
>> - AICs, in the cases concerned with this fix, are used where only one 
>> operation is required. While AICs can be useful for more complex 
>> implementations (using interfaces, multiple methods needed, local fields 
>> etc.), Lambdas are better suited here as they result in a more readable and 
>> concise solution.
>> 
>> ### Fixes
>> - Where applicable, occurrences of AICs were replaced with lambdas to 
>> address the issues above resulting primarily in more readable/concise code.
>
> src/java.naming/share/classes/com/sun/jndi/ldap/LdapPoolManager.java line 401:
> 
>> 399: return AccessController.doPrivileged(
>> 400: (PrivilegedAction) () -> 
>> System.getProperty(propName, defVal)
>> 401: );
> 
> If you want to avoid the cast then you could create the PrivilegedAction 
> explicitly, e.g.
> 
> PrivilegedAction pa = () -> System.getProperty(propName, defVal);
> return AccessController.doPrivileged(pa);

An alternative here would be to use 
sun.security.action.privilegedGetProperty(prop, default).
The package is already exported from java.base to java.desktop, etc.

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Alan Bateman]

On Fri, 9 Apr 2021 13:15:16 GMT, Conor Cleary  wrote:

> ### Description
> This fix is part of a previous effort to both cleanup/modernise JNDI code, 
> the details of which can be seen in 
> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number 
> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where 
> only a single object unique to the requirements of the method is used. The 
> issues these occurrences of AICs cause are highlighted below.
> 
> - AICs, in the cases concerned with this fix, are used where only one 
> operation is required. While AICs can be useful for more complex 
> implementations (using interfaces, multiple methods needed, local fields 
> etc.), Lambdas are better suited here as they result in a more readable and 
> concise solution.
> 
> ### Fixes
> - Where applicable, occurrences of AICs were replaced with lambdas to address 
> the issues above resulting primarily in more readable/concise code.

src/java.naming/share/classes/com/sun/jndi/ldap/LdapPoolManager.java line 401:

> 399: return AccessController.doPrivileged(
> 400: (PrivilegedAction) () -> 
> System.getProperty(propName, defVal)
> 401: );

If you want to avoid the cast then you could create the PrivilegedAction 
explicitly, e.g.

PrivilegedAction pa = () -> System.getProperty(propName, defVal);
return AccessController.doPrivileged(pa);

-

PR: https://git.openjdk.java.net/jdk/pull/3416

RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Conor Cleary]

### Description
This fix is part of a previous effort to both cleanup/modernise JNDI code, the 
details of which can be seen in 
[JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number JNDI 
methods under `java.naming` use Anonymous Inner Classes in cases where only a 
single object unique to the requirements of the method is used. The issues 
these occurrences of AICs cause are highlighted below.

- AICs, in the cases concerned with this fix, are used where only one operation 
is required. While AICs can be useful for more complex implementations (using 
interfaces, multiple methods needed, local fields etc.), Lambdas are better 
suited here as they result in a more readable and concise solution.

### Fixes
- Where applicable, occurrences of AICs were replaced with lambdas to address 
the issues above resulting primarily in more readable/concise code.

-

Commit messages:
 - 8048199: Replace anonymous inner classes with lambdas, where applicable, in 
JNDI

Changes: https://git.openjdk.java.net/jdk/pull/3416/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3416=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8048199
  Stats: 75 lines in 5 files changed: 0 ins; 42 del; 33 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3416.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3416/head:pull/3416

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: RFR: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded

[Alan Bateman]

On Wed, 7 Apr 2021 15:45:30 GMT, Chris Hegarty  wrote:

> Avoid overflow when calculating the number of pages for large mapped segment 
> sizes.

Looks okay to me.

-

Marked as reviewed by alanb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3378

RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles

[Jorn Vernee]

This patch adds a `tableSwitch` combinator that can be used to switch over a 
set of method handles given an index, with a fallback in case the index is out 
of bounds, much like the `tableswitch` bytecode.

The combinator does not support specifying the starting index, so the switch 
cases always run from 0 to however many target handles are specified. A 
starting index can be added manually with another combination step that filters 
the input index by adding or subtracting a constant from it, which does not 
affect performance. One of the reasons for not supporting a starting index is 
that it allows for more lambda form sharing, but also simplifies the 
implementation somewhat. I guess an open question is if a convenience overload 
should be added for that case?

Lookup switch can also be simulated by filtering the input through an injection 
function that translates it into a case index, which has also proven to have 
the ability to have comparable performance to, or even better performance than, 
a bytecode-native `lookupswitch` instruction. I plan to add such an injection 
function to the runtime libraries in the future as well. Maybe at that point it 
could be evaluated if it's worth it to add a lookup switch combinator as well, 
but I don't see an immediate need to include it in this patch.

The current bytecode intrinsification generates a call for each switch case, 
which guarantees full inlining of the target method handles. Alternatively we 
could only have 1 callsite at the end of the switch, where each case just loads 
the target method handle, but currently this does not allow for inlining of the 
handles, since they are not constant.

Maybe a future C2 optimization could look at the receiver input for invokeBasic 
call sites, and if the input is a phi node, clone the call for each constant 
input of the phi. I believe that would allow simplifying the bytecode without 
giving up on inlining.

Some numbers from the added benchmarks:
Benchmark(numCases)  (offset)  (sorted) 
 Mode  Cnt   Score   Error  Units
MethodHandlesTableSwitchConstant.testSwitch   5 0   N/A 
 avgt   30   4.186 � 0.054  ms/op
MethodHandlesTableSwitchConstant.testSwitch   5   150   N/A 
 avgt   30   4.164 � 0.057  ms/op
MethodHandlesTableSwitchConstant.testSwitch  10 0   N/A 
 avgt   30   4.124 � 0.023  ms/op
MethodHandlesTableSwitchConstant.testSwitch  10   150   N/A 
 avgt   30   4.126 � 0.025  ms/op
MethodHandlesTableSwitchConstant.testSwitch  25 0   N/A 
 avgt   30   4.137 � 0.042  ms/op
MethodHandlesTableSwitchConstant.testSwitch  25   150   N/A 
 avgt   30   4.113 � 0.016  ms/op
MethodHandlesTableSwitchConstant.testSwitch  50 0   N/A 
 avgt   30   4.118 � 0.028  ms/op
MethodHandlesTableSwitchConstant.testSwitch  50   150   N/A 
 avgt   30   4.127 � 0.019  ms/op
MethodHandlesTableSwitchConstant.testSwitch 100 0   N/A 
 avgt   30   4.116 � 0.013  ms/op
MethodHandlesTableSwitchConstant.testSwitch 100   150   N/A 
 avgt   30   4.121 � 0.020  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch   5 0   N/A 
 avgt   30   4.113 � 0.009  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch   5   150   N/A 
 avgt   30   4.149 � 0.041  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch  10 0   N/A 
 avgt   30   4.121 � 0.026  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch  10   150   N/A 
 avgt   30   4.113 � 0.021  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch  25 0   N/A 
 avgt   30   4.129 � 0.028  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch  25   150   N/A 
 avgt   30   4.105 � 0.019  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch  50 0   N/A 
 avgt   30   4.097 � 0.021  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch  50   150   N/A 
 avgt   30   4.131 � 0.037  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch 100 0   N/A 
 avgt   30   4.135 � 0.025  ms/op
MethodHandlesTableSwitchOpaqueSingle.testSwitch 100   150   N/A 
 avgt   30   4.139 � 0.145  ms/op
MethodHandlesTableSwitchRandom.testSwitch 5 0  true 
 avgt   30   4.894 � 0.028  ms/op
MethodHandlesTableSwitchRandom.testSwitch 5 0 false 
 avgt   30  11.526 � 0.194  ms/op
MethodHandlesTableSwitchRandom.testSwitch 5   150  true 
 avgt   30   4.882 � 0.025  ms/op
MethodHandlesTableSwitchRandom.testSwitch 5   150 false 
 avgt   30  11.532 � 0.034  ms/op
MethodHandlesTableSwitchRandom.testSwitch10 0  true 
 avgt   30   5.065 � 0.076  ms/op

Re: RFR: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded

[Chris Hegarty]

On Wed, 7 Apr 2021 15:45:30 GMT, Chris Hegarty  wrote:

> Avoid overflow when calculating the number of pages for large mapped segment 
> sizes.

Note for reviewers on the test. A 3GB file is sufficient to demonstrate the 
issue in the old code, as follows (with a 4K page size, since the narrowing 
cast of size is the significant issue):

jshell> int pageSize() { return 4 * 1024; }
|  created method pageSize()

jshell> int pageCount(long size) { return (int)(size + (long)pageSize() - 1L) / 
pageSize(); }
|  created method pageCount(long)

jshell> pageCount(3L * 1024L * 1024L * 1024L)
$3 ==> -262143

-

PR: https://git.openjdk.java.net/jdk/pull/3378

RFR: 8264976: Minor numeric bug in AbstractSplittableWithBrineGenerator.makeSplitsSpliterator

[Aleksey Shipilev]

SonarCloud reports:
  Cast one of the operands of this subtraction operation to a "long".

Here:
Spliterator makeSplitsSpliterator(long index, long 
fence, SplittableGenerator source) {
...
long multiplier = (1 << SALT_SHIFT) - 1; // < here

The shift is integer, and the cast to long is too late. `SALT_SHIFT` is 
currently 4, so this is not the problem. But it would become a problem if 
`SALT_SHIFT` ever becomes 32 or larger. The shift operand should be `1L` for 
safety. Observe:

jshell> Long.toHexString((1 << 31) - 1)
$2 ==> "7fff"

jshell> Long.toHexString((1 << 32) - 1)
$3 ==> "0"

jshell> Long.toHexString((1L << 32) - 1)
$4 ==> ""

Additional testing: 
 - [x] Linux x86_64 fastdebug, `jdk/utils/Random`

-

Commit messages:
 - 8264976: Minor numeric bug in 
AbstractSplittableWithBrineGenerator.makeSplitsSpliterator

Changes: https://git.openjdk.java.net/jdk/pull/3409/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3409=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264976
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3409.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3409/head:pull/3409

PR: https://git.openjdk.java.net/jdk/pull/3409

RFR: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded

[Chris Hegarty]

Avoid overflow when calculating the number of pages for large mapped segment 
sizes.

-

Commit messages:
 - Test update
 - Initial changes

Changes: https://git.openjdk.java.net/jdk/pull/3378/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3378=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264827
  Stats: 32 lines in 5 files changed: 19 ins; 0 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3378.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3378/head:pull/3378

PR: https://git.openjdk.java.net/jdk/pull/3378

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Aleksey Shipilev]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

Shenandoah parts look good. I have a few minor comments after reading the rest.

src/hotspot/cpu/x86/globalDefinitions_x86.hpp line 73:

> 71: 
> 72: #if INCLUDE_JVMCI
> 73: #define COMPRESSED_CLASS_POINTERS_DEPENDS_ON_COMPRESSED_OOPS (EnableJVMCI)

Minor nit: can probably drop parentheses here.

src/hotspot/share/code/compiledIC.cpp line 715:

> 713: tty->print("interpreted");
> 714:   } else {
> 715: tty->print("unknown");

We can probably split this cleanup into the minor issue, your call. The benefit 
of separate issue: backportability.

-

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Andrew Haley]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

AArch64 looks fine.

-

Marked as reviewed by aph (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3398