Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v4]

[David Holmes]

On Mon, 4 Mar 2024 23:44:14 GMT, Erik Joelsson  wrote:

>> Executables and dynamic libraries on Linux can encode a search path that the 
>> dynamic linker will use when looking up library dependencies, generally 
>> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature 
>> to set search paths relative to the location of the binary itself. Typically 
>> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find 
>> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to 
>> find each other.
>> 
>> There are two different types of such rpaths, RPATH and RUNPATH. The former 
>> is the earlier incantation but RUNPATH has been around since about 2003 and 
>> has been default in prominent Linux distros for a long time, and now also 
>> seems to be default in the linker directly from binutils. The toolchain used 
>> by Oracle defaulted to RPATH until at least JDK 11, but since then with some 
>> toolchain upgrade, the default was flipped to RUNPATH.
>> 
>> The main (relevant in this case) difference between the two is that RPATH is 
>> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is 
>> only considered after LD_LIBRARY_PATH. For libraries that are part of a 
>> Linux distribution, letting users, or the system, control and override 
>> builtin rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. 
>> However, for the JDK, there really is no usecase for having an externally 
>> configured LD_LIBRARY_PATH potentially getting in the way of the JDK 
>> libraries finding each other correctly. If a user environment sets 
>> LD_LIBRARY_PATH, and there is a library in that path with the same name as a 
>> JDK library (e.g. libnet.so or some other generically named library) that 
>> external library will be loaded instead of the JDK internal library and that 
>> is basically guaranteed to break the JDK. There is no supported usecase that 
>> I can think of for injecting other versions of such libraries in a JDK 
>> distribution.
>> 
>> I propose that we explicitly configure the JDK build to set RPATH instead of 
>> RUNPATH for Linux binaries. This is done with the linker flag 
>> "--disable-new-dtags".
>
> Erik Joelsson has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   bug ref

Thanks for the further explanation and adding the comment.

LGTM.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18050#pullrequestreview-1916027301

Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v6]

[Joe Wang]

On Mon, 4 Mar 2024 19:07:44 GMT, Naoto Sato  wrote:

>> This PR intends to remove the legacy `COMPAT` locale data from the JDK. The 
>> `COMPAT` locale data was introduced for applications' migratory purposes 
>> transitioning to `CLDR`. It is becoming a technical debt and now is the time 
>> to remove it (we've been emitting a warning at JVM startup since JDK21, if 
>> the app is using `COMPAT`). A corresponding CSR has also been drafted.
>
> Naoto Sato 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 44 additional commits since 
> the last revision:
> 
>  - Merge branch 'master' into JDK-8174269-COMPAT-Removal
>  - Addressing review comments
>  - Update test/jdk/java/text/Format/DateFormat/Bug6683975.java
>
>Co-authored-by: Justin Lu 
>  - Remove `GensrcLocaleData.gmk`
>  - Update make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java
>
>Co-authored-by: Andrey Turbanov 
>  - cleanup
>  - BreakIteratorProvider available locales fix
>  - clean-up
>  - test fixes
>  - makeZoneData.pl fix
>  - ... and 34 more: https://git.openjdk.org/jdk/compare/9da59104...b771e303

LGTM. This is a lot of work. Looking through the files alone takes hours. Kudos 
to the great work!

I kind of like some of the date formats in COMPACT to be honest :-)

make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java line 1320:

> 1318:  * "USdst" -> "D"
> 1319:  *
> 1320:  * `tzdbLinks` retains `Link`s of time zones. if the value

nit, "if the value" seems to be an unfinished sentence.

src/java.base/share/classes/sun/util/locale/provider/BaseLocaleDataMetaInfo.java
 line 31:

> 29:  * It is used to avoid loading non-existent localized resources so that
> 30:  * jar files won't be opened unnecessary to look up them.
> 31:  */

nit: move the class description to right above the class? "unnecessary" -> 
"unnecessarily"

-

Marked as reviewed by joehw (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17991#pullrequestreview-1915980264
PR Comment: https://git.openjdk.org/jdk/pull/17991#issuecomment-1978071713
PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1512206671
PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1512207766

Re: RFR: 8327208: Remove unused method java.util.jar.Manifest.make72Safe

[Jaikiran Pai]

On Mon, 4 Mar 2024 09:55:09 GMT, Eirik Bjørsnøs  wrote:

> Please review this cleanup PR which suggests we remove the package-private, 
> unused and deprecated method `java.util.jar.Manifest.make72Safe`.
> 
> This method was marked deprecated after a cleanup/refactoring in 
> [JDK-8066619](https://bugs.openjdk.org/browse/JDK-8066619) caused it to fall 
> out of use. It should rather be removed.
> 
> Some tests reference the `make72Safe` methods in comment, or implement the 
> legacy versions of the same logic to simulate pre JDK 11 line break behavior 
> of `Manifest.write`. These comments and method names are adjusted to not 
> reference the now removed method.
> 
> This change was initially discussed here: 
> https://mail.openjdk.org/pipermail/core-libs-dev/2024-February/119819.html

Hello Eirik, the removal of this internal unused package private method and the 
updated comments in the tests look good to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18104#pullrequestreview-1915846886

Re: [External] : Re: jpackage requests permission via a dialog

[Alexander Matveev]

Hi Michael,

.DS_Store file contains information about icon position and other visual 
information on how to display folder. Once osascript configures DMG, Finder 
will store these setting in .DS_Store file. If it gets deleted from DMG, DMG 
will look like osascript was never run. So, Alan is right with “the 
configuration of the DMG appearance actually means configuring the .DS_Store 
file in the DMG”.

Thanks,
Alexander

From: Michael Hall 
Date: Monday, March 4, 2024 at 7:44 PM
To: Alan Snyder 
Cc: Alexander Matveev , core-libs-dev@openjdk.org 

Subject: [External] : Re: jpackage requests permission via a dialog
I’m not following what the .DS_Store file has to do with anything, other than 
being a Mac feature that’s occasionally a nuisance when the files end up in 
zip’s or elsewhere where you don’t want them.

I’m also not sure if that was my explanation or Alexander’s you’re talking 
about. Using the osascript to run the dmg gives the dmg a look more like other 
distributed macOS applications.

If that doesn’t matter to you then you could use jpackage to create a 
standalone app image and hdiutil a dmg for it. Opening it will show the files, 
period. Or use Disk Utility to create a dmg would also work. I’ve done that 
sometimes. Then you don’t have to remember the hdiutil command line usage.

> On Mar 4, 2024, at 5:50 PM, Alan Snyder  wrote:
>
> Thank you for the explanation.
>
> It was not obvious to me that the configuration of the DMG appearance 
> actually means configuring the .DS_Store file in the DMG.
>
>   Alan
>

Re: jpackage requests permission via a dialog

[Michael Hall]

I’m not following what the .DS_Store file has to do with anything, other than 
being a Mac feature that’s occasionally a nuisance when the files end up in 
zip’s or elsewhere where you don’t want them. 

I’m also not sure if that was my explanation or Alexander’s you’re talking 
about. Using the osascript to run the dmg gives the dmg a look more like other 
distributed macOS applications. 

If that doesn’t matter to you then you could use jpackage to create a 
standalone app image and hdiutil a dmg for it. Opening it will show the files, 
period. Or use Disk Utility to create a dmg would also work. I’ve done that 
sometimes. Then you don’t have to remember the hdiutil command line usage. 

> On Mar 4, 2024, at 5:50 PM, Alan Snyder  wrote:
> 
> Thank you for the explanation.
> 
> It was not obvious to me that the configuration of the DMG appearance 
> actually means configuring the .DS_Store file in the DMG.
> 
>   Alan
> 

Re: RFR: 8327261: Parsing test for Double/Float succeeds w/o testing all bad cases

[Joe Darcy]

On Tue, 5 Mar 2024 00:51:33 GMT, Naoto Sato  wrote:

> Fixing test cases. For bad test cases, only the first case was run, and the 
> rest were ignored.

Looks fine; thanks for sending out the fix.

test/jdk/java/lang/Double/ParseDouble.java line 559:

> 557: private static void testParsing(String [] input,
> 558: boolean exceptionalInput) {
> 559: for(int i = 0; i < input.length; i++) {

As you're updating the file, I think changing the loop to an enhanced for loop 
would be an improvement.

-

Marked as reviewed by darcy (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18113#pullrequestreview-1915727056
PR Review Comment: https://git.openjdk.org/jdk/pull/18113#discussion_r1512050441

RFR: 8327261: Parsing test for Double/Float succeeds w/o testing all bad cases

[Naoto Sato]

Fixing test cases. For bad test cases, only the first case was run, and the 
rest were ignored.

-

Commit messages:
 - initial commit

Changes: https://git.openjdk.org/jdk/pull/18113/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18113&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8327261
  Stats: 10 lines in 2 files changed: 0 ins; 4 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/18113.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18113/head:pull/18113

PR: https://git.openjdk.org/jdk/pull/18113

Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string

[Justin Lu]

On Sun, 3 Mar 2024 05:00:36 GMT, Guoxiong Li  wrote:

>> Please review this PR and corresponding CSR which prevents an 
>> OutOfMemoryError by restricting the initial maximum fraction digits for an 
>> empty pattern DecimalFormat.
>> 
>> For an empty String pattern DecimalFormat, the maximum fraction digits is 
>> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, 
>> StringBuilder internally doubles capacity attempting to append 
>> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral 
>> compatibility changes.
>
>> When toPattern() is invoked, StringBuilder internally doubles capacity 
>> attempting to append Integer.MAX_VALUE digits until OOME occurs.
> 
> It seems a bug in `toPattern` or `StringBuilder`? May be better to 
> investigate more about it.

Hi @lgxbslgx,

For clarification, this is entirely a bug with DecimalFormat, not 
StringBuilder. An empty String pattern DecimalFormat sets the maximum fraction 
digits to `Integer.MAX_VALUE`. When toPattern() is invoked, the local 
StringBuilder will append until an OOME is thrown by the StringBuilder as there 
is not enough memory, when internally, the buffer is doubled for a value too 
large. But such an OOME would occur for any large enough value, so the issue 
lies with DecimalFormat.

-

PR Comment: https://git.openjdk.org/jdk/pull/18094#issuecomment-1977712428

Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]

[Justin Lu]

On Mon, 4 Mar 2024 16:03:55 GMT, Roger Riggs  wrote:

>> Justin Lu 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 five additional commits 
>> since the last revision:
>> 
>>  - cleanup/typo
>>  - Merge branch 'master' into JDK-8326908-emptyPattern-OOME
>>  - implement feedback + improve pattern related tests
>>  - minor additions
>>  - init
>
> src/java.base/share/classes/java/text/DecimalFormat.java line 3717:
> 
>> 3715: // As maxFracDigits are fully displayed unlike maxIntDigits
>> 3716: // Prevent OOME by setting to a much more reasonable value.
>> 3717: setMaximumFractionDigits(DOUBLE_FRACTION_DIGITS);
> 
> Setting a reasonable default makes sense.  
> In other control paths, the max fraction digits come from the inputs or are 
> explicitly set.
> 
> It might be a reasonable related change to use StringBuilder.repeat() instead 
> of a loop at LIne 3312-3319, where the pattern char(s) are being appended to 
> the result.

Thanks for taking a look. Updated the loop with `repeat` as you suggested, and 
decided to refactor the rest of the method while I was at it.

Additionally, I added some more tests, as it seems that there is a lack of 
pattern tests for DecimalFormat in general.

> In other control paths, the max fraction digits come from the inputs or are 
> explicitly set.

Right, while `Integer.MAX_VALUE` can still be set by other control paths (and 
thus an OOME if toPattern() is invoked), this is something explicitly done by 
the user, and thus we decided we would still allow the behavior.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1511966791

Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]

[Justin Lu]

> Please review this PR and corresponding CSR which prevents an 
> OutOfMemoryError by restricting the initial maximum fraction digits for an 
> empty pattern DecimalFormat.
> 
> For an empty String pattern DecimalFormat, the maximum fraction digits is 
> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, 
> StringBuilder internally doubles capacity attempting to append 
> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral 
> compatibility changes.

Justin Lu 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 five additional commits since the 
last revision:

 - cleanup/typo
 - Merge branch 'master' into JDK-8326908-emptyPattern-OOME
 - implement feedback + improve pattern related tests
 - minor additions
 - init

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18094/files
  - new: https://git.openjdk.org/jdk/pull/18094/files/fc8a7ce4..7ca56500

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=00-01

  Stats: 5885 lines in 919 files changed: 2483 ins; 1248 del; 2154 mod
  Patch: https://git.openjdk.org/jdk/pull/18094.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094

PR: https://git.openjdk.org/jdk/pull/18094

Re: RFR: 8325567: jspawnhelper without args fails with segfault

[Vladimir Petko]

On Mon, 4 Mar 2024 23:23:04 GMT, Roger Riggs  wrote:

>> This change is intended to address the segmentation fault issue that occurs 
>> when jspawnhelper is called without arguments, and it includes an updated 
>> test to verify the behavior in such cases.
>> 
>> Existing tests passes since it does not check behavior without args.
>> After test update the test fails without 
>> 
>>if (argc != 2) {
>> shutItDown();
>> }
>> 
>> code block
>> 
>>> Test results: failed: 1
>>> Report written to 
>>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>>> Results written to 
>>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>> Error: Some tests failed or other problems occurred.
>>> Finished running test 
>>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>>> Test report is stored in 
>>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>>  
>>> ==
>>> Test summary
>>> ==
>>>TEST  TOTAL  PASS  FAIL 
>>> ERROR   
>>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>>> >>   1 0 1 
>>> >> 0 <<
>>> ==
>>> TEST FAILURE
>> 
>> 
>> 
>> after added the code block back 
>> 
>>if (argc != 2) {
>> shutItDown();
>> }
>> 
>> 
>> updated test passes
>> 
>> 
>> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
>>  \\
>> -Xmx768m \\
>> -XX:MaxRAMPercentage=1.04167 \\
>> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>> 
>> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
>>  \\
>> -ea \\
>> -esa \\
>> 
>> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
>>  \\
>> com.sun.javatest.regtest.agent.MainWrapper 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang...
>
> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 111:
> 
>> 109: if (p.exitValue() != 1)
>> 110: System.exit(ERROR + 2);
>> 111: System.exit(0);
> 
> Its bad form to System.exit from not at the top level. Is that necessary.

This is in line with other tests in this file , e.g. see `parentCode()`. 

I agree that code would be cleaner if the test is refactored to return error 
code from the test methods and system.exit() in the main instead, but this 
might be a separate pr done before or after this one.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1511953089

Re: jpackage requests permission via a dialog

[Alan Snyder]

Thank you for the explanation.

It was not obvious to me that the configuration of the DMG appearance actually 
means configuring the .DS_Store file in the DMG.

  Alan

> On Mar 4, 2024, at 1:27 PM, Alexander Matveev  
> wrote:
> 
> Hi Alan,
>  
> See below.
>  
> From: core-libs-dev  > on behalf of Alan Snyder 
> mailto:javali...@cbfiddle.com>>
> Date: Sunday, March 3, 2024 at 7:09 PM
> To: core-libs-dev@openjdk.org  
> mailto:core-libs-dev@openjdk.org>>
> Subject: jpackage requests permission via a dialog
> 
> I tried using jpackage on macOS to create a DMG (which I have not done 
> before) and was surprised when a system dialog was displayed requesting 
> permission for Terminal to control Finder.
> 
> I found this issue described in JDK-8231855, which was closed without 
> explanation as “not an issue”.
> 
> From JDK-8231855: “Looks like this dialog is part of macOS Catalina Data 
> Protection features. I did not found if it possible not to trigger this 
> prompt. Also, this prompt is shown only once and subsequent jpackage runs 
> will not show any prompts. As workaround in case when jpackage will be part 
> of automated build system which needs to be run without user intervention it 
> is recommended to run jpackage first on this system and confirm any prompts.”.
> 
> It seems to me that having a command line build tool try to interact with a 
> user via a dialog is inappropriate, even if it happens only once per system.
> 
> So far, I did not figure out other solution for this problem.
> 
> I’m not sure why this dialog is shown, as it appears that all of the actions 
> are performed by running the hdiutil program.
> 
> This dialog is shown when we running osascript 
> (https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/DMGsetup.scpt)
>  to post process DMG file. This script will setup DMG background and 
> re-arrange icons, so it look nice.
> 
> It does seem odd, however, that jpackage mounts the DMG to modify its 
> contents. Shouldn’t the contents be set up before creating the DMG?
> 
> We need to mount it to run osascript I mentioned above.
> 
> Is mounting the image the source of the dialog?
> 
> No, dialog is shown after we mount it from osascript which is run right after 
> DMG is mounted.
> 
> Is there some reason this cannot be fixed?
> 
> So far I did not found any substitution to our osascript to setup background 
> image and re-arrange icons. I do not think that Apple provides any other ways 
> to do it.
> 
> Thanks,
> 
> Alexander
> 

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v4]

[Erik Joelsson]

> Executables and dynamic libraries on Linux can encode a search path that the 
> dynamic linker will use when looking up library dependencies, generally 
> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature to 
> set search paths relative to the location of the binary itself. Typically 
> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find 
> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to 
> find each other.
> 
> There are two different types of such rpaths, RPATH and RUNPATH. The former 
> is the earlier incantation but RUNPATH has been around since about 2003 and 
> has been default in prominent Linux distros for a long time, and now also 
> seems to be default in the linker directly from binutils. The toolchain used 
> by Oracle defaulted to RPATH until at least JDK 11, but since then with some 
> toolchain upgrade, the default was flipped to RUNPATH.
> 
> The main (relevant in this case) difference between the two is that RPATH is 
> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is 
> only considered after LD_LIBRARY_PATH. For libraries that are part of a Linux 
> distribution, letting users, or the system, control and override builtin 
> rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. However, 
> for the JDK, there really is no usecase for having an externally configured 
> LD_LIBRARY_PATH potentially getting in the way of the JDK libraries finding 
> each other correctly. If a user environment sets LD_LIBRARY_PATH, and there 
> is a library in that path with the same name as a JDK library (e.g. libnet.so 
> or some other generically named library) that external library will be loaded 
> instead of the JDK internal library and that is basically guaranteed to break 
> the JDK. There is no supported usecase that I can think of for injecting 
> other versions of such libraries in a JDK distribution.
> 
> I propose that we explicitly configure the JDK build to set RPATH instead of 
> RUNPATH for Linux binaries. This is done with the linker flag 
> "--disable-new-dtags".

Erik Joelsson has updated the pull request incrementally with one additional 
commit since the last revision:

  bug ref

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18050/files
  - new: https://git.openjdk.org/jdk/pull/18050/files/faafef8a..8639eee5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18050&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18050&range=02-03

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18050.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18050/head:pull/18050

PR: https://git.openjdk.org/jdk/pull/18050

Re: RFR: 8325567: jspawnhelper without args fails with segfault

[Roger Riggs]

On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan  wrote:

> This change is intended to address the segmentation fault issue that occurs 
> when jspawnhelper is called without arguments, and it includes an updated 
> test to verify the behavior in such cases.
> 
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without 
> 
>if (argc != 2) {
> shutItDown();
> }
> 
> code block
> 
>> Test results: failed: 1
>> Report written to 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test 
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in 
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>  
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >>   1 0 1 0 
>> >> <<
>> ==
>> TEST FAILURE
> 
> 
> 
> after added the code block back 
> 
>if (argc != 2) {
> shutItDown();
> }
> 
> 
> updated test passes
> 
> 
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
>  \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
> 
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
>  \\
> -ea \\
> -esa \\
> 
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
>  \\
> com.sun.javatest.regtest.agent.MainWrapper 
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...

Changes requested by rriggs (Reviewer).

Need more time to review.

test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 111:

> 109: if (p.exitValue() != 1)
> 110: System.exit(ERROR + 2);
> 111: System.exit(0);

Its bad form to System.exit from not at the top level. Is that necessary.

-

PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1915545846
PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1977643226
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1511921441

Re: jpackage requests permission via a dialog

[Michael Hall]

If I remember what I read correctly it was Apple’s intention that some services 
require explicit user authorization. 
So, the dialog. 
You could probably hdiutil yourself but the AppleScript does have benefits. 
Such as a background image, the drag to Application Folder option, layout 
appearance with multiple files.  I think there was some provision for 
customization as well. 
From what I remember. 

Re: RFR: 8325567: jspawnhelper without args fails with segfault

[Evgeny Astigeevich]

On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan  wrote:

> This change is intended to address the segmentation fault issue that occurs 
> when jspawnhelper is called without arguments, and it includes an updated 
> test to verify the behavior in such cases.
> 
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without 
> 
>if (argc != 2) {
> shutItDown();
> }
> 
> code block
> 
>> Test results: failed: 1
>> Report written to 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test 
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in 
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>  
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >>   1 0 1 0 
>> >> <<
>> ==
>> TEST FAILURE
> 
> 
> 
> after added the code block back 
> 
>if (argc != 2) {
> shutItDown();
> }
> 
> 
> updated test passes
> 
> 
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
>  \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
> 
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
>  \\
> -ea \\
> -esa \\
> 
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
>  \\
> com.sun.javatest.regtest.agent.MainWrapper 
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...

lgtm

-

Marked as reviewed by eastigeevich (Committer).

PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1915420153

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v4]

[Weijun Wang]

> This code change adds an alternative implementation of user-based 
> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
> Depending on if the Security Manager is allowed, the methods store the 
> current subject differently. See the spec change in the `Subject.java` file 
> for details. When the Security Manager APIs are finally removed in a future 
> release, this new implementation will be only implementation for these 
> methods.
> 
> One major change in the new implementation is that `Subject.getSubject` 
> always throws an `UnsupportedOperationException` since it has an 
> `AccessControlContext` argument but the current subject is no longer 
> associated with an `AccessControlContext` object.
> 
> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
> `current` and `callAs`. If the user application is simply calling 
> `getSubject(AccessController.getContext())`, then switching to `current()` 
> would work. If the `AccessControlContext` argument is retrieved from an 
> earlier `getContext()` call and the associated subject might be different 
> from that of the current `AccessControlContext`, then instead of storing the 
> previous `AccessControlContext` object and passing it into `getSubject` to 
> get the "previous" subject, the application should store the `current()` 
> return value directly.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  revert some test changes, spec change for subject

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17472/files
  - new: https://git.openjdk.org/jdk/pull/17472/files/8f270d09..e57f7250

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=17472&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17472&range=02-03

  Stats: 44 lines in 6 files changed: 18 ins; 3 del; 23 mod
  Patch: https://git.openjdk.org/jdk/pull/17472.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17472/head:pull/17472

PR: https://git.openjdk.org/jdk/pull/17472

Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string

[Raju Gupta]

On Sat, 2 Mar 2024 00:34:32 GMT, Justin Lu  wrote:

> Please review this PR and corresponding CSR which prevents an 
> OutOfMemoryError by restricting the initial maximum fraction digits for an 
> empty pattern DecimalFormat.
> 
> For an empty String pattern DecimalFormat, the maximum fraction digits is 
> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, 
> StringBuilder internally doubles capacity attempting to append 
> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral 
> compatibility changes.

Would declaring an object for the input and then invoking the two methods on 
the same object rather than creating two objects be considered an alternative?

-

Marked as reviewed by rajuc...@github.com (no known OpenJDK username).

PR Review: https://git.openjdk.org/jdk/pull/18094#pullrequestreview-1915406613

Re: jpackage requests permission via a dialog

[Alexander Matveev]

Hi Alan,

See below.

From: core-libs-dev  on behalf of Alan Snyder 

Date: Sunday, March 3, 2024 at 7:09 PM
To: core-libs-dev@openjdk.org 
Subject: jpackage requests permission via a dialog
I tried using jpackage on macOS to create a DMG (which I have not done before) 
and was surprised when a system dialog was displayed requesting permission for 
Terminal to control Finder.

I found this issue described in JDK-8231855, which was closed without 
explanation as “not an issue”.
From JDK-8231855: “Looks like this dialog is part of macOS Catalina Data 
Protection features. I did not found if it possible not to trigger this prompt. 
Also, this prompt is shown only once and subsequent jpackage runs will not show 
any prompts. As workaround in case when jpackage will be part of automated 
build system which needs to be run without user intervention it is recommended 
to run jpackage first on this system and confirm any prompts.”.

It seems to me that having a command line build tool try to interact with a 
user via a dialog is inappropriate, even if it happens only once per system.
So far, I did not figure out other solution for this problem.

I’m not sure why this dialog is shown, as it appears that all of the actions 
are performed by running the hdiutil program.
This dialog is shown when we running osascript 
(https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/DMGsetup.scpt)
 to post process DMG file. This script will setup DMG background and re-arrange 
icons, so it look nice.

It does seem odd, however, that jpackage mounts the DMG to modify its contents. 
Shouldn’t the contents be set up before creating the DMG?
We need to mount it to run osascript I mentioned above.

Is mounting the image the source of the dialog?
No, dialog is shown after we mount it from osascript which is run right after 
DMG is mounted.

Is there some reason this cannot be fixed?
So far I did not found any substitution to our osascript to setup background 
image and re-arrange icons. I do not think that Apple provides any other ways 
to do it.
Thanks,
Alexander

RFR: 8325567: jspawnhelper without args fails with segfault

[Elif Aslan]

This change is intended to address the segmentation fault issue that occurs 
when jspawnhelper is called without arguments, and it includes an updated test 
to verify the behavior in such cases.

Existing tests passes since it does not check behavior without args.
After test update the test fails without 

   if (argc != 2) {
shutItDown();
}

code block

> Test results: failed: 1
> Report written to 
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
> Results written to 
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
> Error: Some tests failed or other problems occurred.
> Finished running test 
> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
> Test report is stored in 
> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>  
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
> >>   1 0 1 0 
> >> <<
> ==
> TEST FAILURE



after added the code block back 

   if (argc != 2) {
shutItDown();
}


updated test passes


lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
 \\
-Xmx768m \\
-XX:MaxRAMPercentage=1.04167 \\
-Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\

-Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
 \\
-ea \\
-esa \\

-Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
 \\
com.sun.javatest.regtest.agent.MainWrapper 
/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperProtocol.d/main.0.jta
result: Passed. Execution successful
 
 
test result: Passed. Execution successful

-

Commit messages:
 - Add Robust arg check
 - Remove usage text and add test
 - 8325567:jspawnhelper without args fails with segfault

Changes: https://git.openjdk.org/jdk/pull/18112/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8325567
  Stats: 45 lines in 2 files changed: 43 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/18112.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112

PR: https://git.openjdk.org/jdk/pull/18112

Re: jpackage requests permission via a dialog

[Alexey Semenyuk]

On 3/3/2024 10:09 PM, Alan Snyder wrote:

I tried using jpackage on macOS to create a DMG (which I have not done before) 
and was surprised when a system dialog was displayed requesting permission for 
Terminal to control Finder.

I found this issue described in JDK-8231855, which was closed without 
explanation as “not an issue”.

There is a comment in the CR with the explanation and suggested workaround.


It seems to me that having a command line build tool try to interact with a 
user via a dialog is inappropriate, even if it happens only once per system.

I’m not sure why this dialog is shown, as it appears that all of the actions 
are performed by running the hdiutil program.

It does seem odd, however, that jpackage mounts the DMG to modify its contents. 
Shouldn’t the contents be set up before creating the DMG?

Is mounting the image the source of the dialog?

Is there some reason this cannot be fixed?

Alexander, could you please help with the above questions?

- Alexey

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Weijun Wang]

On Mon, 4 Mar 2024 16:17:14 GMT, Sean Mullan  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix MBeanServerFileAccessController, more test in SM
>
> src/java.base/share/classes/javax/security/auth/Subject.java line 120:
> 
>> 118:  * {@code getSubject(AccessControlContext)} method.
>> 119: *  If a security manager is not allowed, which means it
>> 120:  * {@linkplain System#setSecurityManager is not set and not allowed to 
>> be set
> 
> I think `SecurityManager.html#set-security-manager` is a better (more 
> informative) link. Also, not sure why it is linked here but not in the "If a 
> security manager is allowed" paragraph. If you link it in the first sentence 
> ("These methods behave differently ...) then you probably only need one link 
> and don't need this link.

OK.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511771517

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Weijun Wang]

On Mon, 4 Mar 2024 15:47:41 GMT, Sean Mullan  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix MBeanServerFileAccessController, more test in SM
>
> test/jdk/javax/security/auth/Subject/CallAsWithScopedValue.java line 27:
> 
>> 25:  * @test
>> 26:  * @bug 8296244
>> 27:  * @enablePreview
> 
> Can you add a comment as to why `enablePreview` is needed? (Assume it is for 
> `StructuredTaskScope`.)

OK.

> test/jdk/javax/security/auth/Subject/Compat.java line 34:
> 
>> 32: /*
>> 33:  * @test
>> 34:  * @run main/othervm -Djava.security.manager=allow Compat
> 
> Missing `@summary` and `@bug`.

Will Add.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511760021
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511761591

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Weijun Wang]

On Mon, 4 Mar 2024 15:15:54 GMT, Sean Mullan  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix MBeanServerFileAccessController, more test in SM
>
> test/jdk/javax/management/monitor/ThreadPoolAccTest.java line 69:
> 
>> 67: }
>> 68: private void setPrincipal() {
>> 69: Subject subject = Subject.current();
> 
> Why change this to `Subject.current()` if you are requiring the SM to be 
> allowed?

When SM is allowed, `Subject.current()` is equivalent to 
`Subject.getSubject(AccessController.getContext())`. I can revert the change.

> test/jdk/javax/security/auth/Subject/UnsupportedSV.java line 59:
> 
>> 57: 
>> 58: // TODO: Still has no way to reject the following code.
>> 59: // Here, AccessController::getContext returns a plan ACC without
> 
> typo: s/plan/plain/

Yes.

> test/jdk/javax/security/auth/Subject/doAs/NestedActions.java line 283:
> 
>> 281: static void readFile(String filename) {
>> 282: System.out.println("ReadFromFileAction: try to read " + 
>> filename);
>> 283: Subject subject = Subject.current();
> 
> Couldn't we have just left these calling `Subject.getSubject` for now since 
> these tests run with an SM enabled?

Yes, we can.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511755373
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511756265
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511755860

Re: RFR: 8327208: Remove unused method java.util.jar.Manifest.make72Safe

[Iris Clark]

On Mon, 4 Mar 2024 09:55:09 GMT, Eirik Bjørsnøs  wrote:

> Please review this cleanup PR which suggests we remove the package-private, 
> unused and deprecated method `java.util.jar.Manifest.make72Safe`.
> 
> This method was marked deprecated after a cleanup/refactoring in 
> [JDK-8066619](https://bugs.openjdk.org/browse/JDK-8066619) caused it to fall 
> out of use. It should rather be removed.
> 
> Some tests reference the `make72Safe` methods in comment, or implement the 
> legacy versions of the same logic to simulate pre JDK 11 line break behavior 
> of `Manifest.write`. These comments and method names are adjusted to not 
> reference the now removed method.
> 
> This change was initially discussed here: 
> https://mail.openjdk.org/pipermail/core-libs-dev/2024-February/119819.html

Marked as reviewed by iris (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18104#pullrequestreview-1915271908

Re: RFR: 8327208: Remove unused method java.util.jar.Manifest.make72Safe

[Lance Andersen]

On Mon, 4 Mar 2024 09:55:09 GMT, Eirik Bjørsnøs  wrote:

> Please review this cleanup PR which suggests we remove the package-private, 
> unused and deprecated method `java.util.jar.Manifest.make72Safe`.
> 
> This method was marked deprecated after a cleanup/refactoring in 
> [JDK-8066619](https://bugs.openjdk.org/browse/JDK-8066619) caused it to fall 
> out of use. It should rather be removed.
> 
> Some tests reference the `make72Safe` methods in comment, or implement the 
> legacy versions of the same logic to simulate pre JDK 11 line break behavior 
> of `Manifest.write`. These comments and method names are adjusted to not 
> reference the now removed method.
> 
> This change was initially discussed here: 
> https://mail.openjdk.org/pipermail/core-libs-dev/2024-February/119819.html

Marked as reviewed by lancea (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18104#pullrequestreview-1915256398

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Sean Mullan]

On Mon, 4 Mar 2024 19:51:38 GMT, Weijun Wang  wrote:

>> src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
>>  line 309:
>> 
>>> 307: final Subject s;
>>> 308: if (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) 
>>> {
>>> 309: s = Subject.current();
>> 
>> We may not want to call `Subject.current()` here, as this may imply that we 
>> will support this functionality even if an SM is not enabled.
>
> I was not exactly sure if we will support this functionality. The class name 
> has `AccessControler` and the method names use `checkAccess`, but they 
> actually do not always depend on security manager.

I think we need @kevinjwalls or @dfuch to help advise on this.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511721920

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Weijun Wang]

On Mon, 4 Mar 2024 15:28:28 GMT, Sean Mullan  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix MBeanServerFileAccessController, more test in SM
>
> src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
>  line 309:
> 
>> 307: final Subject s;
>> 308: if (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
>> 309: s = Subject.current();
> 
> We may not want to call `Subject.current()` here, as this may imply that we 
> will support this functionality even if an SM is not enabled.

I was not exactly sure if we will support this functionality. The class name 
has `AccessControler` and the method names use `checkAccess`, but they actually 
do not always depend on security manager.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511716084

Re: RFR: 8325567: jspawnhelper without args fails with segfault

[Vladimir Petko]

On Fri, 1 Mar 2024 07:37:22 GMT, Alan Bateman  wrote:

> Would it be possible to expand a bit on what is going on here? Are you saying 
> that in the course of upgrading a JDK that you somehow get into a state where 
> jspawnhelper has been replaced with a version from a newer JDK? Is the real 
> issue here that the upgrade didn't shutdown the application and/or something 
> copied over an existing installation during the upgrade?

It appears that answer is yes (in Debian and Ubuntu) if the machine is running 
unattended-upgrades service and openjdk is not blacklisted from upgrades. 
I believe we should blacklist openjdk packages by default to avoid this 
situation.

-

PR Comment: https://git.openjdk.org/jdk/pull/18074#issuecomment-1977331977

Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v6]

[Naoto Sato]

> This PR intends to remove the legacy `COMPAT` locale data from the JDK. The 
> `COMPAT` locale data was introduced for applications' migratory purposes 
> transitioning to `CLDR`. It is becoming a technical debt and now is the time 
> to remove it (we've been emitting a warning at JVM startup since JDK21, if 
> the app is using `COMPAT`). A corresponding CSR has also been drafted.

Naoto Sato 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 44 additional commits since the 
last revision:

 - Merge branch 'master' into JDK-8174269-COMPAT-Removal
 - Addressing review comments
 - Update test/jdk/java/text/Format/DateFormat/Bug6683975.java
   
   Co-authored-by: Justin Lu 
 - Remove `GensrcLocaleData.gmk`
 - Update make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java
   
   Co-authored-by: Andrey Turbanov 
 - cleanup
 - BreakIteratorProvider available locales fix
 - clean-up
 - test fixes
 - makeZoneData.pl fix
 - ... and 34 more: https://git.openjdk.org/jdk/compare/fe877cfd...b771e303

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17991/files
  - new: https://git.openjdk.org/jdk/pull/17991/files/d5953788..b771e303

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=17991&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17991&range=04-05

  Stats: 19239 lines in 1290 files changed: 10762 ins; 3853 del; 4624 mod
  Patch: https://git.openjdk.org/jdk/pull/17991.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17991/head:pull/17991

PR: https://git.openjdk.org/jdk/pull/17991

Integrated: 8309622: Re-examine the cache mechanism in BaseLocale

[Naoto Sato]

On Fri, 9 Jun 2023 22:17:39 GMT, Naoto Sato  wrote:

> This is stemming from the PR: https://github.com/openjdk/jdk/pull/14211 where 
> aggressive GC can cause NPE in `BaseLocale$Key` class. I refactored the 
> in-house cache with WeakHashMap, and removed the Key class as it is no longer 
> needed (thus the original NPE will no longer be possible). Also with the new 
> JMH test case, it gains some performance improvement:
> 
> (w/o fix)
> 
> Benchmark   Mode  Cnt  Score Error  Units
> LocaleCache.testForLanguageTag  avgt   20   5781.275 ± 569.580  ns/op
> LocaleCache.testLocaleOfavgt   20  62564.079 ± 406.697  ns/op
> 
> (w/ fix)
> Benchmark   Mode  Cnt  Score Error  Units
> LocaleCache.testForLanguageTag  avgt   20   4801.175 ± 371.830  ns/op
> LocaleCache.testLocaleOfavgt   20  60394.652 ± 352.471  ns/op

This pull request has now been integrated.

Changeset: f615ac4b
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk/commit/f615ac4bdf94750390d18aa954d41f72eb4ef4d2
Stats: 437 lines in 4 files changed: 43 ins; 267 del; 127 mod

8309622: Re-examine the cache mechanism in BaseLocale

Reviewed-by: dfuchs, rriggs

-

PR: https://git.openjdk.org/jdk/pull/14404

Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled

[ExE Boss]

On Mon, 4 Mar 2024 13:52:13 GMT, Jan Lahoda  wrote:

> Currently, JDK modules load by the bootstrap and platform ClassLoaders are 
> automatically granted the native access. I am working on an upgrade of JLine 
> inside the `jdk.internal.le` module, and I would like to replace the current 
> native bindings with FFM-based bindings (which are now somewhat provided by 
> JLine). But, for that, native access is needed for the `jdk.internal.le` 
> module. We could possibly move the module to the platform ClassLoader, but it 
> seems it might be better to have more control over which modules have the 
> native access.
> 
> This patch introduces an explicit list of modules that will automatically be 
> granted the native access. Note this patch is not yet intended to change the 
> end behavior - the list of modules granted native access is supposed to be 
> the same as modules in the boot and platform ClassLoaders.

src/java.base/share/classes/java/lang/ModuleLayer.java line 885:

> 883: 
> 884: /**
> 885:  * Returns the module with the given name in this later only.

Suggestion:

 * Returns the module with the given name in this layer only.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1511542904

Class loader leaked when ForkJoinPool is used in loaded class, with Java 19+

[S A]

Hi all,

after moving our application to Java 21 (up from Java 17), we noticed a
class loader leak. A memory snapshot points to a ProtectionDomain object
held by a ForkJoinWorkerThread, the protection domain holds the class
loader and prevents GC.

To reproduce outside of our application, we use this snippet:

import java.lang.ref.WeakReference;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Paths;
public class TestClassloaderLeak {
public static void main(String[] args) throws Exception {
WeakReference wr = load();
gc();
System.out.println("wr=" + wr.get());
}
private static void gc() {
System.gc();
System.runFinalization();
}
private static WeakReference load() throws Exception {
URLClassLoader cl = new URLClassLoader(new URL[] { url() },
TestClassloaderLeak.class.getClassLoader());
WeakReference wr = new WeakReference<>(cl);
Class ca = cl.loadClass("A");
ca.getConstructor(String.class).newInstance("A");
cl.close();
return wr;
}
private static URL url() throws MalformedURLException {
return Paths.get("/data/tmp/testleak/lib/").toUri().toURL();
}
}

import java.util.concurrent.ForkJoinPool;
import java.util.concurrent.ForkJoinTask;
public class A {
public final String s;
public A(String s) {
this.s = s;
ForkJoinTask task = ForkJoinPool.commonPool().submit(() -> {
System.out.println("A constructor"); });
try {
task.get();
} catch (Exception e) {
e.printStackTrace(System.out);
}
}
}

Place the compiled A.class at the hard-coded location
"/data/tmp/testleak/lib/", then run the main method with JVM flags
"-Xlog:class+unload". Observe that no class is unloaded, which is not the
case if the main() code runs twice, or if the snippet is executed e.g. with
Java 17.

It seems each time ForkJoinPool creates a new ForkJoinWorkerThread from a
loaded class, it is no longer possible to GC the class loader of the class
using ForkJoinPool.

The leak occurs after this commit, with Java 19+:

https://github.com/openjdk/jdk19u/commit/00e6c63cd12e3f92d0c1d007aab4f74915616ffb

Essentially, our application loads and runs user code. The user changes
their code, runs their code again - we use a new class loader to run the
changed code in the same JVM. We unload the previous class loader, to free
up memory and to avoid problems with hot swapping code (an old version of a
loaded class can cause an error when trying to hot swap the latest loaded
version of that class). So the leaked class loader is giving us trouble.

What possible workarounds can we use to avoid this problem?

Best regards and thanks,
Simeon

Re: RFR: 8327225: Revert DataInputStream.readUTF to static final

[Alan Bateman]

On Mon, 4 Mar 2024 13:55:15 GMT, Claes Redestad  wrote:

> [JDK-8325340](https://bugs.openjdk.org/browse/JDK-8325340) accidentally 
> removed `final` from the `static final DataInputStream.readUTF` method. This 
> has a minor compatibility impact (allows hiding the method in a subclass, 
> while before that would throw an exception at compile time) and since it was 
> not the intent of the prior change to alter any behavioral semantics here I 
> want to revert that change.

It's a bit unusual to have the final modifier on static methods, looks like it 
goes back to JDK 1.1 at least, not clear if it was intentional.

-

PR Comment: https://git.openjdk.org/jdk/pull/18107#issuecomment-1977103579

Re: RFR: 8327225: Revert DataInputStream.readUTF to static final

[Brian Burkhalter]

On Mon, 4 Mar 2024 13:55:15 GMT, Claes Redestad  wrote:

> [JDK-8325340](https://bugs.openjdk.org/browse/JDK-8325340) accidentally 
> removed `final` from the `static final DataInputStream.readUTF` method. This 
> has a minor compatibility impact (allows hiding the method in a subclass, 
> while before that would throw an exception at compile time) and since it was 
> not the intent of the prior change to alter any behavioral semantics here I 
> want to revert that change.

Marked as reviewed by bpb (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18107#pullrequestreview-191489

Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]

[Claudio Nieder]

On Tue, 27 Feb 2024 11:19:59 GMT, Magnus Ihse Bursie  wrote:

>> The idea of setting up general "toolchains" in the native build was good, 
>> but it turned out that we really only need a single toolchain, with a single 
>> twist: if it should use CC or CPP to link. This is better described by a 
>> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the 
>> default).
>> 
>> There is a single exception to this rule, and that is if we want to compile 
>> for the build platform rather than the target platform. (This is only done 
>> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD 
>> (or TARGET_TYPE := TARGET, the default).
>> 
>> The final odd-case was the hack for building hsdis/bin on mingw. This can be 
>> resolved using direct variables to SetupNativeCompilation, instead of first 
>> creating a toolchain.
>> 
>> Doing this refactoring will simplify the SetupNativeCompilation code, and 
>> make it much clearer if we use the C or C++ linker.
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into remove-toolchain-define
>  - Rename LANG to LINK_TYPE
>  - Reword "lib" comment to fit in better
>  - Merge branch 'master' into remove-toolchain-define
>  - 8326583: Remove over-generalized DefineNativeToolchain solution

One little side note, I tried now also on a Linux x86 system and saw the same 
issue, so it is not limited to macOS or aarch64.

-

PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976999445

Re: RFR: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used

[Christoph Langer]

On Fri, 23 Feb 2024 19:18:46 GMT, Christoph Langer  wrote:

> The new test JmodExcludedFiles.java checks that no debug symbol files are 
> contained in the jmod files. It does not take into account that with 
> configure option --with-external-symbols-in-bundles=public we want to have 
> stripped pdb files, also in jmods, to get native callstacks with line number.
> 
> This modifies the test and checks if equivalent *stripped.pdb files exist 
> when .pdb files are encountered inside jmods. In that case one can assume 
> that --with-external-symbols-in-bundles=public was chosen as configure option.

Thanks for the review, Matthias.

-

PR Comment: https://git.openjdk.org/jdk/pull/17990#issuecomment-1976989827

Integrated: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used

[Christoph Langer]

On Fri, 23 Feb 2024 19:18:46 GMT, Christoph Langer  wrote:

> The new test JmodExcludedFiles.java checks that no debug symbol files are 
> contained in the jmod files. It does not take into account that with 
> configure option --with-external-symbols-in-bundles=public we want to have 
> stripped pdb files, also in jmods, to get native callstacks with line number.
> 
> This modifies the test and checks if equivalent *stripped.pdb files exist 
> when .pdb files are encountered inside jmods. In that case one can assume 
> that --with-external-symbols-in-bundles=public was chosen as configure option.

This pull request has now been integrated.

Changeset: 43c6f0b5
Author:Christoph Langer 
URL:   
https://git.openjdk.org/jdk/commit/43c6f0b5880899b797fab2f851bd35be1c342439
Stats: 48 lines in 1 file changed: 30 ins; 5 del; 13 mod

8326591: New test JmodExcludedFiles.java fails on Windows when 
--with-external-symbols-in-bundles=public is used

Reviewed-by: mbaesken

-

PR: https://git.openjdk.org/jdk/pull/17990

Re: RFR: 8318650: Optimized subword gather for x86 targets. [v17]

[Paul Sandoz]

On Sat, 2 Mar 2024 16:22:22 GMT, Jatin Bhateja  wrote:

>> Hi All,
>> 
>> This patch optimizes sub-word gather operation for x86 targets with AVX2 and 
>> AVX512 features.
>> 
>> Following is the summary of changes:-
>> 
>> 1) Intrinsify sub-word gather using hybrid algorithm which initially 
>> partially unrolls scalar loop to accumulates values from gather indices into 
>> a quadword(64bit) slice followed by vector permutation to place the slice 
>> into appropriate vector lanes, it prevents code bloating and generates 
>> compact JIT sequence. This coupled with savings from expansive array 
>> allocation in existing java implementation translates into significant 
>> performance of 1.5-10x gains with included micro.
>> 
>> ![image](https://github.com/openjdk/jdk/assets/59989778/e25ba4ad-6a61-42fa-9566-452f741a9c6d)
>> 
>> 
>> 2) Patch was also compared against modified java fallback implementation by 
>> replacing temporary array allocation with zero initialized vector and a 
>> scalar loops which inserts gathered values into vector. But, vector insert 
>> operation in higher vector lanes is a three step process which first 
>> extracts the upper vector 128 bit lane, updates it with gather subword value 
>> and then inserts the lane back to its original position. This makes inserts 
>> into higher order lanes costly w.r.t to proposed solution. In addition 
>> generated JIT code for modified fallback implementation was very bulky. This 
>> may impact in-lining decisions into caller contexts.
>> 
>> Kindly review and share your feedback.
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review resolutions.

Marked as reviewed by psandoz (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/16354#pullrequestreview-1914752388

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Sean Mullan]

On Tue, 30 Jan 2024 21:58:28 GMT, Weijun Wang  wrote:

>> This code change adds an alternative implementation of user-based 
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
>> Depending on if the Security Manager is allowed, the methods store the 
>> current subject differently. See the spec change in the `Subject.java` file 
>> for details. When the Security Manager APIs are finally removed in a future 
>> release, this new implementation will be only implementation for these 
>> methods.
>> 
>> One major change in the new implementation is that `Subject.getSubject` 
>> always throws an `UnsupportedOperationException` since it has an 
>> `AccessControlContext` argument but the current subject is no longer 
>> associated with an `AccessControlContext` object.
>> 
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
>> `current` and `callAs`. If the user application is simply calling 
>> `getSubject(AccessController.getContext())`, then switching to `current()` 
>> would work. If the `AccessControlContext` argument is retrieved from an 
>> earlier `getContext()` call and the associated subject might be different 
>> from that of the current `AccessControlContext`, then instead of storing the 
>> previous `AccessControlContext` object and passing it into `getSubject` to 
>> get the "previous" subject, the application should store the `current()` 
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix MBeanServerFileAccessController, more test in SM

src/java.base/share/classes/javax/security/auth/Subject.java line 112:

> 110:  *
> 111:  * These methods behave differently depending on
> 112:  * whether a security manager is allowed or disallowed:

Suggest linking "a security manager is allowed or disallowed" to 
`SecurityManager.html#set-security-manager`.

src/java.base/share/classes/javax/security/auth/Subject.java line 120:

> 118:  * {@code getSubject(AccessControlContext)} method.
> 119: *  If a security manager is not allowed, which means it
> 120:  * {@linkplain System#setSecurityManager is not set and not allowed to 
> be set

I think `SecurityManager.html#set-security-manager` is a better (more 
informative) link. Also, not sure why it is linked here but not in the "If a 
security manager is allowed" paragraph. If you link it in the first sentence 
("These methods behave differently ...) then you probably only need one link 
and don't need this link.

test/jdk/javax/security/auth/Subject/CallAsWithScopedValue.java line 27:

> 25:  * @test
> 26:  * @bug 8296244
> 27:  * @enablePreview

Can you add a comment as to why `enablePreview` is needed? (Assume it is for 
`StructuredTaskScope`.)

test/jdk/javax/security/auth/Subject/Compat.java line 34:

> 32: /*
> 33:  * @test
> 34:  * @run main/othervm -Djava.security.manager=allow Compat

Missing `@summary` and `@bug`.

test/jdk/javax/security/auth/Subject/UnsupportedSV.java line 59:

> 57: 
> 58: // TODO: Still has no way to reject the following code.
> 59: // Here, AccessController::getContext returns a plan ACC without

typo: s/plan/plain/

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511419716
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511424024
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511380094
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511393254
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511366395

Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string

[Roger Riggs]

On Sat, 2 Mar 2024 00:34:32 GMT, Justin Lu  wrote:

> Please review this PR and corresponding CSR which prevents an 
> OutOfMemoryError by restricting the initial maximum fraction digits for an 
> empty pattern DecimalFormat.
> 
> For an empty String pattern DecimalFormat, the maximum fraction digits is 
> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, 
> StringBuilder internally doubles capacity attempting to append 
> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral 
> compatibility changes.

src/java.base/share/classes/java/text/DecimalFormat.java line 3717:

> 3715: // As maxFracDigits are fully displayed unlike maxIntDigits
> 3716: // Prevent OOME by setting to a much more reasonable value.
> 3717: setMaximumFractionDigits(DOUBLE_FRACTION_DIGITS);

Setting a reasonable default makes sense.  
In other control paths, the max fraction digits come from the inputs or are 
explicitly set.

It might be a reasonable related change to use StringBuilder.repeat() instead 
of a loop at LIne 3312-3319, where the pattern char(s) are being appended to 
the result.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1511404338

Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v12]

[Adam Sotona]

> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes.
> 
> This patch converts it to use Classfile API.
> 
> It is continuation of https://github.com/openjdk/jdk/pull/10991
> 
> Any comments and suggestions are welcome.
> 
> Please review.
> 
> Thank you,
> Adam

Adam Sotona has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 21 commits:

 - Merge branch 'master' into JDK-8294961-proxy
 - Update src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
   
   Co-authored-by: Mandy Chung 
 - initialization of template entries by index
 - Apply suggestions from code review
   
   Co-authored-by: Mandy Chung 
 - applied suggested changes
 - Revert "StackCounter performance boost"
   
   This reverts commit 0dc63d4edf40fd9458fbfa0c7661d57ed0022981.
 - Revert "SplitConstantPool performance fix"
   
   This reverts commit b7a60ae944983224e3b4c097576c496351394fe0.
 - Revert "applied the recommended changes"
   
   This reverts commit 7d0da2c0190c27f8e2cf89557e31f5d16ab4950e.
 - Revert "minor StackCounter fix"
   
   This reverts commit 41e879348c8f2ea70b25119e65527b81281c33ac.
 - Revert "Update 
src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java"
   
   This reverts commit c8f1d304358e19872450cd29449d82675f9bbe3e.
 - ... and 11 more: https://git.openjdk.org/jdk/compare/0583f735...40f99d1c

-

Changes: https://git.openjdk.org/jdk/pull/17121/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17121&range=11
  Stats: 543 lines in 2 files changed: 129 ins; 189 del; 225 mod
  Patch: https://git.openjdk.org/jdk/pull/17121.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121

PR: https://git.openjdk.org/jdk/pull/17121

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Sean Mullan]

On Tue, 30 Jan 2024 21:58:28 GMT, Weijun Wang  wrote:

>> This code change adds an alternative implementation of user-based 
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
>> Depending on if the Security Manager is allowed, the methods store the 
>> current subject differently. See the spec change in the `Subject.java` file 
>> for details. When the Security Manager APIs are finally removed in a future 
>> release, this new implementation will be only implementation for these 
>> methods.
>> 
>> One major change in the new implementation is that `Subject.getSubject` 
>> always throws an `UnsupportedOperationException` since it has an 
>> `AccessControlContext` argument but the current subject is no longer 
>> associated with an `AccessControlContext` object.
>> 
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
>> `current` and `callAs`. If the user application is simply calling 
>> `getSubject(AccessController.getContext())`, then switching to `current()` 
>> would work. If the `AccessControlContext` argument is retrieved from an 
>> earlier `getContext()` call and the associated subject might be different 
>> from that of the current `AccessControlContext`, then instead of storing the 
>> previous `AccessControlContext` object and passing it into `getSubject` to 
>> get the "previous" subject, the application should store the `current()` 
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix MBeanServerFileAccessController, more test in SM

test/jdk/javax/security/auth/Subject/doAs/NestedActions.java line 283:

> 281: static void readFile(String filename) {
> 282: System.out.println("ReadFromFileAction: try to read " + 
> filename);
> 283: Subject subject = Subject.current();

Couldn't we have just left these calling `Subject.getSubject` for now since 
these tests run with an SM enabled?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511358336

Re: RFR: 8327225: Revert DataInputStream.readUTF to static final

[Roger Riggs]

On Mon, 4 Mar 2024 13:55:15 GMT, Claes Redestad  wrote:

> [JDK-8325340](https://bugs.openjdk.org/browse/JDK-8325340) accidentally 
> removed `final` from the `static final DataInputStream.readUTF` method. This 
> has a minor compatibility impact (allows hiding the method in a subclass, 
> while before that would throw an exception at compile time) and since it was 
> not the intent of the prior change to alter any behavioral semantics here I 
> want to revert that change.

Good catch; it would have been caught by the signature tests but later.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18107#pullrequestreview-1914625661

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Sean Mullan]

On Tue, 30 Jan 2024 21:58:28 GMT, Weijun Wang  wrote:

>> This code change adds an alternative implementation of user-based 
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
>> Depending on if the Security Manager is allowed, the methods store the 
>> current subject differently. See the spec change in the `Subject.java` file 
>> for details. When the Security Manager APIs are finally removed in a future 
>> release, this new implementation will be only implementation for these 
>> methods.
>> 
>> One major change in the new implementation is that `Subject.getSubject` 
>> always throws an `UnsupportedOperationException` since it has an 
>> `AccessControlContext` argument but the current subject is no longer 
>> associated with an `AccessControlContext` object.
>> 
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
>> `current` and `callAs`. If the user application is simply calling 
>> `getSubject(AccessController.getContext())`, then switching to `current()` 
>> would work. If the `AccessControlContext` argument is retrieved from an 
>> earlier `getContext()` call and the associated subject might be different 
>> from that of the current `AccessControlContext`, then instead of storing the 
>> previous `AccessControlContext` object and passing it into `getSubject` to 
>> get the "previous" subject, the application should store the `current()` 
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix MBeanServerFileAccessController, more test in SM

src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
 line 309:

> 307: final Subject s;
> 308: if (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
> 309: s = Subject.current();

We may not want to call `Subject.current()` here, as this may imply that we 
will support this functionality even if an SM is not enabled.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511351340

Integrated: 8323183: ClassFile API performance improvements

[Adam Sotona]

On Mon, 8 Jan 2024 13:38:16 GMT, Adam Sotona  wrote:

> ClassFile API performance related improvements have been separated from 
> #17121 into this PR.
> 
> These improvements are important to minimize performance regression of
> 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the 
> Classfile API to generate proxy classes #17121
> 
> Please review.
> 
> Thanks,
> Adam

This pull request has now been integrated.

Changeset: 0583f735
Author:Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/0583f7357480c0500daa82f490b2fcc05f2fb65a
Stats: 304 lines in 5 files changed: 147 ins; 139 del; 18 mod

8323183: ClassFile API performance improvements

Reviewed-by: redestad

-

PR: https://git.openjdk.org/jdk/pull/17306

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Sean Mullan]

On Tue, 30 Jan 2024 21:58:28 GMT, Weijun Wang  wrote:

>> This code change adds an alternative implementation of user-based 
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
>> Depending on if the Security Manager is allowed, the methods store the 
>> current subject differently. See the spec change in the `Subject.java` file 
>> for details. When the Security Manager APIs are finally removed in a future 
>> release, this new implementation will be only implementation for these 
>> methods.
>> 
>> One major change in the new implementation is that `Subject.getSubject` 
>> always throws an `UnsupportedOperationException` since it has an 
>> `AccessControlContext` argument but the current subject is no longer 
>> associated with an `AccessControlContext` object.
>> 
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
>> `current` and `callAs`. If the user application is simply calling 
>> `getSubject(AccessController.getContext())`, then switching to `current()` 
>> would work. If the `AccessControlContext` argument is retrieved from an 
>> earlier `getContext()` call and the associated subject might be different 
>> from that of the current `AccessControlContext`, then instead of storing the 
>> previous `AccessControlContext` object and passing it into `getSubject` to 
>> get the "previous" subject, the application should store the `current()` 
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix MBeanServerFileAccessController, more test in SM

test/jdk/javax/management/monitor/ThreadPoolAccTest.java line 69:

> 67: }
> 68: private void setPrincipal() {
> 69: Subject subject = Subject.current();

Why change this to `Subject.current()` if you are requiring the SM to be 
allowed?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511332276

Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]

[Aleksei Efimov]

On Fri, 23 Feb 2024 15:30:09 GMT, Aleksei Efimov  wrote:

> Thanks for exploring the possibility of improving tracebility of test 
> invocations to reported bugs.
> 
> >

I've given this test change a second thought, maybe you can try to separate the 
test into two separate test classes? One possibility to avoid duplicating code 
and have a separate test for each bug is to introduce a base test class that 
will contain the common functionality for 
[JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) and 
[JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) tests.

-

PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1976785208

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v2]

[Erik Joelsson]

On Mon, 4 Mar 2024 14:53:21 GMT, Magnus Ihse Bursie  wrote:

>> make/autoconf/flags-cflags.m4 line 40:
>> 
>>> 38: # Default works for linux, might work on other platforms as well.
>>> 39: SHARED_LIBRARY_FLAGS='-shared'
>>> 40: SET_EXECUTABLE_ORIGIN='-Wl,-rpath,\$$ORIGIN[$]1 
>>> -Wl,--disable-new-dtags'
>> 
>> The reason we use `--disable-new-dtags` needs to be documented somewhere.
>
> I agree. A short description and a reference to this bug number would be nice.

Added comment.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18050#discussion_r1511291650

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v2]

[Magnus Ihse Bursie]

On Sun, 3 Mar 2024 22:12:19 GMT, David Holmes  wrote:

>> Erik Joelsson has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Use @requires in test
>
> make/autoconf/flags-cflags.m4 line 40:
> 
>> 38: # Default works for linux, might work on other platforms as well.
>> 39: SHARED_LIBRARY_FLAGS='-shared'
>> 40: SET_EXECUTABLE_ORIGIN='-Wl,-rpath,\$$ORIGIN[$]1 
>> -Wl,--disable-new-dtags'
> 
> The reason we use `--disable-new-dtags` needs to be documented somewhere.

I agree. A short description and a reference to this bug number would be nice.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18050#discussion_r1511290973

Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]

[Magnus Ihse Bursie]

On Mon, 4 Mar 2024 11:45:59 GMT, Julian Waters  wrote:

>  I do hope that dropping ccache support isn't something that's planned though 
> :(

Not really. The benefit of dropping it is quite small, and there might be use 
cases where it helps.

But I think we should perhaps be more explicit in the documentation that it is 
not obvious that it helps performance, and encourage testing before enabling 
it. And perhaps even print a warning in configure if you enable it that you 
need to check that it helps.

-

PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976777022

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v3]

[Magnus Ihse Bursie]

On Mon, 4 Mar 2024 14:57:44 GMT, Erik Joelsson  wrote:

>> Executables and dynamic libraries on Linux can encode a search path that the 
>> dynamic linker will use when looking up library dependencies, generally 
>> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature 
>> to set search paths relative to the location of the binary itself. Typically 
>> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find 
>> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to 
>> find each other.
>> 
>> There are two different types of such rpaths, RPATH and RUNPATH. The former 
>> is the earlier incantation but RUNPATH has been around since about 2003 and 
>> has been default in prominent Linux distros for a long time, and now also 
>> seems to be default in the linker directly from binutils. The toolchain used 
>> by Oracle defaulted to RPATH until at least JDK 11, but since then with some 
>> toolchain upgrade, the default was flipped to RUNPATH.
>> 
>> The main (relevant in this case) difference between the two is that RPATH is 
>> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is 
>> only considered after LD_LIBRARY_PATH. For libraries that are part of a 
>> Linux distribution, letting users, or the system, control and override 
>> builtin rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. 
>> However, for the JDK, there really is no usecase for having an externally 
>> configured LD_LIBRARY_PATH potentially getting in the way of the JDK 
>> libraries finding each other correctly. If a user environment sets 
>> LD_LIBRARY_PATH, and there is a library in that path with the same name as a 
>> JDK library (e.g. libnet.so or some other generically named library) that 
>> external library will be loaded instead of the JDK internal library and that 
>> is basically guaranteed to break the JDK. There is no supported usecase that 
>> I can think of for injecting other versions of such libraries in a JDK 
>> distribution.
>> 
>> I propose that we explicitly configure the JDK build to set RPATH instead of 
>> RUNPATH for Linux binaries. This is done with the linker flag 
>> "--disable-new-dtags".
>
> Erik Joelsson has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add comment

Marked as reviewed by ihse (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18050#pullrequestreview-1914533756

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v3]

[Erik Joelsson]

> Executables and dynamic libraries on Linux can encode a search path that the 
> dynamic linker will use when looking up library dependencies, generally 
> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature to 
> set search paths relative to the location of the binary itself. Typically 
> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find 
> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to 
> find each other.
> 
> There are two different types of such rpaths, RPATH and RUNPATH. The former 
> is the earlier incantation but RUNPATH has been around since about 2003 and 
> has been default in prominent Linux distros for a long time, and now also 
> seems to be default in the linker directly from binutils. The toolchain used 
> by Oracle defaulted to RPATH until at least JDK 11, but since then with some 
> toolchain upgrade, the default was flipped to RUNPATH.
> 
> The main (relevant in this case) difference between the two is that RPATH is 
> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is 
> only considered after LD_LIBRARY_PATH. For libraries that are part of a Linux 
> distribution, letting users, or the system, control and override builtin 
> rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. However, 
> for the JDK, there really is no usecase for having an externally configured 
> LD_LIBRARY_PATH potentially getting in the way of the JDK libraries finding 
> each other correctly. If a user environment sets LD_LIBRARY_PATH, and there 
> is a library in that path with the same name as a JDK library (e.g. libnet.so 
> or some other generically named library) that external library will be loaded 
> instead of the JDK internal library and that is basically guaranteed to break 
> the JDK. There is no supported usecase that I can think of for injecting 
> other versions of such libraries in a JDK distribution.
> 
> I propose that we explicitly configure the JDK build to set RPATH instead of 
> RUNPATH for Linux binaries. This is done with the linker flag 
> "--disable-new-dtags".

Erik Joelsson has updated the pull request incrementally with one additional 
commit since the last revision:

  Add comment

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18050/files
  - new: https://git.openjdk.org/jdk/pull/18050/files/592281f2..faafef8a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18050&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18050&range=01-02

  Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18050.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18050/head:pull/18050

PR: https://git.openjdk.org/jdk/pull/18050

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v2]

[Erik Joelsson]

On Sun, 3 Mar 2024 22:09:51 GMT, David Holmes  wrote:

> I find it somewhat odd that we seem to be add odds with the general 
> programming community when it comes to this behaviour. Giving precedence to 
> `LD_LIBRARY_PATH` is the new behaviour, enabled via `--enable-new-dtags`; and 
> at some point this was made the default behaviour so it must be considered 
> generally desirable. But we have now decided we do not want this behaviour so 
> we have to explicitly disable it via `--disable-new-dtags`.

Your point here is the reason it took me this long to come to the realization 
that my proposal here is correct for a product like the JDK. I have been 
dealing with these rpaths several time over the years and have been confused 
over the significance of the two kinds, but figured that since RUNPATH is the 
new variant and the general move is towards that, we should just stick to that 
too. However, as I allude to in the bug description, we aren't really fitting 
the usecases the default is aiming to solve.

If you are building a single library for distribution, or to install in your 
local Linux distribution, then letting the user have the option to customize 
the dynamic loader with LD_LIBRARY_PATH by default seems reasonable. I can see 
why Linux distributions prefer having LD_LIBRARY_PATH available to override 
rpaths as well. They want to promote applications that are well integrated with 
the system and using the system versions of all dependency libraries instead of 
bundling their own competing versions. 

However, what we are building is more of a self contained application. When 
users jlink an application, it becomes even more obvious. RPATH allows us to 
completely self contain the dependencies between our _internal_ native 
libraries. Any external dependency that the JDK has (glibc etc) we still link 
to from the system the same way as before. Looking for discussions about RPATH 
and RUNPATH online, this is basically the conclusion I find.

A better solution would perhaps have been to name our internal libraries 
better, using a namespace that would make name clashes with other external 
libraries less likely (e.g. libjavanet.so instead of libnet.so). If we had done 
that, then this issue would probably not have been worth raising.

-

PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-1976750074

Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled

[Erik Joelsson]

On Mon, 4 Mar 2024 13:52:13 GMT, Jan Lahoda  wrote:

> Currently, JDK modules load by the bootstrap and platform ClassLoaders are 
> automatically granted the native access. I am working on an upgrade of JLine 
> inside the `jdk.internal.le` module, and I would like to replace the current 
> native bindings with FFM-based bindings (which are now somewhat provided by 
> JLine). But, for that, native access is needed for the `jdk.internal.le` 
> module. We could possibly move the module to the platform ClassLoader, but it 
> seems it might be better to have more control over which modules have the 
> native access.
> 
> This patch introduces an explicit list of modules that will automatically be 
> granted the native access. Note this patch is not yet intended to change the 
> end behavior - the list of modules granted native access is supposed to be 
> the same as modules in the boot and platform ClassLoaders.

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1914434715

Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled

[Maurizio Cimadamore]

On Mon, 4 Mar 2024 13:52:13 GMT, Jan Lahoda  wrote:

> Currently, JDK modules load by the bootstrap and platform ClassLoaders are 
> automatically granted the native access. I am working on an upgrade of JLine 
> inside the `jdk.internal.le` module, and I would like to replace the current 
> native bindings with FFM-based bindings (which are now somewhat provided by 
> JLine). But, for that, native access is needed for the `jdk.internal.le` 
> module. We could possibly move the module to the platform ClassLoader, but it 
> seems it might be better to have more control over which modules have the 
> native access.
> 
> This patch introduces an explicit list of modules that will automatically be 
> granted the native access. Note this patch is not yet intended to change the 
> end behavior - the list of modules granted native access is supposed to be 
> the same as modules in the boot and platform ClassLoaders.

Looks good.

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 273:

> 271: 
> 272: /**
> 273:  * Updates module named name in layer layer to allow access to 
> restricted methods.

Suggestion:

 * Updates module named {@code name} in layer {@code layer} to allow access 
to restricted methods.

src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 806:

> 804: /**
> 805:  * Process the --enable-native-access option to grant access to 
> restricted methods to selected modules.
> 806:  * Also add Enable native access from JDK modules.

I don't think this extra comment is needed.

-

Marked as reviewed by mcimadamore (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1914419463
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1511226929
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1511223692

RFR: 8327225: Revert DataInputStream.readUTF to static final

[Claes Redestad]

[JDK-8325340](https://bugs.openjdk.org/browse/JDK-8325340) accidentally removed 
`final` from the `static final DataInputStream.readUTF` method. This has a 
minor compatibility impact (allows hiding the method in a subclass, while 
before that would throw an exception at compile time) and since it was not the 
intent of the prior change to alter any behavioral semantics here I want to 
revert that change.

-

Commit messages:
 - 8327225: Revert DataInputStream.readUTF to static final

Changes: https://git.openjdk.org/jdk/pull/18107/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18107&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8327225
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18107.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18107/head:pull/18107

PR: https://git.openjdk.org/jdk/pull/18107

RFR: 8327218: Add an ability to specify modules which should have native access enabled

[Jan Lahoda]

Currently, JDK modules load by the bootstrap and platform ClassLoaders are 
automatically granted the native access. I am working on an upgrade of JLine 
inside the `jdk.internal.le` module, and I would like to replace the current 
native bindings with FFM-based bindings (which are now somewhat provided by 
JLine). But, for that, native access is needed for the `jdk.internal.le` 
module. We could possibly move the module to the platform ClassLoader, but it 
seems it might be better to have more control over which modules have the 
native access.

This patch introduces an explicit list of modules that will automatically be 
granted the native access. Note this patch is not yet intended to change the 
end behavior - the list of modules granted native access is supposed to be the 
same as modules in the boot and platform ClassLoaders.

-

Commit messages:
 - Explicitly listing the modules that should get native access.
 - Native access modules-1

Changes: https://git.openjdk.org/jdk/pull/18106/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18106&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8327218
  Stats: 120 lines in 9 files changed: 103 ins; 9 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/18106.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18106/head:pull/18106

PR: https://git.openjdk.org/jdk/pull/18106

Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]

[Julian Waters]

On Mon, 4 Mar 2024 12:58:28 GMT, Claudio Nieder  wrote:

> > Could I trouble you to mention what exactly was different?
> 
> No trouble at all.
> 
> `CCACHE_BASEDIR=/tmp/bceaed6d/jdk/`is completely missing. (The directory is 
> where I checked out OpenJDK)
> 
> `CCACHE_SLOPPINESS` has the value `pch_defines,time_macros` with the working 
> parent commit but just `pch_defines` with this commit. I.e. 
> `CCACHE_SLOPPINESS=pch_defines,time_macros` vs 
> `CCACHE_SLOPPINESS=pch_defines`.

Thanks! Seems curious to me, off the top of my head I can't seem to discern why 
these would change, perhaps it's time for a little investigating

-

PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976534197

Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]

[Claudio Nieder]

On Mon, 4 Mar 2024 11:45:59 GMT, Julian Waters  wrote:

> Could I trouble you to mention what exactly was different?

No trouble at all. 

`CCACHE_BASEDIR=/tmp/bceaed6d/jdk/`is completely missing. (The directory is 
where I checked out OpenJDK)

`CCACHE_SLOPPINESS` has the value `pch_defines,time_macros` with the working 
parent commit but just `pch_defines` with this commit. I.e. 
`CCACHE_SLOPPINESS=pch_defines,time_macros` vs `CCACHE_SLOPPINESS=pch_defines`.

-

PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976528283

Re: RFR: 8323183: ClassFile API performance improvements [v8]

[Claes Redestad]

On Mon, 4 Mar 2024 12:00:18 GMT, Adam Sotona  wrote:

>> ClassFile API performance related improvements have been separated from 
>> #17121 into this PR.
>> 
>> These improvements are important to minimize performance regression of
>> 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the 
>> Classfile API to generate proxy classes #17121
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona 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 24 additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'openjdk/master' into 
> JDK-8323184-performance-fixes
>  - applied suggested changes
>  - Update 
> src/java.base/share/classes/jdk/internal/classfile/impl/StackMapDecoder.java
>
>Co-authored-by: Andrey Turbanov 
>  - updated copyright year
>  - reverted custom method slots counting in StackCounter
>  - improved and extended GenerateStackMaps benchmarks and renamed to 
> CodeAttributeTools
>  - Update 
> src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java
>
>Co-authored-by: liach <7806504+li...@users.noreply.github.com>
>  - applied suggested changes
>  - applied suggested changes
>  - improved StackMapDescoder::initFrameLocals performance
>  - ... and 14 more: https://git.openjdk.org/jdk/compare/d128c9e7...747fce8e

Marked as reviewed by redestad (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17306#pullrequestreview-1914189294

Re: RFR: 8323183: ClassFile API performance improvements [v7]

[Adam Sotona]

On Mon, 4 Mar 2024 10:08:27 GMT, Claes Redestad  wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Update 
>> src/java.base/share/classes/jdk/internal/classfile/impl/StackMapDecoder.java
>>   
>>   Co-authored-by: Andrey Turbanov 
>
> src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java 
> line 36:
> 
>> 34: import java.lang.classfile.constantpool.MemberRefEntry;
>> 35: import static java.lang.classfile.ClassFile.*;
>> 36: import java.lang.constant.MethodTypeDesc;
> 
> Imports are strangely out-of-order and imports in the classfile package look 
> haphazard. Is there some system to it that I don't see? I don't know if we 
> have an applicable style guide to lean on, but alphabetically sorted and 
> static imports split out at the end seem like the convention in the OpenJDK 
> sources.

Fixed, thanks.

> test/micro/org/openjdk/bench/jdk/classfile/CodeAttributeTools.java line 108:
> 
>> 106: @Benchmark
>> 107: public void benchmarkStackMapsGenerator() {
>> 108: for (var d : data) new StackMapGenerator(
> 
> When we return something from a benchmark JMH makes sure to sink that 
> something into a blackhole to avoid JIT assuming the result is unused and 
> eliminating it, wholly or in parts. When it's impractical to return a single 
> thing that captures all the computation in the benchmark it's good practice 
> to let a `org.openjdk.jmh.infra.Blackhole` consume effects to attain the same 
> effect:
> 
> public void benchmarkStackMapsGenerator(Blackhole bh) {
> for (var d : data) bh.consume(new StackMapGenerator(

Fixed, thanks.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1511040395
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1511042556

Re: RFR: 8323183: ClassFile API performance improvements [v8]

[Adam Sotona]

> ClassFile API performance related improvements have been separated from 
> #17121 into this PR.
> 
> These improvements are important to minimize performance regression of
> 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the 
> Classfile API to generate proxy classes #17121
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona 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 24 additional commits since the 
last revision:

 - Merge remote-tracking branch 'openjdk/master' into 
JDK-8323184-performance-fixes
 - applied suggested changes
 - Update 
src/java.base/share/classes/jdk/internal/classfile/impl/StackMapDecoder.java
   
   Co-authored-by: Andrey Turbanov 
 - updated copyright year
 - reverted custom method slots counting in StackCounter
 - improved and extended GenerateStackMaps benchmarks and renamed to 
CodeAttributeTools
 - Update 
src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java
   
   Co-authored-by: liach <7806504+li...@users.noreply.github.com>
 - applied suggested changes
 - applied suggested changes
 - improved StackMapDescoder::initFrameLocals performance
 - ... and 14 more: https://git.openjdk.org/jdk/compare/45f0a396...747fce8e

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17306/files
  - new: https://git.openjdk.org/jdk/pull/17306/files/5a04ec59..747fce8e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=17306&range=07
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17306&range=06-07

  Stats: 122960 lines in 3981 files changed: 64082 ins; 30411 del; 28467 mod
  Patch: https://git.openjdk.org/jdk/pull/17306.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306

PR: https://git.openjdk.org/jdk/pull/17306

Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]

[Julian Waters]

On Mon, 4 Mar 2024 11:28:02 GMT, Magnus Ihse Bursie  wrote:

> > I wonder if it's the SetupToolchain changes that has caused this. ccache is 
> > collapsed into CC and CXX to my knowledge
> 
> Yeah, it must have been. Would you like to take a look at it? Otherwise I'll 
> file a bug and fix it. Breaking ccache was an unintended regression, so it 
> needs to be fixed. If we want to drop ccache support it needs to be done 
> explicitly and separately.

No problem! I do hope that dropping ccache support isn't something that's 
planned though :(

> Additionally some environment variables are missing/different.

Could I trouble you to mention what exactly was different?

-

PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976398726

Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]

[Magnus Ihse Bursie]

On Mon, 4 Mar 2024 06:42:30 GMT, Julian Waters  wrote:

> I wonder if it's the SetupToolchain changes that has caused this. ccache is 
> collapsed into CC and CXX to my knowledge

Yeah, it must have been. Would you like to take a look at it? Otherwise I'll 
file a bug and fix it. Breaking ccache was an unintended regression, so it 
needs to be fixed. If we want to drop ccache support it needs to be done 
explicitly and separately.

-

PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976368808

Re: RFR: 8323183: ClassFile API performance improvements [v7]

[Claes Redestad]

On Mon, 15 Jan 2024 12:01:37 GMT, Adam Sotona  wrote:

>> ClassFile API performance related improvements have been separated from 
>> #17121 into this PR.
>> 
>> These improvements are important to minimize performance regression of
>> 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the 
>> Classfile API to generate proxy classes #17121
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update 
> src/java.base/share/classes/jdk/internal/classfile/impl/StackMapDecoder.java
>   
>   Co-authored-by: Andrey Turbanov 

Improvement looks good, but the microbenchmark needs to sink new instances into 
a blackhole to make sure we measure the right thing.

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java line 
36:

> 34: import java.lang.classfile.constantpool.MemberRefEntry;
> 35: import static java.lang.classfile.ClassFile.*;
> 36: import java.lang.constant.MethodTypeDesc;

Imports are strangely out-of-order and imports in the classfile package look 
haphazard. Is there some system to it that I don't see? I don't know if we have 
an applicable style guide to lean on, but alphabetically sorted and static 
imports split out at the end seem like the convention in the OpenJDK sources.

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java line 
103:

> 101: this.methodDesc = methodDesc;
> 102: this.cp = cp;
> 103: targets = new ArrayDeque<>();

Seeing `ArrayDeque` being put to good use always makes me happy.

test/micro/org/openjdk/bench/jdk/classfile/CodeAttributeTools.java line 108:

> 106: @Benchmark
> 107: public void benchmarkStackMapsGenerator() {
> 108: for (var d : data) new StackMapGenerator(

When we return something from a benchmark JMH makes sure to sink that something 
into a blackhole to avoid JIT assuming the result is unused and eliminating it, 
wholly or in parts. When it's impractical to return a single thing that 
captures all the computation in the benchmark it's good practice to let a 
`org.openjdk.jmh.infra.Blackhole` consume effects to attain the same effect:

public void benchmarkStackMapsGenerator(Blackhole bh) {
for (var d : data) bh.consume(new StackMapGenerator(

-

Changes requested by redestad (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17306#pullrequestreview-1913911634
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1510912679
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1510921619
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1510933717

RFR: 8327208: Remove unused method java.util.jar.Manifest.make72Safe

[Eirik Bjørsnøs]

Please review this cleanup PR which suggests we remove the package-private, 
unused and deprecated method `java.util.jar.Manifest.make72Safe`.

This method was marked deprecated after a cleanup/refactoring in 
[JDK-8066619](https://bugs.openjdk.org/browse/JDK-8066619) caused it to fall 
out of use. It should rather be removed.

Some tests reference the `make72Safe` methods in comment, or implement the 
legacy versions of the same logic to simulate pre JDK 11 line break behavior of 
`Manifest.write`. These comments and method names are adjusted to not reference 
the now removed method.

This change was initially discussed here: 
https://mail.openjdk.org/pipermail/core-libs-dev/2024-February/119819.html

-

Commit messages:
 - Update copyright years
 - Remove the internal, unused method Manifest.make72Safe. Update tests to not 
reference this legacy method.

Changes: https://git.openjdk.org/jdk/pull/18104/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18104&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8327208
  Stats: 33 lines in 3 files changed: 2 ins; 21 del; 10 mod
  Patch: https://git.openjdk.org/jdk/pull/18104.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18104/head:pull/18104

PR: https://git.openjdk.org/jdk/pull/18104

Re: RFR: 8323183: ClassFile API performance improvements [v7]

[Adam Sotona]

On Mon, 15 Jan 2024 12:01:37 GMT, Adam Sotona  wrote:

>> ClassFile API performance related improvements have been separated from 
>> #17121 into this PR.
>> 
>> These improvements are important to minimize performance regression of
>> 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the 
>> Classfile API to generate proxy classes #17121
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update 
> src/java.base/share/classes/jdk/internal/classfile/impl/StackMapDecoder.java
>   
>   Co-authored-by: Andrey Turbanov 

Please review.

Thank you!

-

PR Comment: https://git.openjdk.org/jdk/pull/17306#issuecomment-1976089621

Re: RFR: 8318650: Optimized subword gather for x86 targets. [v17]

[Emanuel Peter]

On Sat, 2 Mar 2024 16:22:22 GMT, Jatin Bhateja  wrote:

>> Hi All,
>> 
>> This patch optimizes sub-word gather operation for x86 targets with AVX2 and 
>> AVX512 features.
>> 
>> Following is the summary of changes:-
>> 
>> 1) Intrinsify sub-word gather using hybrid algorithm which initially 
>> partially unrolls scalar loop to accumulates values from gather indices into 
>> a quadword(64bit) slice followed by vector permutation to place the slice 
>> into appropriate vector lanes, it prevents code bloating and generates 
>> compact JIT sequence. This coupled with savings from expansive array 
>> allocation in existing java implementation translates into significant 
>> performance of 1.5-10x gains with included micro.
>> 
>> ![image](https://github.com/openjdk/jdk/assets/59989778/e25ba4ad-6a61-42fa-9566-452f741a9c6d)
>> 
>> 
>> 2) Patch was also compared against modified java fallback implementation by 
>> replacing temporary array allocation with zero initialized vector and a 
>> scalar loops which inserts gathered values into vector. But, vector insert 
>> operation in higher vector lanes is a three step process which first 
>> extracts the upper vector 128 bit lane, updates it with gather subword value 
>> and then inserts the lane back to its original position. This makes inserts 
>> into higher order lanes costly w.r.t to proposed solution. In addition 
>> generated JIT code for modified fallback implementation was very bulky. This 
>> may impact in-lining decisions into caller contexts.
>> 
>> Kindly review and share your feedback.
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review resolutions.

@jatin-bhateja thanks for all the work, this is a really nice feature!
And thanks for baring with all the comments 😊

Testing up to commit 14 looks good.

@PaulSandoz thanks for looking at the Vector API java code!

-

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16354#pullrequestreview-1913610983