Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v7]

[Mark Reinhold]

On Wed, 4 May 2022 23:10:06 GMT, Joe Darcy  wrote:

>> Add a new system property, java.specification.maintenance.version, to return 
>> the maintenance release number of the Java SE specification being 
>> implemented. The property is unset, optional in the terminology of 
>> System.getProperties, for an initial release of a specification.
>> 
>> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764
>> 
>> I'll update copyright years before an integration.
>
> Joe Darcy 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 12 additional commits since 
> the last revision:
> 
>  - Respond to mbreinhold review feedback.
>  - Merge branch 'master' into JDK-8285497
>  - Update wording to address review feedback.
>  - Merge branch 'master' into JDK-8285497
>  - Change punctuation from review feedback.
>  - Respond to review feedback; make sequence of values explicit.
>  - Respond to review feedback.
>  - Respond to review feedback.
>  - Respond to CSR feedback.
>  - Merge branch 'master' into JDK-8285497
>  - ... and 2 more: 
> https://git.openjdk.java.net/jdk/compare/f3cf898e...7b7720cf

Changes requested by mr (Lead).

src/java.base/share/classes/java/lang/System.java line 790:

> 788:  * href="https://jcp.org/en/procedures/jcp2#3.6.4;>maintenance
> 789:  * release. When defined, its value identifies that
> 790:  * maintenance release. To indicate the first maintenance release

The final sentence can be shortened, and looking at it now the semicolon should 
just be a comma:

 * maintenance release. To indicate the first maintenance release
 * this property will have the value {@code "1"}, to indicate the
 * second maintenance release it will have the value {@code "2"},
 * and so on.

Otherwise, this looks good to me.

-

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

Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v6]

[Mark Reinhold]

On Mon, 2 May 2022 20:31:18 GMT, Joe Darcy  wrote:

>> Add a new system property, java.specification.maintenance.version, to return 
>> the maintenance release number of the Java SE specification being 
>> implemented. The property is unset, optional in the terminology of 
>> System.getProperties, for an initial release of a specification.
>> 
>> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764
>> 
>> I'll update copyright years before an integration.
>
> Joe Darcy 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 10 additional commits since 
> the last revision:
> 
>  - Update wording to address review feedback.
>  - Merge branch 'master' into JDK-8285497
>  - Change punctuation from review feedback.
>  - Respond to review feedback; make sequence of values explicit.
>  - Respond to review feedback.
>  - Respond to review feedback.
>  - Respond to CSR feedback.
>  - Merge branch 'master' into JDK-8285497
>  - Update comment in template.
>  - JDK-8285497: Add system property for Java SE specification maintenance 
> version

Changes requested by mr (Lead).

-

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

Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]

[Mark Reinhold]

On Tue, 3 May 2022 01:20:10 GMT, Joe Darcy  wrote:

>> In the latest push, to address the concerns raised updated the proposed 
>> wording to, in plain text:
>> 
>> Java Runtime Environment specification maintenance version, not defined 
>> before the specification implemented by this runtime has undergone a 
>> maintenance release (optional). When defined, the value of this property may 
>> be interpreted as a positive integer. The value indicates the latest 
>> maintenance release the runtime is known to support. A later release may be 
>> supported by the environment. To indicate the first maintenance release this 
>> property will have the value "1"; to indicate the second maintenance 
>> release, this property will have the value "2", and so on.
>
> PS CSR Updated to reflect this push; please review: 
> https://bugs.openjdk.java.net/browse/JDK-8285764

The negative definition above permits the property always to be undefined, but 
we do want it to be defined when meaningful. It’s also getting to be an awful 
lot of text to add to the otherwise terse tabular summary of system properties 
in the [`System::getProperties()` 
specification](https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/System.html#getProperties()).

Consider using this text in the table:

> Java Runtime Environment specification maintenance version, which may be 
> interpreted as a positive integer (optional, see below)

and then this in a paragraph following the table:

> The {@code java.specification.maintenance.version} property is defined if the 
> specification implemented by this runtime at the time of its construction had 
> undergone a https://jcp.org/en/procedures/jcp2#3.6.4;>maintenance 
> release. When defined, its value identifies that maintenance release. To 
> indicate the first maintenance release this property will have the value 
> {@code "1"}; to indicate the second maintenance release this property will 
> have the value {@code "2"}, and so on.

-

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

Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]

[Mark Reinhold]

On Fri, 29 Apr 2022 02:12:19 GMT, Iris Clark  wrote:

>> Joe Darcy has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Change punctuation from review feedback.
>
> src/java.base/share/classes/java/lang/System.java line 743:
> 
>> 741:  * have the value {@code "1"}; after a second maintenance
>> 742:  * release, this property will have the value {@code "2"},
>> 743:  * and so on.
> 
> There should be no requirement that values be allocated sequentially.  In 
> other words, if JCP MR  does not require an RI, then it should not be 
> surprising if there is no JDK build with maintenance version .  As an 
> example, JSR 337 MR 1 and MR 2 both used the same RI.  If this system 
> property had existed during development of MR 1, it would return "1".  Since 
> no RI was submitted for MR 2, a build returning "2" should never exist.  MR 3 
> did update the RI, so it would return "3".

@irisclark does raise an interesting point: If, say, MR 2 doesn’t require a 
change to the RI then the MR 1 RI is also the MR 2 RI, but its 
`java.specification.maintenance.version` property will report that it’s the MR 
1 RI.

One way to fix this would be to require an RI update with every MR just to 
update this property, even if no other code in the RI changes — but we prefer 
to avoid doing RI builds unnecessarily.

Another way to fix it would be to finesse the specification of this property, 
perhaps:


 * {@systemProperty 
java.specification.maintenance.version}
 * Java Runtime Environment specification maintenance version, 
which may
 * be interpreted as a non-zero integer. If defined, the value of 
this
 * property is the identifying number of the most recent https://jcp.org/en/procedures/jcp2#3.6.4;>specification
 * maintenance release that required a change to the runtime
 * (optional).
 * 

-

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

Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v4]

[Mark Reinhold]

On Thu, 28 Apr 2022 20:54:29 GMT, Joe Darcy  wrote:

>> Add a new system property, java.specification.maintenance.version, to return 
>> the maintenance release number of the Java SE specification being 
>> implemented. The property is unset, optional in the terminology of 
>> System.getProperties, for an initial release of a specification.
>> 
>> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764
>> 
>> I'll update copyright years before an integration.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback; make sequence of values explicit.

src/java.base/share/classes/java/lang/System.java line 740:

> 738:  *  href="https://jcp.org/en/procedures/jcp2#3.6.4;>maintenance
> 739:  * release (optional).
> 740:  * After a first maintenance release, this property will

Separate independent clauses with a semicolon: `After a first maintenance 
release this property will have the value {@code "1"}; after a second 
maintenance release it will have the value {@code "2"}, and so on.`

-

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

Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v3]

[Mark Reinhold]

On Thu, 28 Apr 2022 20:15:36 GMT, Joe Darcy  wrote:

>> Add a new system property, java.specification.maintenance.version, to return 
>> the maintenance release number of the Java SE specification being 
>> implemented. The property is unset, optional in the terminology of 
>> System.getProperties, for an initial release of a specification.
>> 
>> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764
>> 
>> I'll update copyright years before an integration.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

Marked as reviewed by mr (Lead).

-

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

Re: RFR: JDK-8285764: Add system property for Java SE specification maintenance version [v2]

[Mark Reinhold]

On Thu, 28 Apr 2022 17:36:32 GMT, Joe Darcy  wrote:

>> Add a new system property, java.specification.maintenance.version, to return 
>> the maintenance release number of the Java SE specification being 
>> implemented. The property is unset, optional in the terminology of 
>> System.getProperties, for an initial release of a specification.
>> 
>> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764
>> 
>> I'll update copyright years before an integration.
>
> Joe Darcy 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:
> 
>  - Respond to review feedback.
>  - Respond to CSR feedback.
>  - Merge branch 'master' into JDK-8285497
>  - Update comment in template.
>  - JDK-8285497: Add system property for Java SE specification maintenance 
> version

Also, don't forget to update the CSR with the new specification text, for the 
record.
Otherwise, looks good!

src/java.base/share/classes/java/lang/VersionProps.java.template line 113:

> 111: props.put("java.specification.version", VERSION_SPECIFICATION);
> 112: 
> 113: // Uncomment next props.put call after the first maintenance 
> review for a

s/review/release/

-

Changes requested by mr (Lead).

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

[jdk18] Integrated: 8276826: Clarify the ModuleDescriptor.Version specification’s treatment of repeated punctuation characters

[Mark Reinhold]

On Thu, 16 Dec 2021 22:16:26 GMT, Mark Reinhold  wrote:

> Please review this tiny specification clarification.

This pull request has now been integrated.

Changeset: f5d7c777
Author:    Mark Reinhold 
URL:   
https://git.openjdk.java.net/jdk18/commit/f5d7c777bc516fa2e711c19d5281ebf32384b543
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod

8276826: Clarify the ModuleDescriptor.Version specification’s treatment of 
repeated punctuation characters

Reviewed-by: mchung, darcy

-

PR: https://git.openjdk.java.net/jdk18/pull/39

Re: [jdk18] RFR: 8276826: Clarify the ModuleDescriptor.Version specification’s treatment of repeated punctuation characters

[Mark Reinhold]

On Thu, 16 Dec 2021 22:16:26 GMT, Mark Reinhold  wrote:

> Please review this tiny specification clarification.

Drat, used the wrong bug id.

-

PR: https://git.openjdk.java.net/jdk18/pull/39

[jdk18] RFR: 8276826: Clarify the ModuleDescriptor.Version specification’s treatment of repeated punctuation characters

[Mark Reinhold]

Please review this tiny specification clarification.

-

Commit messages:
 - 8278465: Clarify the ModuleDescriptor.Version specification’s treatment of 
repeated punctuation characters

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

PR: https://git.openjdk.java.net/jdk18/pull/39

Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v4]

[Mark Reinhold]

On Tue, 23 Nov 2021 20:29:15 GMT, Andrew Leonard  wrote:

>> Add a new --source-date  (epoch seconds) option to jar and jmod 
>> to allow specification of time to use for created/updated jar/jmod entries. 
>> This then allows the ability to make the content deterministic.
>> 
>> Signed-off-by: Andrew Leonard 
>
> Andrew Leonard has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains five commits:
> 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 
>  - 8276766: Enable jar and jmod to produce deterministic timestamped content
>
>Signed-off-by: Andrew Leonard 

A user who’s not familiar with the lingo of [reproducible 
builds](https://reproducible-builds.org/docs/source-date-epoch/) will be 
mystified by an option named `--source-date`.  A user who just wants to set the 
timestamp of new entries won’t be looking for an option whose name includes 
“source.”

Please consider providing a more general option, say `--date`, which takes an 
[ISO 8601 date/time 
string](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/format/DateTimeFormatter.html#ISO_DATE_TIME).
 That would solve the general problem while also satisfying the requirement for 
reproducible builds. In the build it’s easy enough to convert the seconds-since 
epoch value of `SOURCE_DATE_EPOCH` to an ISO 8601 string (`date -d 
@$SOURCE_DATE_EPOCH --iso-8601=seconds`).

-

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

New candidate JEP: 421: Deprecate Finalization for Removal

[mark . reinhold]

https://openjdk.java.net/jeps/421

  Summary: Deprecate finalization for removal in a future
  release. Finalization remains enabled by default for now, but can
  be disabled to facilitate early testing. In a future release it
  will be disabled by default, and in a later release it will be
  removed. Maintainers of libraries and applications that rely upon
  finalization should consider migrating to other resource management
  techniques such as the try-with-resources statement and cleaners.

- Mark

Re: RFR: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently

[Mark Reinhold]

On Thu, 30 Sep 2021 11:26:12 GMT, Masanori Yano  wrote:

> @mbreinhold Could you comment on this pull request?

This is somewhat tricky code. I’ll take a look, but give me a few days.

-

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

Re: [jdk17] RFR: JDK-8270872: Final nroff manpage update for JDK 17

[Mark Reinhold]

On Thu, 5 Aug 2021 21:40:40 GMT, Naoto Sato  wrote:

>> According to the comments in the makefile 
>> (`closed/make/UpdateOpenManPages.gmk`) the copyright line is taken from the 
>> original Markdown file, so if the year is wrong there, it will be wrong in 
>> the generated nroff file.
>> 
>> I think it would be incorrect to edit the dates locally in these files, 
>> because they'll just be overwritten when we generate the files again. 
>> Ideally, the dates should be fixed (if necessary) in the Markdown files, but 
>> that seems out of scope for this P1.
>> 
>> This is "just" an issue with copyright dates in source files ... and yes, 
>> while I know copyright dates are important, this problem is arguably part of 
>> an ongoing more general problem.
>> 
>> I note that the generated files *do* correctly identify themselves with 
>> `2021` in the visible output generated to the console by the `man` command.
>
> Thanks for the explanation, Jon. Fine by me.

I agree that fixing this date is not necessary at this time.

-

PR: https://git.openjdk.java.net/jdk17/pull/303

Re: [jdk17] RFR: JDK-8270872: Final nroff manpage update for JDK 17

[Mark Reinhold]

On Thu, 5 Aug 2021 19:20:50 GMT, Jonathan Gibbons  wrote:

> Please review a semi-automatic update of the nroff man pages from the 
> upstream files.

Marked as reviewed by mr (Lead).

-

PR: https://git.openjdk.java.net/jdk17/pull/303

New candidate JEP: 416: Reimplement Core Reflection with Method Handles

[mark . reinhold]

https://openjdk.java.net/jeps/416

  Summary: Reimplement java.lang.reflect.Method, Constructor, and Field
  on top of java.lang.invoke method handles.  Making method handles the
  underlying mechanism for reflection will reduce the maintenance and
  development cost of both the java.lang.reflect and java.lang.invoke
  APIs.

- Mark

Integrated: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals

[Mark Reinhold]

On Thu, 13 May 2021 17:14:36 GMT, Mark Reinhold  wrote:

> Please review this implementation of JEP 403 
> (https://openjdk.java.net/jeps/403).
> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
> {linux,macos,windows}-x64 and {linux,macos}-aarch64.

This pull request has now been integrated.

Changeset: e6302354
Author:Mark Reinhold 
URL:   
https://git.openjdk.java.net/jdk/commit/e63023546aaf48ae39c72ab37f6ef3f5474e19cc
Stats: 2842 lines in 26 files changed: 0 ins; 2792 del; 50 mod

8266851: Implement JEP 403: Strongly Encapsulate JDK Internals

Co-authored-by: Alan Bateman 
Reviewed-by: mchung, alanb, hseigel

-

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v3]

[Mark Reinhold]

On Fri, 14 May 2021 08:54:23 GMT, Alan Bateman  wrote:

>> Mark Reinhold has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove now-unnecessary uses and mentions of the --illegal-access option
>
> test/jdk/tools/launcher/modules/illegalaccess/IllegalAccessTest.java line 26:
> 
>> 24: /**
>> 25:  * @test
>> 26:  * @requires vm.compMode != "Xcomp"
> 
> I think `@requires vm.compMode != "Xcomp" was added because the test took too 
> long with -Xcomp. I think it can be dropped now.

Dropped.

> test/jdk/tools/launcher/modules/illegalaccess/IllegalAccessTest.java line 37:
> 
>> 35: 
>> 36: /**
>> 37:  * Basic test of --illegal-access=value to deny or permit access to JDK 
>> internals.
> 
> The comment should probably be updated as the test no longer tests denying or 
> permitting access.

Fixed.

-

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v4]

[Mark Reinhold]

> Please review this implementation of JEP 403 
> (https://openjdk.java.net/jeps/403).
> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
> {linux,macos,windows}-x64 and {linux,macos}-aarch64.

Mark Reinhold has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix up IllegalAccessTest

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4015/files
  - new: https://git.openjdk.java.net/jdk/pull/4015/files/1c14998e..cde0adbb

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

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

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v2]

[Mark Reinhold]

On Thu, 13 May 2021 18:16:23 GMT, Mandy Chung  wrote:

>> Mark Reinhold has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove stray mentions of the jdk.module.illegalAccess property
>
> There are a few tests with `--illegal-access=deny` for example
> 
> test/jdk/java/lang/ModuleTests/BasicModuleTest.java
> test/jdk/java/lang/instrument/RedefineModuleTest.java
> test/jdk/java/lang/invoke/CallerSensitiveAccess.java
> test/jdk/java/lang/reflect/AccessibleObject/ - a few tests
> test/jdk/jdk/modules/open/Basic.java
> test/jdk/tools/launcher/modules/addexports/manifest/AddExportsAndOpensInManifest.java
> 
> 
> It would be good to remove these references to `--illegal-access` option in 
> this patch too.

@mlchung Thanks for pointing out those stray uses of `--illegal-access`. I’ve 
removed them, along with the mention of `--illegal-access` in 
`launcher.properties`.

-

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v3]

[Mark Reinhold]

> Please review this implementation of JEP 403 
> (https://openjdk.java.net/jeps/403).
> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
> {linux,macos,windows}-x64 and {linux,macos}-aarch64.

Mark Reinhold has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove now-unnecessary uses and mentions of the --illegal-access option

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4015/files
  - new: https://git.openjdk.java.net/jdk/pull/4015/files/719ff4ee..1c14998e

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

  Stats: 26 lines in 10 files changed: 0 ins; 8 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4015.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4015/head:pull/4015

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v2]

[Mark Reinhold]

On Thu, 13 May 2021 17:37:42 GMT, Harold Seigel  wrote:

>> Mark Reinhold has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove stray mentions of the jdk.module.illegalAccess property
>
> src/hotspot/share/runtime/arguments.cpp line 2434:
> 
>> 2432: // -agentlib and -agentpath
>> 2433: } else if (match_option(option, "-agentlib:", ) ||
>> 2434:   (is_absolute_path = match_option(option, "-agentpath:", 
>> ))) {
> 
> I think jdk.module.illegalAccess can also be removed from lines 1332 and 2064 
> of arguments.cpp.
> Thanks, Harold

Good point — fixed.  Thanks.

-

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v2]

[Mark Reinhold]

> Please review this implementation of JEP 403 
> (https://openjdk.java.net/jeps/403).
> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
> {linux,macos,windows}-x64 and {linux,macos}-aarch64.

Mark Reinhold has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove stray mentions of the jdk.module.illegalAccess property

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4015/files
  - new: https://git.openjdk.java.net/jdk/pull/4015/files/4e2cbf93..719ff4ee

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

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

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

RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals

[Mark Reinhold]

Please review this implementation of JEP 403 
(https://openjdk.java.net/jeps/403).
Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
{linux,macos,windows}-x64 and {linux,macos}-aarch64.

-

Commit messages:
 - 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals

Changes: https://git.openjdk.java.net/jdk/pull/4015/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4015=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8266851
  Stats: 2811 lines in 16 files changed: 0 ins; 2782 del; 29 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4015.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4015/head:pull/4015

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

Re: RFR: 8266783: java\lang\reflect\Proxy\DefaultMethods.java fails with jtreg 6

[Mark Reinhold]

On Tue, 11 May 2021 16:48:28 GMT, Mandy Chung  wrote:

> Data provider with varargs does not work on TestNG 7.4.0.   Fix the test to 
> create an object array instead of varargs to workaround this issue.

All those backslashes hurt my eyes. Could we please use forward slashes, both 
in the commit message and the issue summary? This isn’t a regular expression.

-

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

New candidate JEP: 415: Context-Specific Deserialization Filters

[mark . reinhold]

https://openjdk.java.net/jeps/415

  Summary: Allow applications to configure context-specific and
  dynamically-selected deserialization filters via a JVM-wide filter
  factory that is invoked to select a filter for each individual
  deserialization operation.

- Mark

Re: RFR: 8265961: Fix comments in logging.properties [v2]

[Mark Reinhold]

On Mon, 26 Apr 2021 17:14:29 GMT, Pavel Rappo  wrote:

>> I had been looking for an example of a "properties" file when spotted a typo 
>> in `logging.properties`. I decided to proofread the file. That resulted in 
>> finding a few other issues.
>
> Pavel Rappo has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rollback "in" -> "with"; remove more trailing whitespace

Marked as reviewed by mr (Lead).

-

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

Re: RFR: 8265961: Fix comments in logging.properties

[Mark Reinhold]

On Mon, 26 Apr 2021 09:33:03 GMT, Pavel Rappo  wrote:

> I had been looking for an example of a "properties" file when spotted a typo 
> in `logging.properties`. I decided to proofread the file. That resulted in 
> finding a few other issues.

Otherwise, these changes look fine.

src/java.logging/share/conf/logging.properties line 5:

> 3: #
> 4: # You can use a different file by specifying a filename
> 5: # in the java.util.logging.config.file system property.

Unless the `java.util.logging.config` property contains more than one item of 
information, the original "with" is more appropriate here.

src/java.logging/share/conf/logging.properties line 46:

> 44: java.util.logging.FileHandler.formatter = java.util.logging.XMLFormatter
> 45: 
> 46: # Limit the messages that are printed on the console to INFO and above.

"Limit the messages ..." could be more direct. Consider "Only print messages to 
the console if they have level INFO or above."

-

Changes requested by mr (Lead).

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

Re: Dynamic Deserialization Filters - a JEP draft

[mark . reinhold]

2021/3/26 8:04:23 -0700, roger.ri...@oracle.com:
> Please review a JEP to extend deserialization filtering (JEP 290) to 
> enable deserialization filters to be selected by the application based 
> on the runtime context.
> 
> JDK-8263381  Dynamic 
> Deserialization Filters

More readably: https://openjdk.java.net/jeps/8263381

- Mark

New candidate JEP: 400: UTF-8 by Default

[mark . reinhold]

https://openjdk.java.net/jeps/400

  Summary: Use UTF-8 as the JDK's default charset, so that APIs that
  depend on the default charset behave consistently across all platforms
  and independently of the user’s locale and configuration.

- Mark

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v14]

[Mark Reinhold]

On Mon, 18 Jan 2021 16:45:00 GMT, Jim Laskey  wrote:

>> This PR is to introduce a new random number API for the JDK. The primary API 
>> is found in RandomGenerator and RandomGeneratorFactory. Further description 
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>> 
>> javadoc can be found at 
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>> 
>> old PR:  https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update package info to credit LMX algorithm

src/java.base/share/classes/java/util/random/RandomGenerator.java line 110:

> 108:  /**
> 109:  * Returns an instance of {@link RandomGenerator} that utilizes the
> 110:  * {@code name} algorithm.

Shouldn't this method, and related methods, mention the fact that 
`RandomGenerator` instances are located as services? I see no mention of of 
that fact anywhere, unless I missed it, but I do see the `uses` and `provides` 
declarations in the module declaration. A paragraph explaining how services are 
used here, perhaps in the package specification, would be ideal.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[mark . reinhold]

2020/12/4 6:08:13 -0800, er...@openjdk.java.net:
> On Fri, 4 Dec 2020 12:30:02 GMT, Alan Bateman  wrote:
>>> And I can certainly move jdwp.spec to java.base instead. That's the
>>> reason I need input on this: All I know is that is definitely not
>>> the responsibility of the Build Group to maintain that document, and
>>> I made my best guess at where to place it.
>> 
>>> And I can certainly move jdwp.spec to java.base instead.
>> 
>> If jdwp.spec has to move to the src tree then src/java.se is probably
>> the most suitable home because Java SE specifies JDWP as an optional
>> interface. So nothing to do with java.base and the build will need to
>> continue to generate the sources for the front-end (jdk.jdi) and
>> back-end (jdk.jdwp.agent) as they implement the protocol.
> 
> My understanding of JEPs is that they should be viewed as living
> documents. In this case, I think it's perfectly valid to update JEP
> 201 with additional source code layout. Both for this and for the
> other missing dirs.

Feature JEPs are living documents until such time as they are delivered.
In this case it would not be appropriate to update JEP 201, which is as
much about the transition from the old source-code layout as it is about
the new layout as of 2014.

At this point, and given that we’d already gone beyond JEP 201 prior to
this change (with `man` and `lib` subdirectories), what’d make the most
sense is a new informational JEP that documents the source-code layout.
Informational JEPs can, within reason, be updated over time.

- Mark

Integrated: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default

[Mark Reinhold]

On Thu, 19 Nov 2020 16:49:30 GMT, Mark Reinhold  wrote:

> Please review this implementation of JEP 396 
> (https://openjdk.java.net/jeps/396).
> Alan Bateman is the original author; I’ve credited him in the commit metadata.
> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.

This pull request has now been integrated.

Changeset: ed4c4ee7
Author:Mark Reinhold 
URL:   https://git.openjdk.java.net/jdk/commit/ed4c4ee7
Stats: 162 lines in 5 files changed: 24 ins; 73 del; 65 mod

8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default

Co-authored-by: Alan Bateman 
Reviewed-by: mchung, alanb

-

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

Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v6]

[Mark Reinhold]

On Fri, 20 Nov 2020 13:08:11 GMT, Alan Bateman  wrote:

>> Looks good.
>
> testWithAddExportsInManifest create an executable JAR with "Add-Exports: 
> java.base/sun.security.x509" in the manifest. It runs it twice, once without 
> any options, the second with --illegal-access=permit, and checks there are no 
> warnings in both cases. To test attempted deep reflection here would need the 
> equivalent of setAccessibleNonPublicMemberNonExportedPackage that tries to 
> access some privates in sun.security.x509. It's okay to use 
> setAccessibleNonPublicMemberNonExportedPackage to test that privates in 
> sun.nio.ch aren't accessible but it's not connected to the Add-Exports 
> attribute in the JAR file.

Thanks for pointing that out. I fixed it by arranging for the `Add-Exports` 
test to export both `sun.security.x509` and `sun.nio.ch`.

-

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

Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v6]

[Mark Reinhold]

> Please review this implementation of JEP 396 
> (https://openjdk.java.net/jeps/396).
> Alan Bateman is the original author; I’ve credited him in the commit metadata.
> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.

Mark Reinhold has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix "Add-Exports" case for the setAccessibleNonPublicMemberNonExportedPackage 
test (v2)

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1324/files
  - new: https://git.openjdk.java.net/jdk/pull/1324/files/1a7ab4e0..0c9654a0

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

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

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

Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v5]

[Mark Reinhold]

> Please review this implementation of JEP 396 
> (https://openjdk.java.net/jeps/396).
> Alan Bateman is the original author; I’ve credited him in the commit metadata.
> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.

Mark Reinhold has updated the pull request incrementally with one additional 
commit since the last revision:

  Revert: Fix "Add-Exports" case for the 
setAccessibleNonPublicMemberNonExportedPackage test
  
  This reverts commit 6a4d4792261f54b014336d1907b0040b15fdb467.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1324/files
  - new: https://git.openjdk.java.net/jdk/pull/1324/files/6a4d4792..1a7ab4e0

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

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

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

Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v4]

[Mark Reinhold]

> Please review this implementation of JEP 396 
> (https://openjdk.java.net/jeps/396).
> Alan Bateman is the original author; I’ve credited him in the commit metadata.
> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.

Mark Reinhold has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix "Add-Exports" case for the setAccessibleNonPublicMemberNonExportedPackage 
test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1324/files
  - new: https://git.openjdk.java.net/jdk/pull/1324/files/a57e6065..6a4d4792

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

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

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

Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v3]

[Mark Reinhold]

> Please review this implementation of JEP 396 
> (https://openjdk.java.net/jeps/396).
> Alan Bateman is the original author; I’ve credited him in the commit metadata.
> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.

Mark Reinhold has updated the pull request incrementally with one additional 
commit since the last revision:

  Add "Add-Exports" case for the setAccessibleNonPublicMemberNonExportedPackage 
test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1324/files
  - new: https://git.openjdk.java.net/jdk/pull/1324/files/baf34a19..a57e6065

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

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

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

Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v3]

[Mark Reinhold]

On Thu, 19 Nov 2020 19:51:59 GMT, Mandy Chung  wrote:

>> Mark Reinhold has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add "Add-Exports" case for the 
>> setAccessibleNonPublicMemberNonExportedPackage test
>
> test/jdk/tools/launcher/modules/illegalaccess/IllegalAccessTest.java line 387:
> 
>> 385: "--illegal-access=permit");
>> 386: }
>> 387: 
> 
> I see `setAccessibleNonPublicMemberExportedPackage` test case testing with 
> `--add-exports` and `--add-opens` and `Add-Opens` in JAR file manifest but 
> not testing with `Add-Exports`.
> 
> Is it worth to include the `Add-Exports` test case?
> 
> Other than this, looks good.

Good point -- case added.

-

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

Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v2]

[Mark Reinhold]

> Please review this implementation of JEP 396 
> (https://openjdk.java.net/jeps/396).
> Alan Bateman is the original author; I’ve credited him in the commit metadata.
> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.

Mark Reinhold has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove unnecessary @module :open from ASN1FormatterTest.java

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1324/files
  - new: https://git.openjdk.java.net/jdk/pull/1324/files/4c0e8887..baf34a19

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

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

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

Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v2]

[Mark Reinhold]

On Thu, 19 Nov 2020 17:03:54 GMT, Alan Bateman  wrote:

>> Mark Reinhold has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove unnecessary @module :open from ASN1FormatterTest.java
>
> test/lib-test/jdk/test/lib/hexdump/ASN1FormatterTest.java line 42:
> 
>> 40:  * @test
>> 41:  * @summary ASN.1 formatting
>> 42:  * @modules java.base/sun.security.util:open
> 
> Roger has since fixed the test via JDK-8255394 so it no longer requires this 
> package to be open.

Thanks! I've removed that :open.

-

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

RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default

[Mark Reinhold]

Please review this implementation of JEP 396 
(https://openjdk.java.net/jeps/396).
Alan Bateman is the original author; I’ve credited him in the commit metadata.
Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.

-

Commit messages:
 - 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default

Changes: https://git.openjdk.java.net/jdk/pull/1324/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1324=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256299
  Stats: 161 lines in 6 files changed: 22 ins; 76 del; 63 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1324.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1324/head:pull/1324

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

Re: RFR: 8255989: Remove explicitly unascribed authorship from Java source files

[Mark Reinhold]

On Fri, 6 Nov 2020 20:11:24 GMT, Pavel Rappo  wrote:

> This PR proposes to remove
> 1. JavaDoc `@author` tags with unclear semantics: `@author 
> unascribed|unattributed|unknown`
> 2. A couple of astray Form Feed (a.k.a. FF, `\f`,  `0xC`, or `^L`) characters

Marked as reviewed by mr (Lead).

-

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

Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag

[Mark Reinhold]

On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons  wrote:

> The change is (just) to remove legacy usages of a JDK-private custom tag.

As the creator of these tags many moons ago, I approve this change.

-

Marked as reviewed by mr (Lead).

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

New candidate JEP: 392: Packaging Tool

[mark . reinhold]

https://openjdk.java.net/jeps/392

- Mark

Re: RFR 8251989: Hex encoder and decoder utility

[mark . reinhold]

2020/8/20 13:59:39 -0700, roger.ri...@oracle.com:
> On 8/20/20 3:10 PM, mark.reinh...@oracle.com wrote:
>> ...
>> 
>> A few comments:
>> 
>>   - Why do the short-form `encoder` factory methods return encoders that
>> produce upper-case hex strings?  `Integer::toHexString` and other
>> existing `toHex` methods return lower-case hex strings.  That’s also
>> what you get from common Unix CLI tools (e.g., `od -tx1`).
>> 
>> Please consider changing these methods to return lower-case encoders.
> 
> It's (almost) a toss up and easy to change; many of the existing uses 
> produce uppercase.

Please change the default to lower case.  It’s what people are going to
expect.

> Perhaps the short form no-arg should be replaced with a short form 
> constructor that
> takes true/false, so it is explicit at the use site or put the case in 
> the name.
> encodeToUpper(), encodeToLower().
> (A boolean parameter is not very informative, a enum would be better but 
> perhaps a bit heavyweight).

Either a boolean or an enum parameter would be massive overkill.

>>   - Is it worth having static `Hex.encode(byte[])` and
>> `Hex.decode(CharSequence)` convenience methods for the simplest
>> cases?
> 
> There was some discussion of that but it idea was to minimize the 
> surface area.

Sorry, where was that discussion?  I can’t find it in the core-libs-dev
archive.

>>   - [Warning: Bikeshed] The verbs “encode” and “decode” seem unfortunate.
>> 
>> Over in `java.nio.charsets` we have encoders that transform
>> characters to bytes, and decoders that transform bytes to characters.
>> The coded thing is the bytes; the uncoded thing is the characters.
>> 
>> In `java.util` we already have the `Base64` class, which provides
>> encoders that transform bytes to characters, and decoders that
>> transform characters to bytes.  The coded thing is the characters;
>> the uncoded thing is the bytes.
>> 
>> The use of “encode” and “decode” in `Base64` was likely inspired by
>> the fact that the format has been known as “base 64 encoding” for
>> decades, having originated as a hack for transporting non-ASCII data
>> via SMTP.  Developers looking to do base-64 operations will, thus,
>> expect this terminology.
> 
> The bias of encoding vs decoding terminology is subtle, based the 'native'
> form of the data. For the Charset classes, the native form of the data 
> is characters,
> and the encoded form is bytes. For Base64, the native form of the data 
> is bytes,
> and the encoded form is Base64 lines.

Indeed -- and my point is that, in this case, encoding and decoding
might not be the best concepts for API discoverability.  (But, maybe
they are.)

It’s technically true that this API does encoding and decoding,
whichever way you look at it.  What I wonder about are the typical use
cases.  Will the primary uses of this API be to encode bytes into
characters so that they can be transported via a medium that can only
handle characters, and then decode the characters back into bytes on the
other end?  Or will the primary uses be to format bytes into a readable
form, and likewise parse arbitrary-length hex strings into bytes?

>> Here you’re proposing that the `Hex` class follow the `Base64` class.
>> Consistency with existing nearby APIs is a worthy goal.  If I were
>> just looking to convert a byte array into a readable hex string,
>> however, I’d probably want to “format” it rather than “encode” it,
>> something like `String.format("%x")` on steroids.  Likewise, if I
>> were looking to convert a hex string into bytes then I’d want to
>> “parse” it rather than “decode” it, i.e., `Integer::parseInt` on
>> steroids.
>> 
>> If you were to rename the nested classes to `Hex.Formatter` and
>> `Hex.Parser`, and rename all methods accordingly, then this API would
>> be inconsistent with the nearby `Base64` but likely more consistent
>> with developer expectations.
>> 
>> (`Hex` is already inconsistent with `Base64` in that it doesn’t
>>  prefix the names of its factory methods with `get`, which is a good
>>  thing.)
> 
> The question is at what level of control does "encoding" become formatting.
> There are very few formatting features in the API, no control of leading 
> zeros, no control
> over indicental whitespace, and no control over width. Similarly with 
> parsing,
> what flexibility does parsing imply that decoding does not, is 
> whitespace ignored, line endings/joins, etc.

Optional support for prefix, suffix, and delimiter characters in both
operations, and support for both lower- and upper-case characters on
input, sure look like formatting and parsing to me.

- Mark

Re: RFR 8251989: Hex encoder and decoder utility

[mark . reinhold]

2020/8/19 14:14:56 -0700, roger.ri...@oracle.com:
> Please review a java.util.Hex API to encode and decode hexadecimal 
> strings to and from byte arrays.
> 
> JavaDoc:
> http://cr.openjdk.java.net/~rriggs/hex-javadoc/java.base/java/util/Hex.html

This mostly looks good -- it’ll be nice to have a standard utility for
this sort of thing.

A few comments:

  - Why do the short-form `encoder` factory methods return encoders that
produce upper-case hex strings?  `Integer::toHexString` and other
existing `toHex` methods return lower-case hex strings.  That’s also
what you get from common Unix CLI tools (e.g., `od -tx1`).

Please consider changing these methods to return lower-case encoders.

  - Is it worth having static `Hex.encode(byte[])` and
`Hex.decode(CharSequence)` convenience methods for the simplest
cases?

  - [Warning: Bikeshed] The verbs “encode” and “decode” seem unfortunate.

Over in `java.nio.charsets` we have encoders that transform
characters to bytes, and decoders that transform bytes to characters.
The coded thing is the bytes; the uncoded thing is the characters.

In `java.util` we already have the `Base64` class, which provides
encoders that transform bytes to characters, and decoders that
transform characters to bytes.  The coded thing is the characters;
the uncoded thing is the bytes.

The use of “encode” and “decode” in `Base64` was likely inspired by
the fact that the format has been known as “base 64 encoding” for
decades, having originated as a hack for transporting non-ASCII data
via SMTP.  Developers looking to do base-64 operations will, thus,
expect this terminology.

Here you’re proposing that the `Hex` class follow the `Base64` class.
Consistency with existing nearby APIs is a worthy goal.  If I were
just looking to convert a byte array into a readable hex string,
however, I’d probably want to “format” it rather than “encode” it,
something like `String.format("%x")` on steroids.  Likewise, if I
were looking to convert a hex string into bytes then I’d want to
“parse” it rather than “decode” it, i.e., `Integer::parseInt` on
steroids.

If you were to rename the nested classes to `Hex.Formatter` and
`Hex.Parser`, and rename all methods accordingly, then this API would
be inconsistent with the nearby `Base64` but likely more consistent
with developer expectations.

(`Hex` is already inconsistent with `Base64` in that it doesn’t
 prefix the names of its factory methods with `get`, which is a good
 thing.)

- Mark

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

[mark . reinhold]

2020/6/3 19:31:13 -0700, mandy.ch...@oracle.com:
> On 6/3/20 5:16 PM, Paul Sandoz wrote:
>> Did you consider an alternative name, such as:
>> 
>> /**
>>  * Initializes {@code targetClass}, if not already initialized.
>>  * ...
>> */
>> public Class initializeClass(Class targetClass) ...
>> 
>> ?
> 
> I considered this. The targetClass may have been initialized. 
> `initializeClass` seems to be read that it should check if it's 
> initialized before calling??  I also considered `ensureInitialized` 
> which can be an option. I'm open to suggestion.

I agree that the `ensure` prefix is useful here.

This method can only ensure that a class is initialized, so including
`Class` in the method name seems redundant.  I’d go with the shorter
`ensureInitialized`.

- Mark

New candidate JEP: 385: Deprecate RMI Activation for Removal

[mark . reinhold]

https://openjdk.java.net/jeps/385

- Mark

Re: RFR: 8242039: Improve jlink VersionPropsPlugin

[mark . reinhold]

2020/4/3 6:36:53 -0700, christoph.lan...@sap.com:
> 2020/4/2 14:12:54 -0700, mark.reinh...@oracle.com:
>> I thought about doing this when I originally wrote that plugin, but it’s
>> so awkward to achieve with ASM -- as demonstrated by your patch -- that
>> I concluded it wasn’t worth it.  Who will notice an extra pop in a basic
>> block that’s only ever executed once?  Is the complexity of this new
>> code worth the benefit?
> 
> Well, first I started playing with this and got a bit obsessed to find
> optimizations in that area. (I learned quite a bit about java asm.)
> It would be of higher (micro-)benefit for common VM startup if the
> fields to be modified could be final but that's even more awkward to
> do and requires intricate knowledge and assumptions about how
> VersionProps.java is structured. So I decided against messing with
> that.
> 
> Eventually I came up with this result and then I also asked myself the
> question whether the new complexity was worth the benefit. I answered
> myself with a yes (though definitely not a clear one ), and that's
> why I proposed the change. After all, the new complexity isn't huge...
> 
> So, would that be your terminal veto or could you imagine accepting
> the change?

I won’t veto it.

- Mark

Re: RFR: 8242039: Improve jlink VersionPropsPlugin

[mark . reinhold]

2020/4/2 8:01:28 -0700, christoph.lan...@sap.com:
> please review a small improvement for the jlink
> VersionPropsPlugin. The Plugin modifies the bytecode of
> java/lang/VersionProps.class to replace the initializion of certain
> vendor specific system properties with custom values. This
> modification currently adds two bytecodes per constant, a pop and
> another ldc. I overhauled it to simply replace the original ldc of the
> old value with another ldc, loading the custom value.

I thought about doing this when I originally wrote that plugin, but it’s
so awkward to achieve with ASM -- as demonstrated by your patch -- that
I concluded it wasn’t worth it.  Who will notice an extra pop in a basic
block that’s only ever executed once?  Is the complexity of this new
code worth the benefit?

- Mark

New candidate JEP: 370: Foreign-Memory Access API

[mark . reinhold]

https://openjdk.java.net/jeps/370

- Mark

Re: RFR: 8234335: Remove line break in class declaration in java.base

[mark . reinhold]

2019/11/19 11:17:22 -0800, john.r.r...@oracle.com:
> On Nov 19, 2019, at 4:00 AM, Lance Andersen  wrote:
>> Seems to be a “your milage varies”.  I am fine with whatever the
>> final decision is.  However, I do believe the comment above reads
>> better and aligns the methods better.
> 
> FWIW, and as author of many of the lines being changed, I prefer that
> comment on a separate from the actual modifiers.  I think the basic
> modifier patterns are easier to recognize quickly when comments and
> annotations are separate.

Personally I prefer to place such pseudo-modifiers on the same line
as the actual modifiers, but I accept that some prefer otherwise.

When in doubt, respect the original author’s choice.

- Mark

Further comments on jpackage (JEP 343)

[mark . reinhold]

Today I took the jpackage tool for another spin, using build
14-jpackage+1-64 from https://jdk.java.net/jpackage.

This looks much better!  The simple command line

  $ jpackage -p lib -m org.openjdk.hello -n hello

did exactly what I expected, and the resulting package installed (and
uninstalled) cleanly on an Ubuntu 18.04 machine.

A few suggestions:

  - The image for the above example is large for such a trivial
application (122MB).  That’s because you invoke the jlink tool with
the `--bind-services` option.  To avoid surprise I suggest that you
not do that by default, and add a `--bind-services` option to
jpackage that’s just passed through to jlink (like `--add-modules`).
That will make jpackage work in the same way as jlink in this regard,
further reducing confusion.

  - The sample usages in the help output are overly focused on
application images.  I suspect that most developers will want to
start by creating actual packages, so consider reordering the
examples to put application packages first, followed by application
images and then runtime images.  You could simplify these first
examples further by removing the `--package-type ` and
describing them as “Generate an application package suitable for
the host system”.

  - Consider renaming the `--package-type` option to `--type`, with a
short-form option `-t`.

- Mark

Re: RFR: JDK-8232806: The LambdaMetaFactory eagerly initializes generated lambdas

[mark . reinhold]

2019/10/28 11:10:25 -0700, vojin.jovano...@oracle.com:
> This email proposes a change to the LambdaMetaFactory that allows to
> disable eager initialization (with Unsafe) of generated lambdas. ...
>
> ...
> 
> After the discussion with Brian Goetz, we have trimmed down to the
> following change:
> 
>   
> https://bugs.openjdk.java.net/secure/attachment/85247/lambda-disable-initialization.diff
> 
> The evolution of this change can be found at the issue: 
> 
>  https://bugs.openjdk.java.net/browse/JDK-8232806

Vojin -- I’d be happy to sponsor this change on your behalf.

I’ve posted a webrev that incorporates the suggestions that Paul and
Rémi made in the JBS issue:

  https://cr.openjdk.java.net/~mr/rev/8232806/

Paul, Rémi -- please make sure that I understood your suggestions
correctly.

- Mark

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[mark . reinhold]

2019/10/23 12:37:56 -0700, bob.vande...@oracle.com:
>> On Oct 23, 2019, at 3:07 PM, mark.reinh...@oracle.com wrote:
>> ...
>> 
>> Addendum: To keep things sane for JFR and the serviceability agent,
>> I had to propagate this change through to those subsystems.
>> 
>> Relative patch below; original webrev updated in place
>> (https://cr.openjdk.java.net/~mr/rev/8232080/).
>> 
>> Okay?
> 
> Looks good.  I’m sure you did this but I scanned the entire JDK
> sources and didn’t see any other uses of JVMFlag:: that would need to
> be updated.

Yep, I did that, and didn’t see anything else either.

Thanks,
- Mark

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[mark . reinhold]

2019/10/22 15:36:28 -0700, mark.reinh...@oracle.com:
> 2019/10/22 12:43:42 -0700, bob.vande...@oracle.com:
>>> On Oct 22, 2019, at 3:22 PM, mark.reinh...@oracle.com wrote:
>>> 2019/10/22 10:31:55 -0700, bob.vande...@oracle.com:
 In arguments.cpp, could you use a new JVMFlag to declare options
 that came from this resource as RESOURCE?
 
 - jint result = parse_each_vm_init_arg(vm_options_args, 
 _mod_javabase, JVMFlag::INTERNAL);
 + jint result = parse_each_vm_init_arg(vm_options_args, 
 _mod_javabase, JVMFlag::RESOURCE);
 
 This will require some minor changes to jvmFlags.hpp
 
 ...
>>> 
>>> Yes, that’d make sense, in which case I’d also change JVMFlag::print_origin
>>> to handle the RESOURCE case (which is easy).
>>> 
>>> Is “RESOURCE” the best name here?  Sounds awfully generic.  How about
>>> “JIMAGE” or “JIMAGE_RESOURCE”?
>> 
>> JIMAGE_RESOURCE or VM_OPTIONS_RESOURCE  works for me.
> 
> JIMAGE_RESOURCE it is, then.  Relative patch below; original webrev
> updated in place (https://cr.openjdk.java.net/~mr/rev/8232080/).

Addendum: To keep things sane for JFR and the serviceability agent,
I had to propagate this change through to those subsystems.

Relative patch below; original webrev updated in place
(https://cr.openjdk.java.net/~mr/rev/8232080/).

Okay?

- Mark



# HG changeset patch
# Parent  d3f05e3b0d76e1a74a10cee180d8953d3045b6c8
Addendum 3 (propagate JIMAGE_RESOURCE): 8232080: jlink plugins for vendor 
information and run-time options

diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp 
b/src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp
--- a/src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp
+++ b/src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp
@@ -134,6 +134,7 @@
 case JVMFlag::ERGONOMIC: return "Ergonomic";
 case JVMFlag::ATTACH_ON_DEMAND: return "Attach on demand";
 case JVMFlag::INTERNAL: return "Internal";
+case JVMFlag::JIMAGE_RESOURCE: return "JImage resource";
 default: ShouldNotReachHere(); return "";
   }
 }
diff --git a/src/hotspot/share/runtime/vmStructs.cpp 
b/src/hotspot/share/runtime/vmStructs.cpp
--- a/src/hotspot/share/runtime/vmStructs.cpp
+++ b/src/hotspot/share/runtime/vmStructs.cpp
@@ -2612,6 +2612,7 @@
   declare_constant(JVMFlag::ERGONOMIC)\
   declare_constant(JVMFlag::ATTACH_ON_DEMAND) \
   declare_constant(JVMFlag::INTERNAL) \
+  declare_constant(JVMFlag::JIMAGE_RESOURCE)  \
   declare_constant(JVMFlag::VALUE_ORIGIN_MASK)\
   declare_constant(JVMFlag::ORIG_COMMAND_LINE)
 
diff --git 
a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Flags.java 
b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Flags.java
--- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Flags.java
+++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Flags.java
@@ -35,7 +35,8 @@
   MANAGEMENT ("Management"),
   ERGONOMIC ("Ergonomic"),
   ATTACH_ON_DEMAND ("Attach on demand"),
-  INTERNAL ("Internal");
+  INTERNAL ("Internal"),
+  JIMAGE_RESOURCE ("JImage");
 
   private final String value;
 
diff --git 
a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java 
b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java
--- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java
+++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java
@@ -114,6 +114,7 @@
   public static intFlags_ERGONOMIC;
   public static intFlags_ATTACH_ON_DEMAND;
   public static intFlags_INTERNAL;
+  public static intFlags_JIMAGE_RESOURCE;
   private static int   Flags_VALUE_ORIGIN_MASK;
   private static int   Flags_ORIG_COMMAND_LINE;
   /** This is only present in a non-core build */
@@ -200,6 +201,8 @@
 return "attach";
 } else if (origin == Flags_INTERNAL) {
 return "internal";
+} else if (origin == Flags_JIMAGE_RESOURCE) {
+return "jimage";
 } else {
 throw new IllegalStateException(
 "Unknown flag origin " + origin + " is detected in " + name);
@@ -484,6 +487,7 @@
 Flags_ERGONOMIC = db.lookupIntConstant("JVMFlag::ERGONOMIC").intValue();
 Flags_ATTACH_ON_DEMAND = 
db.lookupIntConstant("JVMFlag::ATTACH_ON_DEMAND").intValue();
 Flags_INTERNAL = db.lookupIntConstant("JVMFlag::INTERNAL").intValue();
+Flags_JIMAGE_RESOURCE = db.lookupIntConstant("JVMFlag::JIMAGE").intValue();
 Flags_VALUE_ORIGIN_MASK = 
db.lookupIntConstant("JVMFlag::VALUE_ORIGIN_MASK").intValue();
 Flags_ORIG_COMMAND_LINE = 
db.lookupIntConstant("JVMFlag::ORIG_COMMAND_LINE").intValue();
 oopSize  = db.lookupIntConstant("oopSize").intValue();

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[mark . reinhold]

2019/10/22 14:12:18 -0700, mandy.ch...@oracle.com:
> On 10/21/19 8:22 PM, mark.reinh...@oracle.com wrote:
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8232080
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8232753
>> Webrev: https://cr.openjdk.java.net/~mr/rev/8232080/
> 
> Looks good to me. One minor comment:
> 
> AddResourcePlugin isn't really a FILTER, probably best to add a new 
> category for new resource file. jlink hardcodes the order of the 
> categories that determines the order of the plugins to be executed (I 
> see the existing implementation could be cleaned up in the future).  I 
> think we didn't want to the options resource to be filtered by 
> --exclude-resources plugin and so the add-options plugin should run 
> after all filter plugins.

Good point -- that makes sense.

Relative patch below; original webrev updated in place.

Thanks,
- Mark



# HG changeset patch
# Parent  43f160bda3c7a2a7009df91afa37469760d42333
Addendum 2 (jlink ADDER): 8232080: jlink plugins for vendor information and 
run-time options

diff --git 
a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImagePluginConfiguration.java
 
b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImagePluginConfiguration.java
--- 
a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImagePluginConfiguration.java
+++ 
b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImagePluginConfiguration.java
@@ -50,6 +50,7 @@
 
 static {
 CATEGORIES_ORDER.add(Category.FILTER);
+CATEGORIES_ORDER.add(Category.ADDER);
 CATEGORIES_ORDER.add(Category.TRANSFORMER);
 CATEGORIES_ORDER.add(Category.MODULEINFO_TRANSFORMER);
 CATEGORIES_ORDER.add(Category.SORTER);
diff --git 
a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddResourcePlugin.java
 
b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddResourcePlugin.java
--- 
a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddResourcePlugin.java
+++ 
b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddResourcePlugin.java
@@ -58,7 +58,7 @@
 
 @Override
 public Category getType() {
-return Category.FILTER;
+return Category.ADDER;
 }
 
 @Override
diff --git a/src/jdk.jlink/share/classes/jdk/tools/jlink/plugin/Plugin.java 
b/src/jdk.jlink/share/classes/jdk/tools/jlink/plugin/Plugin.java
--- a/src/jdk.jlink/share/classes/jdk/tools/jlink/plugin/Plugin.java
+++ b/src/jdk.jlink/share/classes/jdk/tools/jlink/plugin/Plugin.java
@@ -39,6 +39,7 @@
  * Order of categories matches the plugin sort order.
  * 
  * FILTER: Filter in/out resources or files.
+ * ADDER: Add resources or files.
  * TRANSFORMER: Transform resources or files(eg: refactoring, bytecode
  * manipulation).
  * MODULEINFO_TRANSFORMER: Transform only module-info.class
@@ -52,6 +53,7 @@
  */
 public enum Category {
 FILTER("FILTER"),
+ADDER("ADDER"),
 TRANSFORMER("TRANSFORMER"),
 MODULEINFO_TRANSFORMER("MODULEINFO_TRANSFORMER"),
 SORTER("SORTER"),

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[mark . reinhold]

2019/10/22 12:43:42 -0700, bob.vande...@oracle.com:
>> On Oct 22, 2019, at 3:22 PM, mark.reinh...@oracle.com wrote:
>> 2019/10/22 10:31:55 -0700, bob.vande...@oracle.com:
>>> In arguments.cpp, could you use a new JVMFlag to declare options
>>> that came from this resource as RESOURCE?
>>> 
>>> - jint result = parse_each_vm_init_arg(vm_options_args, 
>>> _mod_javabase, JVMFlag::INTERNAL);
>>> + jint result = parse_each_vm_init_arg(vm_options_args, 
>>> _mod_javabase, JVMFlag::RESOURCE);
>>> 
>>> This will require some minor changes to jvmFlags.hpp
>>> 
>>> ...
>> 
>> Yes, that’d make sense, in which case I’d also change JVMFlag::print_origin
>> to handle the RESOURCE case (which is easy).
>> 
>> Is “RESOURCE” the best name here?  Sounds awfully generic.  How about
>> “JIMAGE” or “JIMAGE_RESOURCE”?
> 
> JIMAGE_RESOURCE or VM_OPTIONS_RESOURCE  works for me.

JIMAGE_RESOURCE it is, then.  Relative patch below; original webrev
updated in place (https://cr.openjdk.java.net/~mr/rev/8232080/).

- Mark




# HG changeset patch
# Parent  efca1844245ad7351418ef41efc86c5055ac3edf
Addendum 1 (JVMFlags): 8232080: jlink plugins for vendor information and 
run-time options

diff --git a/src/hotspot/share/runtime/arguments.cpp 
b/src/hotspot/share/runtime/arguments.cpp
--- a/src/hotspot/share/runtime/arguments.cpp
+++ b/src/hotspot/share/runtime/arguments.cpp
@@ -2203,7 +2203,7 @@
   set_mode_flags(_mixed);
 
   // Parse args structure generated from java.base vm options resource
-  jint result = parse_each_vm_init_arg(vm_options_args, _mod_javabase, 
JVMFlag::INTERNAL);
+  jint result = parse_each_vm_init_arg(vm_options_args, _mod_javabase, 
JVMFlag::JIMAGE_RESOURCE);
   if (result != JNI_OK) {
 return result;
   }
diff --git a/src/hotspot/share/runtime/flags/jvmFlag.cpp 
b/src/hotspot/share/runtime/flags/jvmFlag.cpp
--- a/src/hotspot/share/runtime/flags/jvmFlag.cpp
+++ b/src/hotspot/share/runtime/flags/jvmFlag.cpp
@@ -697,6 +697,8 @@
   st->print("attach"); break;
 case INTERNAL:
   st->print("internal"); break;
+case JIMAGE_RESOURCE:
+  st->print("jimage"); break;
   }
   st->print("}");
 }
diff --git a/src/hotspot/share/runtime/flags/jvmFlag.hpp 
b/src/hotspot/share/runtime/flags/jvmFlag.hpp
--- a/src/hotspot/share/runtime/flags/jvmFlag.hpp
+++ b/src/hotspot/share/runtime/flags/jvmFlag.hpp
@@ -44,8 +44,9 @@
 ERGONOMIC= 5,
 ATTACH_ON_DEMAND = 6,
 INTERNAL = 7,
+JIMAGE_RESOURCE  = 8,
 
-LAST_VALUE_ORIGIN = INTERNAL,
+LAST_VALUE_ORIGIN = JIMAGE_RESOURCE,
 VALUE_ORIGIN_BITS = 4,
 VALUE_ORIGIN_MASK = right_n_bits(VALUE_ORIGIN_BITS),
 

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[mark . reinhold]

2019/10/22 7:15:10 -0700, alan.bate...@oracle.com:
> On 22/10/2019 04:22, mark.reinh...@oracle.com wrote:
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8232080
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8232753
>> Webrev: https://cr.openjdk.java.net/~mr/rev/8232080/
> 
> I've read through the code for the new jlink plugins and the changes to 
> VersionProps and it looks good.
> 
> An alternative for VersionProps would have been to generate a helper 
> class at link time rather than extending the static initilizer to set 
> the fields. That would allow the VENDOR* fields to be final (but I don't 
> think it makes too much difference and it may be a bit more complicated 
> due to having 4 plugins contributing code).

Yes, that would’ve been more complex, and the finality of these fields
isn’t critical.

> The need for a plugin per CLI option may be motivation to explore (in 
> the future) having a single plugin expose multiple options.

Agreed.

Thanks,
- Mark

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[mark . reinhold]

2019/10/22 10:31:55 -0700, bob.vande...@oracle.com:
> In arguments.cpp, could you use a new JVMFlag to declare options that came 
> from this resource as RESOURCE?
> 
> - jint result = parse_each_vm_init_arg(vm_options_args, _mod_javabase, 
> JVMFlag::INTERNAL);
> + jint result = parse_each_vm_init_arg(vm_options_args, _mod_javabase, 
> JVMFlag::RESOURCE);
> 
> This will require some minor changes to jvmFlags.hpp
> 
>   34 struct JVMFlag {
>   35   enum Flags {
>   36 // latest value origin
>   37 DEFAULT  = 0,
>   38 COMMAND_LINE = 1,
>   39 ENVIRON_VAR  = 2,
>   40 CONFIG_FILE  = 3,
>   41 MANAGEMENT   = 4,
>   42 ERGONOMIC= 5,
>   43 ATTACH_ON_DEMAND = 6,
>   44 INTERNAL = 7,
> 
> +  45 RESOURCE = 8,
> 
>   46 
> 
> -  47  LAST_VALUE_ORIGIN = INTERNAL,
>  + 47 LAST_VALUE_ORIGIN = RESOURCE,

Yes, that’d make sense, in which case I’d also change JVMFlag::print_origin
to handle the RESOURCE case (which is easy).

Is “RESOURCE” the best name here?  Sounds awfully generic.  How about
“JIMAGE” or “JIMAGE_RESOURCE”?

- Mark

14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[mark . reinhold]

RFE: https://bugs.openjdk.java.net/browse/JDK-8232080
CSR: https://bugs.openjdk.java.net/browse/JDK-8232753
Webrev: https://cr.openjdk.java.net/~mr/rev/8232080/

This change implements jlink plugins, and associated changes in the VM
and libraries, to support the following new jlink options:

  - --vendor-bug-url= overrides the vendor bug URL baked
into the build.  The value of the system property "java.vendor.url.bug"
will be .

  - --vendor-vm-bug-url= overrides the vendor VM bug
URL baked into the build.  This value will be displayed in VM error
logs.

  - --vendor-version= overrides the vendor version string
baked into the build, if any.  The value of the system property
"java.vendor.version" will be .  This value will be
displayed in the output of java --version.

  - --add-options= prepends the specified  string,
which may include whitespace, before any other options when invoking
the VM in the resulting image.

The vendor-information plugins work by using the JDK’s internal ASM
library to redefine static fields in the java.lang.VersionProps class.
The VM reads the vendor-version and vendor-vm-bug-url strings from that
class during startup.

The add-options plugin works by storing the requested options in an
internal resource, /java.base/jdk/internal/vm/options.  The VM loads that
resource from the lib/modules jimage file during startup and prepends any
options found there to those given on the command line.

Passes tier1-3 and JCK on {linux,macos,windows}-x64.

Thanks,
- Mark

Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.

[mark . reinhold]

2019/10/2 5:45:10 -0700, goetz.lindenma...@sap.com:
> thanks for looking at my change! Can I add you as reviewer?

Sure.

>> This is very nice work!  I especially appreciate the thorough tests.
> 
> Thanks!  But adapting the many tests to changed messages is 
> quite cumbersome :) Thanks for supplying the patch right away!

Nothing that a little Emacs-fu can’t handle.

>> ...
>> 
>> I also noticed that the generated messages use single quotes (‘'’) to
>> quote the names of fields, etc., rather than double quotes (‘"’).
> 
> I'm fine with this, but I think hotspot uses "'" for citing code
> quite consistently. E.g., have a look at 
> http://hg.openjdk.java.net/jdk/jdk/file/b25362cec8ce/src/hotspot/share/interpreter/linkResolver.cpp
> This can easily be changed.  I could do a separate change 
> for hotspot and change all uses of ' in exceptions to ". What 
> do you think?

I think that’d be a fine clean-up task, for later.

- Mark

Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.

[mark . reinhold]

2019/9/24 1:13:14 -0700, goetz.lindenma...@sap.com:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/19/

This is very nice work!  I especially appreciate the thorough tests.

Looking at the tests, and the details of the JEP, I noticed that the
generated messages all end in a period (e.g., “... is null.”).  This
is pretty unusual in the JDK and jarring to the eye, except in the
rare cases in which an exception message is composed of multiple
distinct sentences.  I’m surprised that no one noticed this before.

As a data point, of the 17,999 messages that I was able to find in
invocations of exception constructors in the JDK 14 library code just
now, only 998 of them end in periods.  (I didn’t look at HotSpot, which
is trickier to grep for such things.)

So, I suggest you drop the trailing periods.

I also noticed that the generated messages use single quotes (‘'’) to
quote the names of fields, etc., rather than double quotes (‘"’).

On this choice, our corpus is less instructive: Of the 17,999 messages
I gathered, 446 of them use quotes of some sort, 227 of those use double
quotes, and 219 use single quotes.

Even so, I’ll propose here that double quotes are preferable: They’re
consistent with the Java language itself, they’re consistent with common
usage in both American and British English, and they’re less apt to be
confused with single quotes used as apostrophes (e.g., "MemoryHandler
can't load handler target \"" + targetName + "\"").

So, I also suggest that you change the single quotes to double quotes.

Fortunately, and because you have such good tests, this is pretty easy
to do.  Attached is a patch against your patch that makes these changes,
and after which all the tests still pass.

- Mark
# HG changeset patch
# User mr
# Date 1569968713 25200
#  Tue Oct 01 15:25:13 2019 -0700
# Node ID d7d49b8c3c44c527c1b60a590228259c02377738
# Parent  06c12a39584c579e9b9cd9d6bbba01a88b9ff764
Remove periods, and double quotes, for 8218628

diff --git a/src/hotspot/share/interpreter/bytecodeUtils.cpp b/src/hotspot/share/interpreter/bytecodeUtils.cpp
--- a/src/hotspot/share/interpreter/bytecodeUtils.cpp
+++ b/src/hotspot/share/interpreter/bytecodeUtils.cpp
@@ -1168,8 +1168,8 @@
 }
 
 bool ExceptionMessageBuilder::print_NPE_cause(outputStream* os, int bci, int slot) {
-  if (print_NPE_cause0(os, bci, slot, _max_cause_detail, false, " because '")) {
-os->print("' is null.");
+  if (print_NPE_cause0(os, bci, slot, _max_cause_detail, false, " because \"")) {
+os->print("\" is null");
 return true;
   }
   return false;
@@ -1346,7 +1346,7 @@
 case Bytecodes::_invokeinterface: {
   int cp_index = Bytes::get_native_u2(code_base + pos) DEBUG_ONLY(+ ConstantPool::CPCACHE_INDEX_TAG);
   if (max_detail == _max_cause_detail && !inner_expr) {
-os->print(" because the return value of '");
+os->print(" because the return value of \"");
   }
   print_method_name(os, _method, cp_index);
   return true;
@@ -1417,19 +1417,19 @@
 int name_and_type_index = cp->name_and_type_ref_index_at(cp_index);
 int name_index = cp->name_ref_index_at(name_and_type_index);
 Symbol* name = cp->symbol_at(name_index);
-os->print("Cannot read field '%s'", name->as_C_string());
+os->print("Cannot read field \"%s\"", name->as_C_string());
   } break;
 case Bytecodes::_putfield: {
 int cp_index = Bytes::get_native_u2(code_base + pos) DEBUG_ONLY(+ ConstantPool::CPCACHE_INDEX_TAG);
-os->print("Cannot assign field '%s'", get_field_name(_method, cp_index));
+os->print("Cannot assign field \"%s\"", get_field_name(_method, cp_index));
   } break;
 case Bytecodes::_invokevirtual:
 case Bytecodes::_invokespecial:
 case Bytecodes::_invokeinterface: {
 int cp_index = Bytes::get_native_u2(code_base+ pos) DEBUG_ONLY(+ ConstantPool::CPCACHE_INDEX_TAG);
-os->print("Cannot invoke '");
+os->print("Cannot invoke \"");
 print_method_name(os, _method, cp_index);
-os->print("'");
+os->print("\"");
   } break;
 
 default:
@@ -1474,7 +1474,6 @@
 if (!emb.print_NPE_cause(ss, bci, slot)) {
   // Nothing was printed. End the sentence without the 'because'
   // subordinate sentence.
-  ss->print(".");
 }
   }
   return true;
diff --git a/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPEInHiddenTopFrameTest.java b/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPEInHiddenTopFrameTest.java
--- a/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPEInHiddenTopFrameTest.java
+++ b/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPEInHiddenTopFrameTest.java
@@ -77,7 +77,7 @@
  null :
  // This is the correct message, but it describes code generated on-the-fly.
  // You get it if you disable hiding frames (-XX:+ShowHiddenframes).
-   

Re: Formatting rules for exception messages

[mark . reinhold]

2019/9/27 6:23:54 -0700, Florian Weimer :
> * mark reinhold:
> 
>> 2019/3/25 5:24:37 -0700, Florian Weimer :
>>> Are there any guidelines for formatting exception messages?
>>> 
>>> In particular, I'm interested in the case when the exception message
>>> is a (clipped) sentence.  Is it supposed to start with a capital
>>> letter?
>>> 
>>> If the message refers to a parameter name, the spelling should reflect
>>> the name exactly, of course.  There seems to be a slight bias towards
>>> capitalization, based on a few greps.  ...
>> 
>> The first word of any exception message in code that I’ve written, or
>> reviewed, is always capitalized unless that word conveys case-sensitive
>> technical information (e.g., a parameter name, as you mentioned).  This
>> improves readability, especially in longer exception messages that
>> contain additional punctuation characters.
> 
> Thank you for confirming my observation.  Would it make sense to have
> these rules documented somewhere?

Yes, it would.  Perhaps an informational JEP -- I’ll add it to my queue.

- Mark

Re: CharsetEncoder.maxBytesPerChar()

[mark . reinhold]

2019/9/24 13:00:21 -0700, ulf.zi...@cosoco.de:
> Am 21.09.19 um 00:03 schrieb mark.reinh...@oracle.com:
>> To avoid this confusion, a more verbose specification might read:
>> * Returns the maximum number of $otype$s that will be produced for each
>> * $itype$ of input.  This value may be used to compute the worst-case 
>> size
>> * of the output buffer required for a given input sequence. This value
>> * accounts for any necessary content-independent prefix or suffix
>> #if[encoder]
>> * $otype$s, such as byte-order marks.
>> #end[encoder]
>> #if[decoder]
>> * $otype$s.
>> #end[decoder]
> 
> wouldn't it be more clear to use "char" or even "{@code char}" instead
> "character" as replacment for the $xtype$ parameters?

The specifications of the Charset{De,En}coder classes make it clear
up front that “character” means “sixteen-bit Unicode character,” so
I don’t think changing “character” everywhere to “{@code char}” is
necessary.

This usage of “character” is common throughout the API specification.
With the introduction of 32-bit Unicode characters we started calling
those “code points,” but kept on calling sixteen-bit characters just
“characters.”  (I don’t think the official term “Unicode code unit”
ever caught on, and it’s a bit of a mouthful anyway.)

- Mark

Re: Thread.suspend/resume - any reason not to deprecate forRemoval=true

[mark . reinhold]

2019/9/24 8:04:55 -0700, alan.bate...@oracle.com:
> Thread.suspend/resume (and the corresponding methods in ThreadGroup) 
> have been deprecated since 1.2 (1998). I haven't see anything use these 
> methods in many years. Would anyone care if their deprecation is changed 
> to forRemoval=true with a view to really removing them in the future? 
> Just to clear, I'm only asking about the Thread/ThreadGroups APIs, we 
> have to keep suspend/resume support in the debugger and tool APIs.

Go for it!

- Mark

Re: CharsetEncoder.maxBytesPerChar()

[mark . reinhold]

2019/9/20 13:25:38 -0700, naoto.s...@oracle.com:
> I am looking at the following bug:
> 
> https://bugs.openjdk.java.net/browse/JDK-8230531
> 
> and hoping someone who is familiar with the encoder will clear things 
> out. As in the bug report, the method description reads:
> 
> --
> Returns the maximum number of bytes that will be produced for each 
> character of input. This value may be used to compute the worst-case 
> size of the output buffer required for a given input sequence.
> --
> 
> Initially I thought it would return the maximum number of encoded bytes 
> for an arbitrary input "char" value, i.e. a code unit of UTF-16 
> encoding. For example, any UTF-16 Charset (UTF-16, UTF-16BE, and 
> UTF-16LE) would return 2 from the method, as the code unit is a 16 bit 
> value. In reality, the encoder of UTF-16 Charset returns 4, which 
> accounts for the initial byte-order mark (2 bytes for a code unit, plus 
> size of the BOM).

Exactly.  A comment in the implementation, in sun.nio.cs.UnicodeEncoder,
mentions this (perhaps you already saw it):

protected UnicodeEncoder(Charset cs, int bo, boolean m) {
super(cs, 2.0f,
* // Four bytes max if you need a BOM
* m ? 4.0f : 2.0f,
  // Replacement depends upon byte order
  ((bo == BIG)
   ? new byte[] { (byte)0xff, (byte)0xfd }
   : new byte[] { (byte)0xfd, (byte)0xff }));
usesMark = needsMark = m;
byteOrder = bo;
}

>   This is justifiable because it is meant to be the 
> worst case scenario, though. I believe this implementation has been 
> there since the inception of java.nio, i.e., JDK 1.4.

Yes, it has.

> Obviously I can clarify the spec of maxBytesPerChar() to account for the 
> conversion independent prefix (or suffix) bytes, such as BOM, but I am 
> not sure the initial intent of the method. If it intends to return pure 
> max bytes for a single input char, UTF-16 should also have been 
> returning 2. But in that case, caller would not be able to calculate the 
> worst case byte buffer size as in the bug report.

The original intent is that the return value of this method can be used
to allocate a buffer that is guaranteed to be large enough for any
possible output.  Returning 2 for UTF-16 would, as you observe, not work
for that purpose.

To avoid this confusion, a more verbose specification might read:

 * Returns the maximum number of $otype$s that will be produced for each
 * $itype$ of input.  This value may be used to compute the worst-case size
 * of the output buffer required for a given input sequence. This value
 * accounts for any necessary content-independent prefix or suffix
#if[encoder]
 * $otype$s, such as byte-order marks.
#end[encoder]
#if[decoder]
 * $otype$s.
#end[decoder]

(The example of byte-order marks applies only to CharsetEncoders, so
 I’ve conditionalized that text for Charset-X-Coder.java.template.)

- Mark

Re: Comments on jpackage (JEP 343)

[mark . reinhold]

jlink’s -o/--output option names exactly the thing being output,
rather than a container for the thing being output.

The jpackage option we’re discussing here names a container for the
thing being output, much like the -d option of javac and javadoc.

Using -d/--dest for jpackage is consistent with the JDK’s other
command-line tools.

- Mark

Comments on jpackage (JEP 343)

[mark . reinhold]

I spent some time last week playing with the jpackage tool, using build
14-jpackage+1-35 from https://jdk.java.net/jpackage.

Overall, I can see that you’ve made good progress, but there’s still some
work to be done.  I’ll start with high-level comments and questions, and
then comment on my experience using the tool on Debian and then macOS.

High-level comments/questions

  - It’s good that you’re publishing EA builds, but I haven’t seen very
much feedback from those builds.  It may be better to propose this as
an experimental feature when it’s ready to target.  That would give
you the freedom to improve it incompatibly over one or two release
cycles before you commit to a final version.

  - The tool still generates an image by default, rather than a package.
This will surprise many users, especially those who just want to do
something simple and straightforward.  The least-surprising default
behavior would be to generate a package of the type most suitable for
the current platform.  An option to generate just an image would be
fine, for those who want to tweak it before the actual packaging
step, but that shouldn’t be the default.

  - Related to the previous point, I should only have to specify the
`--package-type` option if I want something other than the default
for the current platform.

  - The tool assumes that the application being packaged will have a GUI.
This isn’t surprising, considering its heritage, but as a consequence
it always produces packages that perform GUI-specific actions, such
as installing icons and desktop-menu entries.  If I’m just packaging
a command-line tool then these are unnecessary, and supporting them
can pull in lots of additional dependencies (e.g., a ton of Perl
scripts on Debian).  Consider adding an option (`--gui`?) so that
the user can indicate when an application is to be installed for
graphical use.

  - The `--output`/`-o` option is confusing.  It doesn’t name the output
itself, but rather a directory into which the single item of output
will be placed.  Typing `-o .` all the time is just annoying.  It’d
be more logical to rename this option to `--dest`/`-d` and to make it
optional, with a default value of `.`.

  - If my app is modular, and my main module has a version string, then
it’d be nice for that to be used as the package version unless a
specific version is specified on the command line.

  - Terminology: For Linux we generally speak of “packages” rather than
“bundles.”  Rename `--linux-bundle-name` to `--linux-package-name`.

  - The `--temp-root` option could be shortened to `--temp`.

  - Periods at the ends of error messages are unsightly and unnecessary.
We don’t use them in other JDK tools.  Please remove them.

  - It’d be nice to have an option that displays the programs being run
by the tool, in a form that can easily be cut-and-pasted into a
script for those who need to do a lot of customization.  The current
verbose output shows (at least some of) this information, but in a
difficult-to-use format.

  - What’s the rationale for copying the entire “input” directory as-is?
Why is its structure important?  Couldn’t you just as well limit this
to a single directory full of JAR files?

Comments on Debian packaging

  - On a Debian machine I tried to create a package for a trivial,
two-module application using the command

  $ jpackage -o . --package-type deb -p lib -m org.openjdk.hello -n hello

which gave me the error message

   Bundler DEB Installer skipped because of a configuration problem: 
java.lang.NullPointerException.

(Side note: This message is confusing since the tool is creating a
 Debian package, not an “installer.”)

  - On a hunch, I specified a main application class:

  $ jpackage -o . --package-type deb -p lib -m 
org.openjdk.hello/org.openjdk.hello.Main -n hello

and that created `hello_1.0-1_amd64.deb`.  It shouldn’t have been
necessary to specify a main class since the main module does have a
`ModuleMainClass` attribute in its module descriptor.

  - The resulting package does not depend upon any others, i.e., the
`Depends:` line in its control file is empty.  This can’t possibly be
right, since the embedded JDK depends on many system libraries for
proper operation (`libc`, `libfreetype`, `libpthread`, etc.).

  - The resulting package would install into `/opt/hello`, as expected,
but the `/opt/hello/bin` directory would contain not just the `hello`
application launcher but also `hello.desktop`, `hello.png`, and
`libapplauncher.so`.  These aren’t appropriate for a `bin` directory
and should be placed elsewhere, most likely `/opt/hello/lib`.

  - The resulting package would install `/opt/hello/app` and
`/opt/hello/runtime` directories.  These are not strictly forbidden
by the Linux FHS [1], but it’d be better to put 

New candidate JEP: 358: Helpful NullPointerExceptions

[mark . reinhold]

https://openjdk.java.net/jeps/358

- Mark

Re: New candidate JEP: 356: Enhanced Pseudo-Random Number Generators

[mark . reinhold]

2019/6/21 13:22:17 -0700, guy.ste...@oracle.com:
> On Jun 21, 2019, at 2:36 PM, mark.reinh...@oracle.com wrote:
>> By my count this JEP would add ten new public types to the java.util
>> package.  Is it time to consider creating a java.util.random subpackage?
> 
> Sure, that would be a completely reasonable move. Who should make that
> decision?

I suggest that you propose it, as part of this JEP.

It might make sense to put the key interface(s) directly in java.util,
with static factories for convenience.  Up to you.

- Mark

Re: New candidate JEP: 356: Enhanced Pseudo-Random Number Generators

[mark . reinhold]

By my count this JEP would add ten new public types to the java.util
package.  Is it time to consider creating a java.util.random subpackage?

- Mark

New candidate JEP: 356: Enhanced Pseudo-Random Number Generators

[mark . reinhold]

https://openjdk.java.net/jeps/356

- Mark

Re: JEP 343: Packaging Tool

[mark . reinhold]

2019/6/5 16:09:11 -0700, Alan Snyder :
> I haven’t used recent versions of this tool, but I have found it
> essential to be able to modify the image before the final package is
> created. There is no way that this tool can anticipate all of the
> custom configuration that might be needed, and I do not want to
> duplicate its ability to create an image and to build a package from
> the image.

That’s a fair point.

It does suggest, however, that perhaps this should be two separate
tools -- one to create an OS-specific image, and another to create
an OS-specific package.  They should work well together, of course,
but it’s not clear that they need to be merged into a single tool.

- Mark

Re: 13 RFR (XXS) 8197927: Allow the system property `java.vendor.version` to be undefined

[mark . reinhold]

Lance, Mandy, Christoph -- thanks for the reviews!

- Mark

13 RFR (XXS) 8197927: Allow the system property `java.vendor.version` to be undefined

[mark . reinhold]

Bug: https://bugs.openjdk.java.net/browse/JDK-8197927
CSR: https://bugs.openjdk.java.net/browse/JDK-8225381

Revise the specification of the `java.lang.System::getProperties` method
to allow the values of specific system properties to be undefined, and
to allow the system property `java.vendor.version` to be undefined.

Javadoc: 
https://cr.openjdk.java.net/~mr/rev/8179727/java.base/java/lang/System.html#getProperties%28%29

Could I please have reviews for both the patch (below), and the CSR?

Thanks,
- Mark



diff --git a/src/java.base/share/classes/java/lang/System.java 
b/src/java.base/share/classes/java/lang/System.java
--- a/src/java.base/share/classes/java/lang/System.java
+++ b/src/java.base/share/classes/java/lang/System.java
@@ -618,8 +618,9 @@
  * {@link #getProperty(String)} method is returned as a
  * {@code Properties} object. If there is no current set of
  * system properties, a set of system properties is first created and
- * initialized. This set of system properties always includes values
- * for the following keys:
+ * initialized. This set of system properties includes a value
+ * for each of the following keys unless the description of the associated
+ * value indicates that the value is optional:
  * 
  * Shows property keys and associated 
values
  * 
@@ -639,7 +640,7 @@
  * {@systemProperty java.vendor.url}
  * Java vendor URL
  * {@systemProperty java.vendor.version}
- * Java vendor version
+ * Java vendor version (optional) 
  * {@systemProperty java.home}
  * Java installation directory
  * {@systemProperty java.vm.specification.version}

JEP 343: Packaging Tool

[mark . reinhold]

I saw that you moved JEP 343 to “Proposed to Target,” so I spent a
couple of hours looking at it.  You’ve made good progress but I don’t
think this is in the “nearly finished” state that we ask of features
in the six-month cadence.  I suggest that you move this JEP back to
Candidate for now and continue refining it.  The next feature release,
JDK 14, is just six months away.

Some specific observations and suggestions:

  - At the moment there are 75 open issues in JBS [1], 69 of which are
P3 or higher.  JDK 13 will enter RDP 1 next week, so there’s not
much time to make progress on all those issues.

  - The use of the term “installer” in the JEP and in the command-line
options is confusing.  The tool only creates installers on Windows
and macOS (pkg); the other formats that it supports (macOS dmg,
Linux deb/rpm) are OS-specific packages rather than interactive
installers.  Consider replacing the term “installer” with “package”
throughout.  This would also align better with the name of the tool
itself.

  - It’s not clear why there are distinct subcommands to create an image
vs. to create an OS-specific package.  Given the name of the tool,
I’d expect creating a package to be the primary behavior.  An option
to preserve the image, which is just a temporary result, could make
sense, as well as another option to skip the creation of the
package, but I don’t understand the need for subcommands.

  - On a Debian machine I tried to create a package from a trivial,
two-module application using the command

  $ jpackage create-installer -o /tmp -p lib -m org.openjdk.hello -n hello

This terminated with exit code 255 and an error message.  In Linux,
and Unix/POSIX generally, an exit code of 255 means that the exit
status was out of range.  I suggest you exit with the value 1 on
errors, at least on Linux.

  - The error message from the above command was:

  Bundler RPM Bundle skipped because of a configuration problem: Can not 
find rpmbuild 4 or newer..
  Advice to fix: Install packages needed to build RPM, version 4 or newer.

I’m on a Debian machine, trying to create an OS-specific (i.e.,
Debian) image, so this was a confusing message.  (Yes, I know it’s
possible to create rpms under Debian if you have the right tools
installed, but that’s not what I was trying to do here.)

  - What’s more, even though the tool exited on error it still produced
a Debian package in the output directory, but I found it only by
accident.

  - Looking at the content of the generated Debian package, the control
file has many fields that don’t have corresponding jpackage options.
That could be a problem for some developers.

  - The data in the Debian package would place the application into a
directory named `/usr/bin/hello`, which is completely wrong.  Please
see the Filesystem Hierarchy Standard [2] and the Debian Policy
Manual [3] for details.

  - I tried to create a package that would install into the `/opt`
directory by appending `--install-dir /opt` to the above command
line.  The data in the resulting package would indeed install into
`/opt`, but the structure within that directory would be incorrect.
There should be an `/opt/hello/bin` directory containing the `hello`
launcher, and the remainder of the content should be organized per
the usual conventions [4].

To get this into better shape I suggest that you seek advice and, when
appropriate, reviews from developers who have deep experience with the
rpm and deb package formats (there are several such people here in the
OpenJDK Community).  It’d also be good to get feedback from macOS and
Windows packaging experts, but I don’t personally know of any.

- Mark


[1] 
https://bugs.openjdk.java.net/issues/?jql=project%20in%20(JDK)%20AND%20component%20in%20(tools)%20AND%20Subcomponent%20in%20(jpackage)%20and%20statuscategory%20not%20in%20(Done)
[2] http://www.pathname.com/fhs/pub/fhs-2.3.html#USRBINMOSTUSERCOMMANDS
[3] https://www.debian.org/doc/debian-policy/index.html
[4] 
http://www.pathname.com/fhs/pub/fhs-2.3.html#OPTADDONAPPLICATIONSOFTWAREPACKAGES

New candidate JEP: 353: Reimplement the Legacy Socket API

[mark . reinhold]

https://openjdk.java.net/jeps/353

- Mark

New candidate JEP: 352: Non-Volatile Mapped Byte Buffers

[mark . reinhold]

https://openjdk.java.net/jeps/352

- Mark

Re: Formatting rules for exception messages

[mark . reinhold]

2019/3/25 5:24:37 -0700, Florian Weimer :
> Are there any guidelines for formatting exception messages?
> 
> In particular, I'm interested in the case when the exception message
> is a (clipped) sentence.  Is it supposed to start with a capital
> letter?
> 
> If the message refers to a parameter name, the spelling should reflect
> the name exactly, of course.  There seems to be a slight bias towards
> capitalization, based on a few greps.  ...

The first word of any exception message in code that I’ve written, or
reviewed, is always capitalized unless that word conveys case-sensitive
technical information (e.g., a parameter name, as you mentioned).  This
improves readability, especially in longer exception messages that
contain additional punctuation characters.

- Mark

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[mark . reinhold]

2019/3/14 8:00:20 -0700, maurizio.cimadam...@oracle.com:
> I second what Mandy says.
> 
> First let me start by saying that this enhancement will be a great 
> addition to our platform; back in the days when I was teaching some Java 
> classes at the university, I was very aware of how hard it is to 
> diagnose a NPE for someone novel to Java programming.

Agreed!

> ...
> 
> I also think that the design space for such an enhancement is non 
> trivial, and would best be explored (and captured!) in a medium that is 
> something other than a patch. ...

Agreed, also.

Goetz -- if, per Mandy’s suggestion, you’re going to write something
up using the JEP template, might I suggest that you then submit it as
an actual JEP?  Giving visibility to, and recording, such design-space
explorations is one of the primary goals of the JEP process.

- Mark

Re: 12 RFR (S): 8216532: tools/launcher/Test7029048.java fails (Solaris)

[mark . reinhold]

2019/1/16 16:21:17 -0800, joe.da...@oracle.com:
> Rather than naming the enum for the test cases as "CASE", more 
> conventional naming would be "TestCase" or "Case" for the type itself.

Good point.  The ALL_CAPS treatment was a holdover from the original.
Changed to TestCase.

- Mark

Re: 12 RFR (S): 8216532: tools/launcher/Test7029048.java fails (Solaris)

[mark . reinhold]

2019/1/16 11:45:05 -0800, roger.ri...@oracle.com:
> Looks fine.

Thanks!

> Bump the Copyright to 2019.

Will do.

- Mark

12 RFR (S): 8216532: tools/launcher/Test7029048.java fails (Solaris)

[mark . reinhold]

Bug: https://bugs.openjdk.java.net/browse/JDK-8216532
Webrev: https://cr.openjdk.java.net/~mr/rev/8216532/

The fix for 8210669 [1] enabled some tests to run that had not been
running, and perhaps had never run.

Amongst these tests was tools/launcher/Test7029048.java, which is meant
to ensure that the Java launcher defends itself against interfering
settings of the LD_LIBRARY_PATH environment variable, as implemented in
the fix for 7029048 [2].

It took me quite a while to understand exactly what this test was doing,
but in the end the root cause was that the test could not distinguish
between an LD_LIBRARY_PATH value that’s empty from one that contains
just a single path.  This is important on Solaris, where the test also
checks that setting LD_LIBRARY_PATH_64 does not interfere with the
launcher.  In this context the environment variable can contain either
no path or just one path, depending upon which case is being tested.

I fixed this accounting problem and, while I was at it, made a few other
changes to clarify the test for future maintainers.

In a cruel twist of fate, it turns out that the behavior implemented for
7029048, and verified by this test, is incorrect.  On Solaris, if you
set LD_LIBRARY_PATH_64 then that completely overrides any setting of
LD_LIBRARY_PATH.  The launcher tries to defend itself against a setting
of LD_LIBRARY_PATH_64 by setting LD_LIBRARY_PATH, but that will have no
effect -- it should really set LD_LIBRARY_PATH_64.  I’ve filed 8217216
[3] to record this.

Passes tiers 1-4 on {linux,macosx,windows}-x64 and solaris-sparcv9.

Thanks,
- Mark


[1] https://bugs.openjdk.java.net/browse/JDK-8210669
[2] https://bugs.openjdk.java.net/browse/JDK-7029048
[3] https://bugs.openjdk.java.net/browse/JDK-8217216

12 RFR (XS) 8215301: Module-summary page is unreadably wide

[mark . reinhold]

Bug: https://bugs.openjdk.java.net/browse/JDK-8215301
Webrev: https://cr.openjdk.java.net/~mr/rev/8215301/

The `make generate-summary` target produces a handy tabular summary
of all of the modules in a JDK build into
`$BUILD/images/gengraphs/module-summary.html`. Several JDK modules
(e.g., `java.desktop` and `jdk.internal.vm.compiler`) include many
providers for a particular service, and the providers are all listed
in a single line, resulting in a very wide HTML page that’s difficult
to read even on a large screen.

Output before: 
https://cr.openjdk.java.net/~mr/rev/8215301/module-summary-old.html
After: https://cr.openjdk.java.net/~mr/rev/8215301/module-summary-new.html

Thanks,
- Mark

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

[mark . reinhold]

2018/12/11 16:38:32 -0800, stuart.ma...@oracle.com:
> Adam,
> 
> Starting from your patch, I've removed changes relating to "crap" and "damn" 
> and 
> the changes to upstream jszip.js. This leaves the patches appended below. The 
> SoftChannel.java change is most likely a typo that should be fixed. The 
> BitArray.java change is part of Xalan. We don't actually keep our sources in 
> sync with Xalan, but I note that upstream [7] has made the same change, so we 
> might as well change it too.
> 
> ...

Thanks for the enlightening historical analysis, and for wrangling
this patch.

> ...
> 
> # HG changeset patch
> # User afarley
> # Date 1544574289 28800
> #  Tue Dec 11 16:24:49 2018 -0800
> # Node ID 0c40c78b6d139eb05b0718d0b524a465419ee9cb
> # Parent  b75a44aad06cd93c823159265a1f200bf0ce390c
> 8215217: OpenJDK Source Has Too Many Swear Words

Nit: Please use sentence case in issue summaries (“OpenJDK source has
too many swear words”) rather than title case.  This is just a patch,
not a novel.

- Mark

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

[mark . reinhold]

2018/12/11 7:03:57 -0800, adam.far...@uk.ibm.com:
> I've spotted 12 instances of swear words in OpenJDK source comments, and 
> it seems appropriate to remove them.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215217

(webrev: http://cr.openjdk.java.net/~afarley/8215217/webrev/)

> I've created a webrev and attached to the bug.
> 
> Also, I've mentioned in the bug that there are additional swears in more 
> excusable locations. It would be good to get the community's take on 
> those.

It also would be good to discuss the instances that you’ve proposed
to change in your patch.

I can certainly see removing the f-word, and other words of a sexual
nature.  Those are clearly inappropriate.

Removing lesser words, and continuing to police their use henceforth,
strikes me as overkill.

What do other Committers think?

- Mark

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[mark . reinhold]

2018/10/3 12:13:15 -0700, kim.barr...@oracle.com:
> ...
> 
> https://bugs.openjdk.java.net/browse/JDK-8208089

Or, more readably: https://openjdk.java.net/jeps/8208089

- Mark

Re: 8207690: Parsing API for classpath and similar path strings

[mark . reinhold]

2018/9/13 10:21:26 -0700, roger.ri...@oracle.com:
> Consider:
> 
> public class SearchPath {
>   public static SearchPath of(String searchPath) {...}
> public static SearchPath of(List elements) {...}
> public Stream stream() {...}
> public List asList() {...}
>   public String toString() {...}
>   private SearchPath() {};
> }
> 
> A SearchPath can be constructed from various forms of search path 
> elements and can create other forms as needed.
> As a class it would be extensible and can start small and grow as needed.
> 
> Examples:
>List list = SearchPath.of("a:b:c").asList();
> 
>  Path p = SearchPath.of("x:y:z").stream()
> .filter(Predicate.not(String::isEmpty))
> .map(Path::of)
> .filter(Files::isDirectory)
>   .filter(q -> Files.exists(q.resolve("x.jar)))
> .findFirst()
> .orElseThrow();
> 
> If that seems like a reasonable base, I (or some other volunteer) can 
> flesh it out with the suggestions.

Yes -- this is along the lines of what I was trying to suggest.
Stick to the goal of representing a search path, and leave actual
I/O operations, if any, to the caller.

I think that puts this class squarly in the realm of java.util
rather than java.nio.file.

- Mark

New candidate JEP: 343: Packaging Tool

[mark . reinhold]

http://openjdk.java.net/jeps/343

- Mark

Re: RFR 8210670 (S): Accept double-dash VM-name options at launch time

[mark . reinhold]

2018/9/12 14:16:00 -0700, mark.reinh...@oracle.com:
> ...
> 
> 8210670: Accept double-dash VM-name options at launch time
> 
>   Per JEP 293 (Guidelines for JDK Command-Line Tool Options) [2], enhance
>   the Java launcher so that `--server` is accepted to select the server VM,
>   in addition to `-server`, and likewise for any other VMs listed in the
>   $JDK/lib/jvm.cfg file.
> 
>   JBS: https://bugs.openjdk.java.net/browse/JDK-8210670
>   Webrev: http://cr.openjdk.java.net/~mr/rev/8210670/
> 
> Passes tier1 tests; tier2-3 in progress.

Addendum: Joe reminded me that this change should have a CSR, so here it
is: https://bugs.openjdk.java.net/browse/JDK-8210682 .  A review of that
too would be appreciated.

I’ve also revised the code for this change, and hence the webrev, to
show the double-dash VM-name options in the `java --help` output.

- Mark

RFR 8210670 (S): Accept double-dash VM-name options at launch time

[mark . reinhold]

To make this trivial enhancement I first had to fix some broken tests,
so there are two issues and two webrevs here.

8210669: Some launcher tests assume a pre-JDK 9 run-time image layout

  Two launcher tests, ExecutionEnvironment.java and Test7029048.java (in
  test/jdk/tools/launcher), still assume the old image layout in which a
  HotSpot shared library (libjava.so) is found in $JDK/lib/$ARCH/$VMTYPE
  on Linux and Solaris.  The intermediate $ARCH directory was removed in
  JDK 9, as part of JEP 220 (Modular Run-Time Images) [1].  Since that
  time these tests have been succeeding anyway, because they don’t fail
  when the requested VM is not found.

  The tests should be fixed to fail when the requested VM is not found,
  and to look for VM shared libraries in the proper location.

  JBS: https://bugs.openjdk.java.net/browse/JDK-8210669
  Webrev: http://cr.openjdk.java.net/~mr/rev/8210669/

8210670: Accept double-dash VM-name options at launch time

  Per JEP 293 (Guidelines for JDK Command-Line Tool Options) [2], enhance
  the Java launcher so that `--server` is accepted to select the server VM,
  in addition to `-server`, and likewise for any other VMs listed in the
  $JDK/lib/jvm.cfg file.

  JBS: https://bugs.openjdk.java.net/browse/JDK-8210670
  Webrev: http://cr.openjdk.java.net/~mr/rev/8210670/

Passes tier1 tests; tier2-3 in progress.

Thanks,
- Mark


[1] http://openjdk.java.net/jeps/220
[2] http://openjdk.java.net/jeps/293

Re: 8207690: Parsing API for classpath and similar path strings

[mark . reinhold]

2018/9/11 4:50:49 -0700, chris.hega...@oracle.com:
>> On 11 Sep 2018, at 10:24, Alan Bateman  wrote:
>> On 10/09/2018 21:55, Roger Riggs wrote:
>>> Nope! there's quoting on Windows that gets short changed with that work 
>>> around.
>>> 
>>> Other opinions?, Suggestions?
>> 
>> One suggestion is reduce this down to one method that returns a
>> stream rather than a collection. It could work lazily if you
>> want. Something like:
>> 
>>  Stream splitSearchPath(String input)
> 
> I agree with Alan, this seems like the lowest-order primitive.

Agreed, but it begs a question that applies to all these alternatives:
Why does this API belong in java.nio.file.Paths?

In other words, search paths are a different abstraction from filesystem
paths.  Should they have their own class, even if it only winds up with
a couple of static utility methods?  java.nio.file.SearchPath?

public class SearchPath {
public static Stream splitToStream(String searchPath);
public static List splitToPaths(String searchPath);
public static String join(Collection);
}

- Mark

Re: 8207690: Parsing API for classpath and similar path strings

[mark . reinhold]

2018/9/11 8:05:23 -0700, roger.ri...@oracle.com:
> Right, that is a description of the shell environment on Windows and how 
> the java launcher behaves.
> On Windows the launcher expands wildcards when evaluating classpath from 
> the environment
> and the command line args. After they are expanded the java.class.path 
> property is set.

Class-path wildcards are expanded by the launcher and the compiler, on
all platforms, to include all files in a specific directory that end in
`.jar` or `.JAR`.  In most shells you need to quote them so that the
shell doesn’t expand them, e.g., `--class-path lib/\*:.`.

- Mark

Re: Long chains created by direct Buffer::slice

[mark . reinhold]

2018/7/20 10:55:52 -0700, Florian Weimer :
> ...
> 
> The constructor invoked:
> 
> | // For duplicates and slices
> | //
> | Direct$Type$Buffer$RW$$BO$(DirectBuffer db, // package-private
> |int mark, int pos, int lim, int cap,
> |int off)
> | {
> | #if[rw]
> | super(mark, pos, lim, cap);
> | address = db.address() + off;
> | #if[byte]
> | cleaner = null;
> | #end[byte]
> | att = db;
> | #else[rw]
> | super(db, mark, pos, lim, cap, off);
> | this.isReadOnly = true;
> | #end[rw]
> | }
> 
> The key part is the assignment to the att member.  If I understand
> this correctly, it is needed to keep the backing object alive during
> the lifetime of this buffer.

Correct.

>   However, it causes the creation of a
> long chain of buffer objects.  With -Xmx100m or so, the following test
> will OOM fairly quickly for this reason:
> 
> |   volatile ByteBuffer buffer;
> |   …
> |   buffer = ByteBuffer.allocateDirect(16384);
> |   while (true) {
> |   buffer = buffer.duplicate();
> |   }

Well spotted!  This bug has been lurking there for sixteen years.

> 
> I wonder if it would be possible to change the setting of the att
> member to this instead:
> 
> | if (db.att == null) {
> | att = db;
> | } else {
> | att = db.att;
> | }
> 
> This would only keep the object alive which actually owns the backing
> storage, as if Buffer::slice had been invoked on it directly.

Your suggested fix looks fine.

- Mark

Re: RFR: Small cleanups in java.lang.ref

[mark . reinhold]

2018/5/18 3:11:25 -0700, per.li...@oracle.com:
> On 05/17/2018 10:03 PM, mark.reinh...@oracle.com wrote:
>> 2018/5/16 18:31:15 -0700, marti...@google.com:
>>> I've been confused by NULL vs null for years.
>> 
>> That’s because `NULL` in ReferenceQueue.java refers to the singleton
>> object `ReferenceQueue.NULL`, not the null value.  See the long comment
>> at the top of Reference.java for an explanation.
>> 
>> To improve this you could change mentions of `NULL` in the comments to
>> `ReferenceQueue.NULL`, but changing them to `null` would be incorrect.
> 
> The comments that were changed in the proposed patch refer to the 
> Reference.next and Reference.discovered fields, not Reference.queue. So 
> "null" should be correct there.

Oops, you’re right!  Confused by my own twenty-year old code ...

What’s missing here is a similar comment on the `Reference.queue` field.
Suggested text:

/* When active: queue with which this reference is registered
 * pending: queue with which this reference is registered
 *enqueued: ReferenceQueue.ENQUEUED
 *inactive: ReferenceQueue.NULL
 */

Martin -- while you’re at it, please lowercase “Enqueued:” and
“Inactive:” in the other comments, and hard-wrap the comment on
the `discovered` field to 80 columns.

- Mark


>>> 8203327: Small cleanups in java.lang.ref
>>> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
>>> https://bugs.openjdk.java.net/browse/JDK-8203327

Re: RFR: Small cleanups in java.lang.ref

[mark . reinhold]

2018/5/16 18:31:15 -0700, marti...@google.com:
> I've been confused by NULL vs null for years.

That’s because `NULL` in ReferenceQueue.java refers to the singleton
object `ReferenceQueue.NULL`, not the null value.  See the long comment
at the top of Reference.java for an explanation.

To improve this you could change mentions of `NULL` in the comments to
`ReferenceQueue.NULL`, but changing them to `null` would be incorrect.

> 8203327: Small cleanups in java.lang.ref
> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
> https://bugs.openjdk.java.net/browse/JDK-8203327

The other changes look fine.

- Mark

Re: uploading of new code

[mark . reinhold]

2018/4/18 10:27:36 -0700, e...@zusammenkunft.net:
> Hello, I would put it on a standalone Git repo on one of the public
> hosting sites lile.Github, especially for a first discussion
> (especially good if added JMH comparisions). I would not expect a id
> quickly/easily and a in-tree webrev could be created by sponsor for
> final ok.

For IP clarity, no Oracle engineer will review code that is not posted,
in some form or another (e-mail, webrev, JIRA), directly to OpenJDK.

For an initial discussion a patch in e-mail is a fine way to start, even
for 3,000 lines of code.  Those who care can either read it directly or
else import it into a local repo for further evaluation.

- Mark

Re: Clean-room implementation of Double::toString(double) and Float::toString(float)

[mark . reinhold]

2018/4/12 1:12:36 -0700, raffaello.giulie...@gmail.com:
> my code is now ready to be uploaded to the OpenJDK reps. Currently it 
> resides on GitHub and is under the "GPLv2 + Classpath Exception" 
> license, with myself as the copyright holder.
> 
> I asked Oracle about how the copyright notice should be adapted to meet 
> the OCA requirements but, as of today, I got no answer.
> 
> Is there any official reference about the copyright notice on OpenJDK 
> contributions?

For library code, the template is in $JDK/make/templates/gpl-cp-header [1].
It begins:

  Copyright (c) %YEARS% Oracle and/or its affiliates. All rights reserved.
  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

  This code is free software; you can redistribute it and/or modify it
  under the terms of the GNU General Public License version 2 only, as
  ...

You can just use the Oracle copyright line or, at your option, replace
it with your own.  In any case %YEARS% should be replaced with the year
in which the file was created, and if it was modified in any later year
then the creation year should be followed by the latest year in which
the file was modified, separated by a comma.

(We can better advise you on the details once we see the actual code.)

- Mark


[1] http://hg.openjdk.java.net/jdk/jdk/file/tip/make/templates/gpl-cp-header