Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v4]
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
I just tested this pull request by building [my JavaFX pull request][1] on six
Linux architectures, and it works beautifully!
First I built this pull request branch of OpenJDK for Linux on the following
hardware platforms: amd64 (x86_64), arm64 (aarch64), armhf (armv7l), i386
(i636), ppc64el (ppc64le), and s390x (s390x).
Then I added three lines of Groovy code to the JavaFX `build.gradle` file where
it invokes the `jmod` tool:
if (sourceDateEpoch != null) {
args("--source-date", sourceDateEpoch)
}
Then I built JavaFX twice for each of the architectures, with each build on a
clean, isolated container created from scratch. The result is 6 architectures ×
4,360 files = 26,160 identical files between the two sets of builds. The JMOD
archives were the last missing piece for JavaFX, so thank you, Andrew, for
completing the puzzle.
By the way, my builds of OpenJDK did not include your `jarjmodorder` branch,
yet the order for the entries in the JMOD archives are the same. In fact, I
checked my old builds and the entry order has always been the same between
builds for the JMOD archives. Only the entry timestamps were different. Is that
expected? I mean, although the order is not deterministic, is it expected that
they would end up in the same order when the archives are built in the same
environment by the same build process?
[1]: https://github.com/openjdk/jfx/pull/446
-
PR: https://git.openjdk.java.net/jdk/pull/6481
Re: RFR: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories [v2]
On Wed, 24 Nov 2021 17:06:41 GMT, Mandy Chung wrote: >> jdeps intends to report an error if there are multiple versions of the same >> class being parsed. module-info.class should be excluded from such >> detection. >> >> This patch also fixes a data race in `VersionHelper::set` and also unwraps >> the `ExecutionException` when FutureTask of parsing the classes throws an >> exception to report `MultiReleaseException` properly. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor fix to avoid casting Hello Mandy, one small issue - `ClassFileReader`, `DependencyFinder` and `VersionHelper` are misisng a copyright year update for this change. - PR: https://git.openjdk.java.net/jdk/pull/6530
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v9]
On Wed, 24 Nov 2021 15:04:35 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with three additional > commits since the last revision: > > - review suggestion - format code in ModuleInfoEntry to use 4 space > indentation > - review suggestion - change javadoc to mention module-info.class instead of > module descriptor > - review suggestion - use if/else instead of ternary operator Thank you everyone for the reviews. - PR: https://git.openjdk.java.net/jdk/pull/5486
Integrated: 8258117: jar tool sets the time stamp of module-info.class entries to the current time
On Mon, 13 Sep 2021 05:35:23 GMT, Jaikiran Pai wrote: > The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. This pull request has now been integrated. Changeset: a81e4fc0 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/a81e4fc07b654a3cc954921981d9d3c0cfd8bcec Stats: 285 lines in 2 files changed: 263 ins; 0 del; 22 mod 8258117: jar tool sets the time stamp of module-info.class entries to the current time Reviewed-by: lancea, ihse, alanb - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
On Thu, 18 Nov 2021 03:22:50 GMT, Tim Prinzing wrote:
> JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
Looks good.
src/jdk.compiler/share/classes/com/sun/tools/javac/util/RawDiagnosticFormatter.java
line 63:
> 61: * Helper class to generate stable positions for AST nodes occurring
> in diagnostic arguments.
> 62: * If the AST node appears in the same line number as the main
> diagnostic, the line is information is omitted.
> 63: * Otherwise both line and column information is included, using the
> format {@code line:col}".
Not a showstopper by any means. But while you are at it, maybe you could also
fix this?
Suggestion:
* If the AST node appears in the same line number as the main diagnostic,
the line information is omitted.
* Otherwise both line and column information is included, using the format
{@code line:col}.
-
Marked as reviewed by prappo (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
On Thu, 18 Nov 2021 19:18:24 GMT, Jonathan Gibbons wrote: > Remarkable to have not been noticed for so long! Remarkable, but not too surprising: the PR consists of source files and tests that are not part of the API Documentation. - PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v3]
On Wed, 24 Nov 2021 23:25:22 GMT, Naoto Sato wrote: >> This fix intends to honor the type (std/dst/generic) of parsed zone name for >> selecting the offset at the overlap. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with two additional > commits since the last revision: > > - Replaced integer literals. > - Refined wording #2 Thanks Naoto. The change Look good to me. - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6527
Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v2]
On Wed, 24 Nov 2021 21:27:15 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Refined wording
>
> src/java.base/share/classes/java/time/format/DateTimeFormatter.java line 509:
>
>> 507: * parsed from the zone name that does not imply daylight saving time,
>> then
>> 508: * {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} is issued
>> 509: * to use the standard offset at the overlap, before forming the
>> instant.
>
> Is the standard offset the subject instead of the withLaterOffsetAtOverlap
> method? Calling the method seems to be an impl detail to me. We might
> rephrase the sentence to sth. like:
> If the {@code ZoneId} parsed does not indicate daylight saving time, the
> standard offset will be used at the local time-line overlap as specified in
> the {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} method to form the
> instant.
Removed the method, and made `standard offset` the subject of the sentence.
> src/java.base/share/classes/java/time/format/Parsed.java line 139:
>
>> 137: * The parsed zone name type.
>> 138: */
>> 139: int zoneNameType = -1;
>
> Could be an Enum if that helps with readability.
Since the index needs some calculation, I left it as `int`. Changed to use the
constant fields for better readability.
-
PR: https://git.openjdk.java.net/jdk/pull/6527
Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v3]
> This fix intends to honor the type (std/dst/generic) of parsed zone name for > selecting the offset at the overlap. Corresponding CSR has also been drafted. Naoto Sato has updated the pull request incrementally with two additional commits since the last revision: - Replaced integer literals. - Refined wording #2 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6527/files - new: https://git.openjdk.java.net/jdk/pull/6527/files/99ad3cad..89c894ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6527=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6527=01-02 Stats: 18 lines in 4 files changed: 2 ins; 1 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/6527.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6527/head:pull/6527 PR: https://git.openjdk.java.net/jdk/pull/6527
Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v2]
On Tue, 23 Nov 2021 23:50:36 GMT, Naoto Sato wrote: >> This fix intends to honor the type (std/dst/generic) of parsed zone name for >> selecting the offset at the overlap. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Refined wording Thanks, Joe. Refined the wording in the compatibility assessment in the CSR. - PR: https://git.openjdk.java.net/jdk/pull/6527
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
On Thu, 18 Nov 2021 03:22:50 GMT, Tim Prinzing wrote: > JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
On Thu, 18 Nov 2021 03:22:50 GMT, Tim Prinzing wrote: > JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream Remarkable to have not been noticed for so long! - Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6443
RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream - Commit messages: - Fixed @code and @link in some javadoc for JDK-8276674 Changes: https://git.openjdk.java.net/jdk/pull/6443/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6443=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276674 Stats: 14 lines in 9 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/6443.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6443/head:pull/6443 PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v2]
On Tue, 23 Nov 2021 23:50:36 GMT, Naoto Sato wrote:
>> This fix intends to honor the type (std/dst/generic) of parsed zone name for
>> selecting the offset at the overlap. Corresponding CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refined wording
For the compatibility assessment, it looks like an incompatible change since
apps that expect DST will get a different offset at the overlap. The risk is
low, perhaps because of the rare use case? That additional explanation might be
helpful.
src/java.base/share/classes/java/time/format/DateTimeFormatter.java line 509:
> 507: * parsed from the zone name that does not imply daylight saving time,
> then
> 508: * {@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} is issued
> 509: * to use the standard offset at the overlap, before forming the instant.
Is the standard offset the subject instead of the withLaterOffsetAtOverlap
method? Calling the method seems to be an impl detail to me. We might rephrase
the sentence to sth. like:
If the {@code ZoneId} parsed does not indicate daylight saving time, the
standard offset will be used at the local time-line overlap as specified in the
{@link ChronoZonedDateTime#withLaterOffsetAtOverlap()} method to form the
instant.
src/java.base/share/classes/java/time/format/Parsed.java line 139:
> 137: * The parsed zone name type.
> 138: */
> 139: int zoneNameType = -1;
Could be an Enum if that helps with readability.
-
PR: https://git.openjdk.java.net/jdk/pull/6527
Integrated: 8277806: 4 tools/jar failures per platform after JDK-8272728
On Wed, 24 Nov 2021 20:08:50 GMT, Lance Andersen wrote: > HI all, > > Attached is a patch for 4 failing MR tests due the issue that was resolved > via JDK-8272728 > > Best > Lance This pull request has now been integrated. Changeset: b5841ba3 Author:Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/b5841ba3f3d079f3cfee532a4e7f23b00f5cd063 Stats: 6 lines in 2 files changed: 0 ins; 4 del; 2 mod 8277806: 4 tools/jar failures per platform after JDK-8272728 Reviewed-by: alanb, jjg - PR: https://git.openjdk.java.net/jdk/pull/6546
Re: RFR: 8277806: 4 tools/jar failures per platform after JDK-8272728
On Wed, 24 Nov 2021 20:08:50 GMT, Lance Andersen wrote: > HI all, > > Attached is a patch for 4 failing MR tests due the issue that was resolved > via JDK-8272728 > > Best > Lance Marked as reviewed by jjg (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6546
Re: RFR: 8277806: 4 tools/jar failures per platform after JDK-8272728
On Wed, 24 Nov 2021 20:08:50 GMT, Lance Andersen wrote: > HI all, > > Attached is a patch for 4 failing MR tests due the issue that was resolved > via JDK-8272728 > > Best > Lance Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6546
RFR: 8277806: 4 tools/jar failures per platform after JDK-8272728
HI all, Attached is a patch for 4 failing MR tests due the issue that was resolved via JDK-8272728 Best Lance - Commit messages: - Fix for JDK-8272728 Changes: https://git.openjdk.java.net/jdk/pull/6546/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6546=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277806 Stats: 6 lines in 2 files changed: 0 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6546.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6546/head:pull/6546 PR: https://git.openjdk.java.net/jdk/pull/6546
Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v4]
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
RFR: 8277155: Compress and expand vector operations
Add two new cross-lane vector operations, `compress` and `expand`.
An example of such usage might be code that selects elements from array `a` and
stores those selected elements in array `z`:
int[] a = ...;
int[] z = ...;
int ai = 0, zi = 0;
while (ai < a.length) {
IntVector av = IntVector.fromArray(SPECIES, a, ai);
// query over elements of vector av
// returning a mask marking elements of interest
VectorMask m = interestingBits(av, ...);
IntVector zv = av.compress(m);
zv.intoArray(z, zi, m.compress());
ai += SPECIES.length();
zi += m.trueCount();
}
(There's also a more sophisticated version using `unslice` to coalesce matching
elements with non-masked stores.)
Given RDP 1 for 18 is getting close, 2021/12/09, we may not get this reviewed
in time and included in [JEP 417](https://openjdk.java.net/jeps/417). Still I
think I think it worth starting the review now (the CSR is marked provisional).
-
Commit messages:
- 8277155: Compress and expand vector operations
Changes: https://git.openjdk.java.net/jdk/pull/6545/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6545=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8277155
Stats: 5429 lines in 105 files changed: 5315 ins; 21 del; 93 mod
Patch: https://git.openjdk.java.net/jdk/pull/6545.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6545/head:pull/6545
PR: https://git.openjdk.java.net/jdk/pull/6545
Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v4]
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
Thank you for your efforts here.
I just happened to notice that you created a
[CSR](https://bugs.openjdk.java.net/browse/JDK-8277755), which is great, thank
you.
Please add an explanation as to why the compatibility risk is minimal as that
field is currently empty to the CSR.
The current version of the PR looks reasonable. Please see a few additional
comments below
src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 2290:
> 2288: private void setSourceDate(ZipEntry e, long origTime) {
> 2289: if (sourceDate != -1) {
> 2290: e.setTimeLocal(LocalDateTime.ofEpochSecond(sourceDate, 0,
> ZoneOffset.UTC));
The above could potentially throw a DateTimeException which may be OK but I
would sanity check there are no issues
src/jdk.jartool/share/man/jar.1 line 1:
> 1: .\" Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights
> reserved.
The man pages are maintained in the close repository
test/jdk/tools/jar/JarEntryTime.java line 129:
> 127: // Make a jar file from that directory structure with
> 128: // --source-date set to epoch seconds 1647302400 (15/03/2022)
> 129: long sourceDate = 1647302400L;
Please consider adding a few before Epoch test values.
-
PR: https://git.openjdk.java.net/jdk/pull/6481
Re: RFR: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories [v2]
On Wed, 24 Nov 2021 17:06:41 GMT, Mandy Chung wrote: >> jdeps intends to report an error if there are multiple versions of the same >> class being parsed. module-info.class should be excluded from such >> detection. >> >> This patch also fixes a data race in `VersionHelper::set` and also unwraps >> the `ExecutionException` when FutureTask of parsing the classes throws an >> exception to report `MultiReleaseException` properly. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor fix to avoid casting Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6530
Re: RFR: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories [v2]
> jdeps intends to report an error if there are multiple versions of the same > class being parsed. module-info.class should be excluded from such > detection. > > This patch also fixes a data race in `VersionHelper::set` and also unwraps > the `ExecutionException` when FutureTask of parsing the classes throws an > exception to report `MultiReleaseException` properly. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: minor fix to avoid casting - Changes: - all: https://git.openjdk.java.net/jdk/pull/6530/files - new: https://git.openjdk.java.net/jdk/pull/6530/files/2cb770d4..2045e372 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6530=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6530=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6530.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6530/head:pull/6530 PR: https://git.openjdk.java.net/jdk/pull/6530
Re: RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On 11/24/2021 8:35 AM, Andy Herrick wrote:
Wait - the original change, 'addArgument("-J-Djlink.debug=true");'
adds "-D-J..." as a jpackage arg. This is not a supported command
line option, I'm not sure why it is not throwing an error all the
time, but you need to use addArgument("--java-option
-Djlink.debug=true" ); instead.
This would add -Djlink.debug=true to java options of an app, right? But
we need -Djlink.debug=true for jlink launched from jpackage.
- Alexey
I'll look into why "-J..." is being silently ignored (at least on
windows) and not throwing a invalid argument exception.
/Andy
On 11/24/2021 4:03 AM, Jaikiran Pai wrote:
Can I please get a review of this change which adds
`jlink.debug=true` system property while launching `jpackage` tests?
The previous fix for this in https://github.com/openjdk/jdk/pull/6491
didn't take into account the part where the `jpackage` tool gets
launched as a `ToolProvider` from some of these tests. This resulted
in a large number of tests failing (across different OS) in `tier2`
with errors like:
Error: Invalid Option: [-J-Djlink.debug=true]
In this current PR, the changed code now takes into account the
possibility of `jpackage` being launched as a `ToolProvider` and in
such cases doesn't add this option. To achieve this, the adding of
this argument is delayed till when the actual execution is about to
happen and thus it's now done in the
`adjustArgumentsBeforeExecution()` method of the jpackage test
framework.
With this change, I have now run the `jdk:tier2` locally on a macos
instance and the tests have all passed:
Test results: passed: 3,821; failed: 3
Report written to
jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html
Results written to
jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2
Error: Some tests failed or other problems occurred.
Finished running test 'jtreg:test/jdk:tier2'
Test report is stored in
build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2
==
Test summary
==
TEST TOTAL PASS
FAIL ERROR
jtreg:test/jdk:tier2 3824 3821 3 0 <<
==
The 3 failing tests are unrelated to this change and belong to the
`java/nio/channels/DatagramChannel/` test package.
Furthermore, I've looked into the generated logs of the following
tests to verify that the `-J-Djlink.debug=true` does get passed in
relevant tests and doesn't in those that failed previously in `tier2`:
test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java
test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java
test/jdk/tools/jpackage/share/ArgumentsTest.java
A sample from one of the logs where this system property is expected
to be passed along:
TRACE: exec: Execute
[jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage
--input ./test/input --dest ./test/output --name "Name With Space"
--type pkg --main-jar hello.jar --main-class Hello
-J-Djlink.debug=true --verbose](15); inherit I/O...
I would still like to request someone with access to CI or other OSes
(like Windows and Linux) to help test `tier2` against this PR.
-
Commit messages:
- 8277647: [REDO] JDK-8277507 Add jlink.debug system property while
launching jpackage tests to help diagonize recent intermittent failures
Changes: https://git.openjdk.java.net/jdk/pull/6534/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6534=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8277647
Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/6534.diff
Fetch: git fetch https://git.openjdk.java.net/jdk
pull/6534/head:pull/6534
PR: https://git.openjdk.java.net/jdk/pull/6534
Re: RFR: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories [v2]
On Wed, 24 Nov 2021 10:05:11 GMT, Alan Bateman wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> minor fix to avoid casting
>
> src/jdk.jdeps/share/classes/com/sun/tools/jdeps/DependencyFinder.java line
> 277:
>
>> 275: throw (Error)t;
>> 276: } else {
>> 277: throw new Error(e);
>
> A minor suggestion is that you could avoid the casts with:
>
> Throwable cause = ...
> if (cause instanceof RuntimeException e) {
> throw e;
> } else if (cause instanceof Error e) {
> throw e;
> } else {
Fixed. Thanks for reminding.
-
PR: https://git.openjdk.java.net/jdk/pull/6530
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Wed, 24 Nov 2021 15:20:42 GMT, kabutz wrote: >> BigInteger currently uses three different algorithms for multiply. The >> simple quadratic algorithm, then the slightly better Karatsuba if we exceed >> a bit count and then Toom Cook 3 once we go into the several thousands of >> bits. Since Toom Cook 3 is a recursive algorithm, it is trivial to >> parallelize it. I have demonstrated this several times in conference talks. >> In order to be consistent with other classes such as Arrays and Collection, >> I have added a parallelMultiply() method. Internally we have added a >> parameter to the private multiply method to indicate whether the calculation >> should be done in parallel. >> >> The performance improvements are as should be expected. Fibonacci of 100 >> million (using a single-threaded Dijkstra's sum of squares version) >> completes in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with >> the sequential multiply() method. This is on my 1-8-2 laptop. The final >> multiplications are with very large numbers, which then benefit from the >> parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number. >> >> We have also parallelized the private square() method. Internally, the >> square() method defaults to be sequential. >> >> Some benchmark results, run on my 1-6-2 server: >> >> >> Benchmark (n) Mode Cnt Score >> Error Units >> BigIntegerParallelMultiply.multiply100ss4 51.707 >> ± 11.194 ms/op >> BigIntegerParallelMultiply.multiply 1000ss4988.302 >> ± 235.977 ms/op >> BigIntegerParallelMultiply.multiply 1ss4 24662.063 >> ± 1123.329 ms/op >> BigIntegerParallelMultiply.parallelMultiply100ss4 49.337 >> ± 26.611 ms/op >> BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560 >> ± 268.903 ms/op >> BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551 >> ± 1899.444 ms/op >> >> >> We can see that for larger calculations (fib 100m), the execution is 2.7x >> faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for >> small (fib 1m) it is roughly the same. Considering that the fibonacci >> algorithm that we used was in itself sequential, and that the last 3 >> calculations would dominate, 2.7x faster should probably be considered quite >> good on a 1-6-2 machine. > > kabutz has updated the pull request incrementally with one additional commit > since the last revision: > > Made forkOrInvoke() method protected to avoid strange compiler error Based on the excellent feedback in this thread, I have added a depth threshold for the parallelMultiply(). This is based on log2(processors) (int)Math.ceil(Math.log(Runtime.getRuntime().availableProcessors()) / Math.log(2)) and can be overridden with `-Djava.math.BigInteger.parallelForkThreshold=num` We can also influence this number with `-XX:ActiveProcessorCount=num`, but that would have a wider impact. Here are results for running the benchmark `BigIntegerParallelMultiply` out of the box on my 1-6-2 server: Benchmark (n) Mode Cnt Score Error Units BigIntegerParallelMultiply.multiply100ss4 77.314 ± 35.690 ms/op BigIntegerParallelMultiply.multiply 1000ss4938.650 ± 69.232 ms/op BigIntegerParallelMultiply.multiply 1ss4 24811.339 ± 1457.943 ms/op BigIntegerParallelMultiply.parallelMultiply100ss4 67.550 ± 39.059 ms/op BigIntegerParallelMultiply.parallelMultiply 1000ss4489.738 ± 140.965 ms/op BigIntegerParallelMultiply.parallelMultiply 1ss4 8814.412 ± 542.567 ms/op Setting ActiveProcessorCount to 1 means that we do not fork at all: Benchmark (n) Mode Cnt Score Error Units BigIntegerParallelMultiply.multiply100ss4 72.752 ± 26.336 ms/op BigIntegerParallelMultiply.multiply 1000ss4949.417 ± 19.686 ms/op BigIntegerParallelMultiply.multiply 1ss4 24530.183 ± 1319.775 ms/op BigIntegerParallelMultiply.parallelMultiply100ss4 71.492 ± 21.860 ms/op BigIntegerParallelMultiply.parallelMultiply 1000ss4926.025 ± 41.539 ms/op BigIntegerParallelMultiply.parallelMultiply 1ss4 23947.256 ± 90.076 ms/op Similarly, when we set `-Djava.math.BigInteger.parallelForkThreshold=0` we do not fork either: Benchmark (n) Mode Cnt Score Error Units BigIntegerParallelMultiply.multiply100ss4 73.121 ± 30.315 ms/op
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v9]
On Wed, 24 Nov 2021 15:04:35 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with three additional > commits since the last revision: > > - review suggestion - format code in ModuleInfoEntry to use 4 space > indentation > - review suggestion - change javadoc to mention module-info.class instead of > module descriptor > - review suggestion - use if/else instead of ternary operator Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8277322: Document that setting an invalid property `jdk.serialFilter` disables deserialization
On Tue, 23 Nov 2021 23:07:08 GMT, Stuart Marks wrote:
>> Most configurations of `jdk.serialFilter` and` jdk.serialFilterFactory` will
>> be valid.
>> If they are not valid, the cause must be clear and useful suggestion made to
>> correct the command line
>> or security properties.
>>
>> It must not be possible to do any deserialization if the command line (or
>> security) properties are invalid.
>> Since the states are held in static fields of `ObjectInputFilter.Config`,
>> the initialization is done in a static initializer.
>> Exceptions/errors in the static initializer cause
>> `ExceptionInInitializerError` to be thrown.
>> In most cases, that will be sufficient to report the invalid properties and
>> the program will be terminated.
>> The message on the error should be sufficient correct the filter
>>
>> However, there is a chance that `ExceptionInInitializerError` will be caught
>> and ignored.
>> Ignoring the exception should not allow deserialization.
>> At the moment, this comes for free because when the initialization of
>> `ObjectInputFilter.Config`
>> fails any subsequent reference to the class causes `NoClassDefFoundError`.
>> Neither calls to `ObjectInputFilter.Config` get or set methods will succeed.
>> Deserialization is prevented because `ObjectInputStream` constructors call
>> `Config.getSerialFilterFactorySingleton` which will fail with
>> `NoClassDefFoundError`.
>>
>> The question that has been raised is how much of that should be included in
>> the specification
>> of `ObjectInputFilter.Config`. The CSR proposes that after
>> `ExceptionInInitializerError` is
>> thrown, it is specified that an implementation specific exception/error is
>> thrown when attempting to use `Config`.
>> There is no additional value in being specific about the error that is
>> thrown.
>
> If the intent is to disable serialization entirely, then this state should be
> represented explicitly. Having things throw `NoClassDefFoundError` looks like
> a mistake and a bug that needs to be fixed. In addition, it requires that all
> code paths to deserialization use `OIF.Config` in order to provoke the NCDFE.
> This might be true today, but under maintenance a different code path might
> be introduced that happens not to use that class, allowing deserialization to
> proceed.
>
> An alternative policy might be to disallow any deserialization that would use
> the default filter. This could be accomplished by having a "fail-safe" or
> "fallback" filter that always returns REJECT. This would be at least as
> restrictive and thus no less safe than any valid filter that could be
> installed. In addition, it would allow things that don't use the global
> filter to proceed, such as,
>
> var ois = new ObjectInputStream(...);
> ois.setObjectInputFilter(new ObjectInputFilter() { ... });
>
> or
>
> var ois = new ObjectInputStream(...);
> ois.setObjectInputFilter(ObjectInputFilter.Config.createFilter(...));
>
> There could be a reasonable policy discussion about whether it's preferable
> to disable all deserialization or just deserialization that depends on the
> (invalidly specified) global filter.
This is about the second line of defense; what happens when the developer
deliberately ignores the first error.
If the command line parameters are invalid it might be an option to call
`System.exit(1)` but there
is no precedent for that and it seems undesirable.
Any and all deserialization is only possible after the command line or security
properties, if any, are successfully applied.
In the examples above, the constructors for `ObjectInputStream` do not succeed
if either the serial filter or filter factory are not valid. The builtin
filter factory applies the default filter regardless of the setting of an
`ObjectInputFilter` set on the stream. The only way to completely control the
filter combinations is to provide
a valid filter factory on the command line; but that is not the case raising
the issue here.
The initialization of both could be re-specified and re-implemented to allow
the initialization of `Config` to
complete successfully but defer throwing an exception (or Error) until either
filter or filter factory
was requested from `Config.getSerialFilter` or `Config.getSerialFilterFactory`.
Both of them are called from the `ObjectInputStream` constructors.
At present, it is incompletely specified and implemented to throw
`IllegalStateException` for `getSerialFilterFactory`.
The `NCDFE` is a more reliable backstop against misuse from any source,
including reflection, than the alternative.
-
PR: https://git.openjdk.java.net/jdk/pull/6508
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v9]
On Wed, 24 Nov 2021 15:04:35 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with three additional > commits since the last revision: > > - review suggestion - format code in ModuleInfoEntry to use 4 space > indentation > - review suggestion - change javadoc to mention module-info.class instead of > module descriptor > - review suggestion - use if/else instead of ternary operator Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
> BigInteger currently uses three different algorithms for multiply. The simple > quadratic algorithm, then the slightly better Karatsuba if we exceed a bit > count and then Toom Cook 3 once we go into the several thousands of bits. > Since Toom Cook 3 is a recursive algorithm, it is trivial to parallelize it. > I have demonstrated this several times in conference talks. In order to be > consistent with other classes such as Arrays and Collection, I have added a > parallelMultiply() method. Internally we have added a parameter to the > private multiply method to indicate whether the calculation should be done in > parallel. > > The performance improvements are as should be expected. Fibonacci of 100 > million (using a single-threaded Dijkstra's sum of squares version) completes > in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with the > sequential multiply() method. This is on my 1-8-2 laptop. The final > multiplications are with very large numbers, which then benefit from the > parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number. > > We have also parallelized the private square() method. Internally, the > square() method defaults to be sequential. > > Some benchmark results, run on my 1-6-2 server: > > > Benchmark (n) Mode Cnt Score > Error Units > BigIntegerParallelMultiply.multiply100ss4 51.707 > ± 11.194 ms/op > BigIntegerParallelMultiply.multiply 1000ss4988.302 > ± 235.977 ms/op > BigIntegerParallelMultiply.multiply 1ss4 24662.063 > ± 1123.329 ms/op > BigIntegerParallelMultiply.parallelMultiply100ss4 49.337 > ± 26.611 ms/op > BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560 > ± 268.903 ms/op > BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551 > ± 1899.444 ms/op > > > We can see that for larger calculations (fib 100m), the execution is 2.7x > faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for > small (fib 1m) it is roughly the same. Considering that the fibonacci > algorithm that we used was in itself sequential, and that the last 3 > calculations would dominate, 2.7x faster should probably be considered quite > good on a 1-6-2 machine. kabutz has updated the pull request incrementally with one additional commit since the last revision: Made forkOrInvoke() method protected to avoid strange compiler error - Changes: - all: https://git.openjdk.java.net/jdk/pull/6409/files - new: https://git.openjdk.java.net/jdk/pull/6409/files/0d52e423..59de5298 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6409=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6409=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6409.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6409/head:pull/6409 PR: https://git.openjdk.java.net/jdk/pull/6409
Integrated: 8276665: ObjectInputStream.GetField.get(name, object) should throw ClassNotFoundException
On Mon, 15 Nov 2021 17:28:30 GMT, Roger Riggs wrote: > The ObjectInputStream.GetField.get(String name, Object val) method is > returning null instead of throwing an exception when the class of the object > is not found. The caller is not able to correctly handle the case where the > class is not found. The signature of GetField.get(name, val) should have a > throws ClassNotFoundException and a ClassNotFoundException exception should > be thrown. This pull request has now been integrated. Changeset: 0384739a Author:Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/0384739afc2d470ab6a3525e9d85aca0af58f2ed Stats: 210 lines in 2 files changed: 206 ins; 0 del; 4 mod 8276665: ObjectInputStream.GetField.get(name, object) should throw ClassNotFoundException Reviewed-by: naoto, lancea, smarks - PR: https://git.openjdk.java.net/jdk/pull/6393
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v9]
> The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. Jaikiran Pai has updated the pull request incrementally with three additional commits since the last revision: - review suggestion - format code in ModuleInfoEntry to use 4 space indentation - review suggestion - change javadoc to mention module-info.class instead of module descriptor - review suggestion - use if/else instead of ternary operator - Changes: - all: https://git.openjdk.java.net/jdk/pull/5486/files - new: https://git.openjdk.java.net/jdk/pull/5486/files/6f1c1b62..bdc741ca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=07-08 Stats: 12 lines in 1 file changed: 4 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v4]
> BigInteger currently uses three different algorithms for multiply. The simple > quadratic algorithm, then the slightly better Karatsuba if we exceed a bit > count and then Toom Cook 3 once we go into the several thousands of bits. > Since Toom Cook 3 is a recursive algorithm, it is trivial to parallelize it. > I have demonstrated this several times in conference talks. In order to be > consistent with other classes such as Arrays and Collection, I have added a > parallelMultiply() method. Internally we have added a parameter to the > private multiply method to indicate whether the calculation should be done in > parallel. > > The performance improvements are as should be expected. Fibonacci of 100 > million (using a single-threaded Dijkstra's sum of squares version) completes > in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with the > sequential multiply() method. This is on my 1-8-2 laptop. The final > multiplications are with very large numbers, which then benefit from the > parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number. > > We have also parallelized the private square() method. Internally, the > square() method defaults to be sequential. > > Some benchmark results, run on my 1-6-2 server: > > > Benchmark (n) Mode Cnt Score > Error Units > BigIntegerParallelMultiply.multiply100ss4 51.707 > ± 11.194 ms/op > BigIntegerParallelMultiply.multiply 1000ss4988.302 > ± 235.977 ms/op > BigIntegerParallelMultiply.multiply 1ss4 24662.063 > ± 1123.329 ms/op > BigIntegerParallelMultiply.parallelMultiply100ss4 49.337 > ± 26.611 ms/op > BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560 > ± 268.903 ms/op > BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551 > ± 1899.444 ms/op > > > We can see that for larger calculations (fib 100m), the execution is 2.7x > faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for > small (fib 1m) it is roughly the same. Considering that the fibonacci > algorithm that we used was in itself sequential, and that the last 3 > calculations would dominate, 2.7x faster should probably be considered quite > good on a 1-6-2 machine. kabutz has updated the pull request incrementally with one additional commit since the last revision: Added limit on the number of recursive tasks based on number of processors - Changes: - all: https://git.openjdk.java.net/jdk/pull/6409/files - new: https://git.openjdk.java.net/jdk/pull/6409/files/81a8b599..0d52e423 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6409=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6409=02-03 Stats: 143 lines in 2 files changed: 69 ins; 36 del; 38 mod Patch: https://git.openjdk.java.net/jdk/pull/6409.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6409/head:pull/6409 PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v8]
On Wed, 24 Nov 2021 14:06:29 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 13 commits: > > - Merge latest from master branch > - use proper FileTime centric APIs > - review suggestion - use FileTime instead of Long to prevent any potential > NPEs during unboxing > - review suggestion - split into multiple statements to make it easily > readable > - review suggestion - Use Path.of instead of Paths.get in testcase > - review suggestion - store e.getValue() and reuse the stored value > - Merge latest from master branch > - use testng asserts > - Remove "final" usage from test > - convert test to testng > - ... and 3 more: > https://git.openjdk.java.net/jdk/compare/cf7adae6...6f1c1b62 A few minor cleanups but otherwise I think this is good. src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1057: > 1055: ZipEntry e = new ZipEntry(name); > 1056: FileTime lastModified = mie.getLastModifiedTime(); > 1057: e.setLastModifiedTime(lastModified != null ? lastModified : > FileTime.fromMillis(System.currentTimeMillis())); I think this would be a bit more readable if it were if-then-else. src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1768: > 1766: return is.readAllBytes(); > 1767: } > 1768:} I think the comment would be clear if it says "the last modified time of the module-info.class". Also would you mind changing ModuleInfoEntry to use 4-space rather than 2-space indentation? - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
Hello Andy,
On 24/11/21 7:23 pm, Andy Herrick wrote:
never mind my previous email, addArgument("--java-option ...") ,
would be for argument to the java launching the app.
you are right to use -J-Djlink.debug=true . It must be interpreted in
the standard launcher used for jpackage, because the -J-D arg never
gets to the jpackage java code and the System Property "jlink.debug"
is already set to true by then.
Thank you for the clarification and verifying the value. I did a similar
(manual) check and like you say, the system property does rightly make
it to the JLinkTask.
-Jaikiran
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v8]
On Wed, 24 Nov 2021 14:06:29 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 13 commits: > > - Merge latest from master branch > - use proper FileTime centric APIs > - review suggestion - use FileTime instead of Long to prevent any potential > NPEs during unboxing > - review suggestion - split into multiple statements to make it easily > readable > - review suggestion - Use Path.of instead of Paths.get in testcase > - review suggestion - store e.getValue() and reuse the stored value > - Merge latest from master branch > - use testng asserts > - Remove "final" usage from test > - convert test to testng > - ... and 3 more: > https://git.openjdk.java.net/jdk/compare/cf7adae6...6f1c1b62 Thank you Lance and Magnus for the latest reviews and the testing. A recent commit to master in this area meant that there was a merge conflict in this PR. I have now resolved that and pushed the merge to this PR (no other changes). The test continues to pass. Alan, do these latest round of changes around `FileTime` usage that you requested, look fine? - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v7]
On Mon, 22 Nov 2021 14:37:47 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > use proper FileTime centric APIs Looks good to me. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v8]
> The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits: - Merge latest from master branch - use proper FileTime centric APIs - review suggestion - use FileTime instead of Long to prevent any potential NPEs during unboxing - review suggestion - split into multiple statements to make it easily readable - review suggestion - Use Path.of instead of Paths.get in testcase - review suggestion - store e.getValue() and reuse the stored value - Merge latest from master branch - use testng asserts - Remove "final" usage from test - convert test to testng - ... and 3 more: https://git.openjdk.java.net/jdk/compare/cf7adae6...6f1c1b62 - Changes: https://git.openjdk.java.net/jdk/pull/5486/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5486=07 Stats: 278 lines in 2 files changed: 259 ins; 0 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
never mind my previous email, addArgument("--java-option ...") , would
be for argument to the java launching the app.
you are right to use -J-Djlink.debug=true . It must be interpreted in
the standard launcher used for jpackage, because the -J-D arg never gets
to the jpackage java code and the System Property "jlink.debug" is
already set to true by then.
/Andy
On 11/24/2021 8:35 AM, Andy Herrick wrote:
Wait - the original change, 'addArgument("-J-Djlink.debug=true");'
adds "-D-J..." as a jpackage arg. This is not a supported command
line option, I'm not sure why it is not throwing an error all the
time, but you need to use addArgument("--java-option
-Djlink.debug=true" ); instead.
I'll look into why "-J..." is being silently ignored (at least on
windows) and not throwing a invalid argument exception.
/Andy
On 11/24/2021 4:03 AM, Jaikiran Pai wrote:
Can I please get a review of this change which adds
`jlink.debug=true` system property while launching `jpackage` tests?
The previous fix for this in https://github.com/openjdk/jdk/pull/6491
didn't take into account the part where the `jpackage` tool gets
launched as a `ToolProvider` from some of these tests. This resulted
in a large number of tests failing (across different OS) in `tier2`
with errors like:
Error: Invalid Option: [-J-Djlink.debug=true]
In this current PR, the changed code now takes into account the
possibility of `jpackage` being launched as a `ToolProvider` and in
such cases doesn't add this option. To achieve this, the adding of
this argument is delayed till when the actual execution is about to
happen and thus it's now done in the
`adjustArgumentsBeforeExecution()` method of the jpackage test
framework.
With this change, I have now run the `jdk:tier2` locally on a macos
instance and the tests have all passed:
Test results: passed: 3,821; failed: 3
Report written to
jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html
Results written to
jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2
Error: Some tests failed or other problems occurred.
Finished running test 'jtreg:test/jdk:tier2'
Test report is stored in
build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2
==
Test summary
==
TEST TOTAL PASS
FAIL ERROR
jtreg:test/jdk:tier2 3824 3821 3 0 <<
==
The 3 failing tests are unrelated to this change and belong to the
`java/nio/channels/DatagramChannel/` test package.
Furthermore, I've looked into the generated logs of the following
tests to verify that the `-J-Djlink.debug=true` does get passed in
relevant tests and doesn't in those that failed previously in `tier2`:
test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java
test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java
test/jdk/tools/jpackage/share/ArgumentsTest.java
A sample from one of the logs where this system property is expected
to be passed along:
TRACE: exec: Execute
[jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage
--input ./test/input --dest ./test/output --name "Name With Space"
--type pkg --main-jar hello.jar --main-class Hello
-J-Djlink.debug=true --verbose](15); inherit I/O...
I would still like to request someone with access to CI or other OSes
(like Windows and Linux) to help test `tier2` against this PR.
-
Commit messages:
- 8277647: [REDO] JDK-8277507 Add jlink.debug system property while
launching jpackage tests to help diagonize recent intermittent failures
Changes: https://git.openjdk.java.net/jdk/pull/6534/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6534=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8277647
Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/6534.diff
Fetch: git fetch https://git.openjdk.java.net/jdk
pull/6534/head:pull/6534
PR: https://git.openjdk.java.net/jdk/pull/6534
Re: RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
Wait - the original change, 'addArgument("-J-Djlink.debug=true");' adds
"-D-J..." as a jpackage arg. This is not a supported command line
option, I'm not sure why it is not throwing an error all the time, but
you need to use addArgument("--java-option -Djlink.debug=true" ); instead.
I'll look into why "-J..." is being silently ignored (at least on
windows) and not throwing a invalid argument exception.
/Andy
On 11/24/2021 4:03 AM, Jaikiran Pai wrote:
Can I please get a review of this change which adds `jlink.debug=true` system
property while launching `jpackage` tests?
The previous fix for this in https://github.com/openjdk/jdk/pull/6491 didn't
take into account the part where the `jpackage` tool gets launched as a
`ToolProvider` from some of these tests. This resulted in a large number of
tests failing (across different OS) in `tier2` with errors like:
Error: Invalid Option: [-J-Djlink.debug=true]
In this current PR, the changed code now takes into account the possibility of
`jpackage` being launched as a `ToolProvider` and in such cases doesn't add
this option. To achieve this, the adding of this argument is delayed till when
the actual execution is about to happen and thus it's now done in the
`adjustArgumentsBeforeExecution()` method of the jpackage test framework.
With this change, I have now run the `jdk:tier2` locally on a macos instance
and the tests have all passed:
Test results: passed: 3,821; failed: 3
Report written to
jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html
Results written to
jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2
Error: Some tests failed or other problems occurred.
Finished running test 'jtreg:test/jdk:tier2'
Test report is stored in
build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2
==
Test summary
==
TEST TOTAL PASS FAIL ERROR
jtreg:test/jdk:tier2 3824 3821 3 0 <<
==
The 3 failing tests are unrelated to this change and belong to the
`java/nio/channels/DatagramChannel/` test package.
Furthermore, I've looked into the generated logs of the following tests to
verify that the `-J-Djlink.debug=true` does get passed in relevant tests and
doesn't in those that failed previously in `tier2`:
test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java
test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java
test/jdk/tools/jpackage/share/ArgumentsTest.java
A sample from one of the logs where this system property is expected to be
passed along:
TRACE: exec: Execute [jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage
--input ./test/input --dest ./test/output --name "Name With Space" --type pkg
--main-jar hello.jar --main-class Hello -J-Djlink.debug=true --verbose](15); inherit
I/O...
I would still like to request someone with access to CI or other OSes (like
Windows and Linux) to help test `tier2` against this PR.
-
Commit messages:
- 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching
jpackage tests to help diagonize recent intermittent failures
Changes: https://git.openjdk.java.net/jdk/pull/6534/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6534=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8277647
Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/6534.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6534/head:pull/6534
PR: https://git.openjdk.java.net/jdk/pull/6534
Re: RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Wed, 24 Nov 2021 08:54:08 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which adds `jlink.debug=true` system > property while launching `jpackage` tests? > > The previous fix for this in https://github.com/openjdk/jdk/pull/6491 didn't > take into account the part where the `jpackage` tool gets launched as a > `ToolProvider` from some of these tests. This resulted in a large number of > tests failing (across different OS) in `tier2` with errors like: > > > Error: Invalid Option: [-J-Djlink.debug=true] > > > In this current PR, the changed code now takes into account the possibility > of `jpackage` being launched as a `ToolProvider` and in such cases doesn't > add this option. To achieve this, the adding of this argument is delayed till > when the actual execution is about to happen and thus it's now done in the > `adjustArgumentsBeforeExecution()` method of the jpackage test framework. > > With this change, I have now run the `jdk:tier2` locally on a macos instance > and the tests have all passed: > > > Test results: passed: 3,821; failed: 3 > Report written to > jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html > Results written to > jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2 > Error: Some tests failed or other problems occurred. > Finished running test 'jtreg:test/jdk:tier2' > Test report is stored in > build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2 > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >>> jtreg:test/jdk:tier2 3824 3821 3 0 << > == > > The 3 failing tests are unrelated to this change and belong to the > `java/nio/channels/DatagramChannel/` test package. > Furthermore, I've looked into the generated logs of the following tests to > verify that the `-J-Djlink.debug=true` does get passed in relevant tests and > doesn't in those that failed previously in `tier2`: > > test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java > test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java > test/jdk/tools/jpackage/share/ArgumentsTest.java > > A sample from one of the logs where this system property is expected to be > passed along: > >> TRACE: exec: Execute >> [jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage --input >> ./test/input --dest ./test/output --name "Name With Space" --type pkg >> --main-jar hello.jar --main-class Hello -J-Djlink.debug=true --verbose](15); >> inherit I/O... > > > I would still like to request someone with access to CI or other OSes (like > Windows and Linux) to help test `tier2` against this PR. @sashamatveev should be able to run the tier2 tests for you as part of his (re)reviewing the fix. - PR: https://git.openjdk.java.net/jdk/pull/6534
Re: RFR: JDK-8277375: jdeps errors on a class path with a file path with no permission
> On Nov 24, 2021, at 5:51 AM, Michael Hall wrote:
>
>
>
>> On Nov 24, 2021, at 2:55 AM, Alan Bateman wrote:
>>
>> On Wed, 24 Nov 2021 01:12:01 GMT, Mandy Chung wrote:
>>
>>> This changes jdeps -cp to ignore files/directories with no permission to
>>> access. This is consistent with the runtime behavior.
>>
>> src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line
>> 235:
>>
>>> 233: @Override
>>> 234: public FileVisitResult visitFileFailed(Path file,
>>> IOException exc) throws IOException {
>>> 235: return FileVisitResult.CONTINUE;
>>
>> The bug report may be pilot error in that they seem to have specified a root
>> directory as the class path. If the scan is changed then maybe it should
>> minimally emit a warning rather than silently ignoring the non-accessible
>> files.
>>
>> -
>>
>> PR: https://git.openjdk.java.net/jdk/pull/6531
>
> This was testing against a simple unpackaged class in my home directory. All
> the dependencies were javafx modular ones.
> There was definitely no dependency on a /Pictures package where there would
> be any need to look any further on this path.
> Is this scanning every directory off a class path one on the chance there
> might be a java class file that is a dependency?
>
Given this invocation
jdeps --module-path ~/Documents/javafx-sdk-17.0.1/lib --print-module-deps -cp .
PushMe.class
Would there be any need to scan class path at all? That would mean a module
would have a class path dependency wouldn’t it? Is that possible?
Yes, I shouldn’t of included -cp at all but shouldn’t jeeps ignore it?
Integrated: 8275063: Implementation of Foreign Function & Memory API (Second incubator)
On Tue, 12 Oct 2021 11:16:51 GMT, Maurizio Cimadamore wrote: > This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 This pull request has now been integrated. Changeset: 96e36071 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/96e36071b63b624d56739b014b457ffc48147c4f Stats: 14700 lines in 193 files changed: 6958 ins; 5126 del; 2616 mod 8275063: Implementation of Foreign Function & Memory API (Second incubator) Reviewed-by: erikj, psandoz, jvernee, darcy - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: JDK-8277375: jdeps errors on a class path with a file path with no permission
> On Nov 24, 2021, at 2:55 AM, Alan Bateman wrote:
>
> On Wed, 24 Nov 2021 01:12:01 GMT, Mandy Chung wrote:
>
>> This changes jdeps -cp to ignore files/directories with no permission to
>> access. This is consistent with the runtime behavior.
>
> src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 235:
>
>> 233: @Override
>> 234: public FileVisitResult visitFileFailed(Path file,
>> IOException exc) throws IOException {
>> 235: return FileVisitResult.CONTINUE;
>
> The bug report may be pilot error in that they seem to have specified a root
> directory as the class path. If the scan is changed then maybe it should
> minimally emit a warning rather than silently ignoring the non-accessible
> files.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/6531
This was testing against a simple unpackaged class in my home directory. All
the dependencies were javafx modular ones.
There was definitely no dependency on a /Pictures package where there would be
any need to look any further on this path.
Is this scanning every directory off a class path one on the chance there might
be a java class file that is a dependency?
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v7]
On Mon, 22 Nov 2021 14:37:47 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > use proper FileTime centric APIs I ran your latest run through Mach5 tier1 - tier3 without any failures. Your latest updates seem OK to me. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v4]
On Tue, 23 Nov 2021 20:29:15 GMT, Andrew Leonard wrote: >> Add a new --source-date (epoch milliseconds) 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 Looks good to me, but you need more reviewers - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6481
Re: RFR: JDK-8273146: Start of release updates for JDK 19 [v3]
On Tue, 23 Nov 2021 19:23:40 GMT, Joe Darcy wrote: >> The time to get JDK 19 underway draws nigh, please review this usual set of >> start-of-release updates, including CSRs for the javac and javax.lang.model >> updates: >> >> JDK-8277512: Add SourceVersion.RELEASE_19 >> https://bugs.openjdk.java.net/browse/JDK-8277512 >> >> JDK-8277514: Add source 19 and target 19 to javac >> https://bugs.openjdk.java.net/browse/JDK-8277514 >> >> Clean local tier 1 test runs for langtools, jdk, and hotspot. > > 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: > > - Remove SharedSpaces options from VMDeprecatedOptions.java. > - Merge branch 'master' into JDK-8273146 > - Respond to review feedback. > - Add --release 18 information. Good tier1 test results. > - JDK-8273146: Start of release updates for JDK 19 Build changes look good - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6493
Re: RFR: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories
On Tue, 23 Nov 2021 20:54:55 GMT, Mandy Chung wrote:
> jdeps intends to report an error if there are multiple versions of the same
> class being parsed. module-info.class should be excluded from such
> detection.
>
> This patch also fixes a data race in `VersionHelper::set` and also unwraps
> the `ExecutionException` when FutureTask of parsing the classes throws an
> exception to report `MultiReleaseException` properly.
Marked as reviewed by alanb (Reviewer).
src/jdk.jdeps/share/classes/com/sun/tools/jdeps/DependencyFinder.java line 277:
> 275: throw (Error)t;
> 276: } else {
> 277: throw new Error(e);
A minor suggestion is that you could avoid the casts with:
Throwable cause = ...
if (cause instanceof RuntimeException e) {
throw e;
} else if (cause instanceof Error e) {
throw e;
} else {
-
PR: https://git.openjdk.java.net/jdk/pull/6530
RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
Can I please get a review of this change which adds `jlink.debug=true` system property while launching `jpackage` tests? The previous fix for this in https://github.com/openjdk/jdk/pull/6491 didn't take into account the part where the `jpackage` tool gets launched as a `ToolProvider` from some of these tests. This resulted in a large number of tests failing (across different OS) in `tier2` with errors like: Error: Invalid Option: [-J-Djlink.debug=true] In this current PR, the changed code now takes into account the possibility of `jpackage` being launched as a `ToolProvider` and in such cases doesn't add this option. To achieve this, the adding of this argument is delayed till when the actual execution is about to happen and thus it's now done in the `adjustArgumentsBeforeExecution()` method of the jpackage test framework. With this change, I have now run the `jdk:tier2` locally on a macos instance and the tests have all passed: Test results: passed: 3,821; failed: 3 Report written to jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html Results written to jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2 Error: Some tests failed or other problems occurred. Finished running test 'jtreg:test/jdk:tier2' Test report is stored in build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2 == Test summary == TEST TOTAL PASS FAIL ERROR >> jtreg:test/jdk:tier2 3824 3821 3 0 << == The 3 failing tests are unrelated to this change and belong to the `java/nio/channels/DatagramChannel/` test package. Furthermore, I've looked into the generated logs of the following tests to verify that the `-J-Djlink.debug=true` does get passed in relevant tests and doesn't in those that failed previously in `tier2`: test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java test/jdk/tools/jpackage/share/ArgumentsTest.java A sample from one of the logs where this system property is expected to be passed along: > TRACE: exec: Execute > [jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage --input > ./test/input --dest ./test/output --name "Name With Space" --type pkg > --main-jar hello.jar --main-class Hello -J-Djlink.debug=true --verbose](15); > inherit I/O... I would still like to request someone with access to CI or other OSes (like Windows and Linux) to help test `tier2` against this PR. - Commit messages: - 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures Changes: https://git.openjdk.java.net/jdk/pull/6534/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6534=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277647 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6534.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6534/head:pull/6534 PR: https://git.openjdk.java.net/jdk/pull/6534
Re: RFR: JDK-8277375: jdeps errors on a class path with a file path with no permission
On Wed, 24 Nov 2021 01:12:01 GMT, Mandy Chung wrote:
> This changes jdeps -cp to ignore files/directories with no permission to
> access. This is consistent with the runtime behavior.
src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 235:
> 233: @Override
> 234: public FileVisitResult visitFileFailed(Path file,
> IOException exc) throws IOException {
> 235: return FileVisitResult.CONTINUE;
The bug report may be pilot error in that they seem to have specified a root
directory as the class path. If the scan is changed then maybe it should
minimally emit a warning rather than silently ignoring the non-accessible files.
-
PR: https://git.openjdk.java.net/jdk/pull/6531
