Re: RFR: 8273401: Remove JarIndex support in URLClassPath
On Tue, 7 Sep 2021 03:34:27 GMT, wxiang
wrote:
> There is a bug for URLClassPath.findResources with JarIndex.
> With some discussions about the bug, the current priority is to remove the
> JAR index support in URLClassPath,
> and don’t need to do anything to the jar tool in the short term, except just
> to move JarIndex to the jdk.jartool module.
>
> The PR includes:
> 1. remove the JarIndex support in URLClassPath
> 2. move JarIndex into jdk.jartool module.
Thanks for taking this on. I've done a pass over the changes and it looks
complete, just a few issues. Next step will be to create a CSR for this change.
src/java.base/share/classes/java/util/jar/JarFile.java line 220:
> 218: * The index file name.
> 219: */
> 220: public static final String INDEX_NAME = "META-INF/INDEX.LIST";
Adding this as a public field means it becomes part of the API, so it shouldn't
be public here.
src/java.base/share/classes/java/util/jar/JarVerifier.java line 147:
> 145:
> 146: if (uname.equals(JarFile.MANIFEST_NAME) ||
> 147: uname.equals(JarFile.INDEX_NAME)) {
It would be useful if someone from security-libs could comment on this. The
interaction between signed JAR and JAR index isn't very clear. The change you
have is safe but it might be that we can drop the checking for INDEX.LIST here.
test/jdk/sun/misc/JarIndex/JarIndexMergeTest.java line 44:
> 42: import sun.tools.jar.JarIndex;
> 43:
> 44: public class JarIndexMergeTest {
Is this the one remaining test in sun/misc/JarIndex? I think it can be moved to
test/jdk/java/util/jar. I think we should change the package name in the
summary comment too.
-
PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273314: Add tier4 test groups [v3]
On Mon, 6 Sep 2021 13:22:03 GMT, Aleksey Shipilev wrote:
>> During the review of JDK-8272914 that added hotspot:tier{2,3} groups,
>> @iignatev suggested to create tier4 groups that capture all tests not in
>> tiers{1,2,3}. I have excluded `vmTestbase` and `hotspot:tier4,` because they
>> take 10+ hours on my highly parallel machine. I have also excluded
>> `applications` from `hotspot:tier4`, because they require test dependencies
>> (e.g. jcstress).
>>
>> Sample run:
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:tier4 426 425 1 0
<<
jtreg:test/jdk:tier4 2891 2885 4 2
<<
>>jtreg:test/langtools:tier40 0 0 0
>>
>>jtreg:test/jaxp:tier4 0 0 0 0
>>
>> ==
>>
>> real 64m13.994s
>> user 1462m1.213s
>> sys 39m38.032s
>>
>>
>> There are interesting test failures on my machine, which I would address
>> separately.
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Drop applications and fix the comment
Once again, the disconnect between Oracle and OpenJDK test definitions seems to
be the problem for Oracle's side. Rectifying that disconnect might fall under
the scope of this PR, but I have to point out that it is a courtesy of upstream
open-source project to care about proprietary downstream definitions.
More to the point: since `tier4` as "catch-all-the-rest" was Igor's idea, I
assumed that would be in agreement with Oracle's test definitions. Following
this discussion, it seems I assumed wrong. So it puts me in a weird position to
be between two Oracle engineers arguing about proprietary test definitions I
cannot really know about, and have no decision power about. For all I care for
OpenJDK, we might as well model `tier4` after what Oracle does, as to minimize
confusion for Oracle engineers. But then again, I have no idea what Oracle
means by `tier4`. So as the alternative, I can postpone this PR until you folks
have a coherent view on this, or I can just give up on this PR and re-assign
the RFE to Igor, assuming he is willing to work this out.
Tell me what you want me to do here.
-
PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8255878: FilterInputStream is missing implementations of Java 9 InputStream methods [v2]
On Mon, 6 Sep 2021 12:02:12 GMT, Alan Bateman wrote: > Ah yes, I think you are right. In that case JDK-8255878 can be closed as WNF > or else FilterInputStream provides implementations of these methods that > don't directly delegate. We could set a boolean in the constructor if `this.getClass() == FilterInputStream.class` and only delegate in that case. On the other hand - FilterInputStream seems to be intended for subclassing, so maybe that optimization should be done in its subclasses instead, when possible. That said - I see that the class level documentation of FilterInputStream says: > The class FilterInputStream itself simply overrides all methods of > InputStream with versions that pass all requests to the contained input > stream. which is no longer strictly true - should this sentence be amended to reflect what really happens? - PR: https://git.openjdk.java.net/jdk/pull/5367
Re: better random numbers
Hello, On 2021-09-05 16:43, Andrew Haley wrote: On 9/3/21 12:35 AM, John Rose wrote: The reference I’d like to give here is to Dr. Melissa O’Neill’s website and articles: I'm quite sceptical. Anyone who says a (non-cryptographic) random- number generator is "hard to predict" is either quite naive or in a state of sin, (;-) and while O’Neill’s argument seems sound, it doesn't seem to have convinced the academic world. Lemire is thoughtful: https://lemire.me/blog/2017/08/15/on-melissa-oneills-pcg-random-number-generator/ On this blog entry (year 2017), Lemire is not giving any technical or scientific argument in favor or against PCG. He also refers to, and quotes from, a blog entry (year 2015) of an influential researcher (whose work he respects) suggesting the entry has harsh words about PCG. The fact is, that entry doesn't mention PCG or O'Neill at all and the quotation if not found there. Looking at Lemire's formal papers, they don't seem to be about PRNGs, except for one (curiously written with O'Neill herself in 2019!) about statistical tests of variants of Xorshift and Xoroshiro. I'm not competent on PRNGs at all. Still, I wouldn't rely on Lemire's blog entry when it comes to PCG. I'd rather look for other (rare) re/sources. I wonder about AES, which can do (on Apple M1) 2 parallel rounds per clock cycle. I'm quite tempted to try a reduced- round AES on the TestU01 statistical tests. Maybe 6 rounds? However, there can be a long latency between FPU and integer CPU, so perhaps it's not such a great idea. Also, you have to load the key registers before you can generate a random number, so it only really works if you want to generate a lot of bits at a time. But it is maybe 128 randomish bits per a few clock cycles.
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v3]
> Hi all, > > Could you please review the 8269373 bug fixes? > > These tests call java.lang.ProcessBuilder in direct, so not used jtreg > command option. To run non-localized tests, -Duser.language=en and > -Duser.country=US options should be added in ProcessBuilder. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform - Changes: - all: https://git.openjdk.java.net/jdk/pull/4594/files - new: https://git.openjdk.java.net/jdk/pull/4594/files/99925f72..655f5db0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4594&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4594&range=01-02 Stats: 31 lines in 6 files changed: 16 ins; 9 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/4594.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4594/head:pull/4594 PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v3]
On Tue, 7 Sep 2021 11:12:56 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: some tests in jdk/tools/launcher/ fails on localized Windows > platform I fixed it to exit silently in case the locale is not US. Thank you. - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v4]
> Hi all, > > Could you please review the 8269373 bug fixes? > > These tests call java.lang.ProcessBuilder in direct, so not used jtreg > command option. To run non-localized tests, -Duser.language=en and > -Duser.country=US options should be added in ProcessBuilder. Masanori Yano has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Merge branch 'master' into 8269373 - 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform - 8269373: use test opts for process arguments - 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform - Changes: https://git.openjdk.java.net/jdk/pull/4594/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4594&range=03 Stats: 15 lines in 3 files changed: 12 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4594.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4594/head:pull/4594 PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v5]
> Hi all, > > Could you please review the 8269373 bug fixes? > > These tests call java.lang.ProcessBuilder in direct, so not used jtreg > command option. To run non-localized tests, -Duser.language=en and > -Duser.country=US options should be added in ProcessBuilder. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform - Changes: - all: https://git.openjdk.java.net/jdk/pull/4594/files - new: https://git.openjdk.java.net/jdk/pull/4594/files/98e7cfcb..b5a46b4a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4594&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4594&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4594.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4594/head:pull/4594 PR: https://git.openjdk.java.net/jdk/pull/4594
Re: better random numbers
> > On this blog entry (year 2017), Lemire is not giving any technical or > scientific argument in favor or against PCG. > > He also refers to, and quotes from, a blog entry (year 2015) of an > influential researcher (whose work he respects) suggesting the entry has > harsh words about PCG. The fact is, that entry doesn't mention PCG or > O'Neill at all and the quotation if not found there. > That "influential researcher" is probably Sebastiano Vigna who has indeed harsh words on PCG: https://pcg.di.unimi.it/pcg.php
Re: better random numbers
On 2021-09-07 13:48, Stefan Zobel wrote: On this blog entry (year 2017), Lemire is not giving any technical or scientific argument in favor or against PCG. He also refers to, and quotes from, a blog entry (year 2015) of an influential researcher (whose work he respects) suggesting the entry has harsh words about PCG. The fact is, that entry doesn't mention PCG or O'Neill at all and the quotation if not found there. That "influential researcher" is probably Sebastiano Vigna who has indeed harsh words on PCG: https://pcg.di.unimi.it/pcg.php Perhaps. But the page you refer to is (unfortunately) not dated, except for a mention of a year 2020 result from INRIA. Lemire's blog post is from 2017. Besides, the quotation on Lemire's blog entry is not found here. Two search engines don't know about the quotation either, except for Lemire's blog.
Re: Suspicious duplicate condition in java.util.regex.Grapheme#isExcludedSpacingMark
Bug submitted on your behalf.
https://bugs.openjdk.java.net/browse/JDK-8273430
> On Sep 6, 2021, at 4:16 AM, Andrey Turbanov wrote:
>
> Hello.
> I found suspicious condition in the method
> java.util.regex.Grapheme#isExcludedSpacingMark
> It's detected by IntelliJ IDEA inspection 'Condition is covered by
> further condition'
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/regex/Grapheme.java#L157
>
> ```
> private static boolean isExcludedSpacingMark(int cp) {
> return cp == 0x102B || cp == 0x102C || cp == 0x1038 ||
> cp >= 0x1062 && cp <= 0x1064 ||
> cp >= 0x1062 && cp <= 0x106D || // <== here is the warning
> cp == 0x1083 ||
> cp >= 0x1087 && cp <= 0x108C ||
> cp == 0x108F ||
> cp >= 0x109A && cp <= 0x109C ||
> cp == 0x1A61 || cp == 0x1A63 || cp == 0x1A64 ||
> cp == 0xAA7B || cp == 0xAA7D;
> }
> ```
> There are 2 sub-conditions in this complex condition:
> cp >= 0x1062 && cp <= 0x1064 ||
> cp >= 0x1062 && cp <= 0x106D ||
>
> The second condition is _wider_ than the first one.
> I believe it's a bug. The second condition (according to
> https://www.compart.com/en/unicode/category/Mc) should look like this:
>
> cp >= 0x1067 && cp <= 0x106D ||
>
> 0x1065, 0x1066 are not from the Spacing_Mark category.
>
>
> Andrey Turbanov
Re: RFR: 8273278: Support XSLT on GraalVM Native Image--deterministic bytecode generation in XSLT
On Tue, 7 Sep 2021 06:44:22 GMT, Alan Bateman wrote: > I guess a question here will be whether this should be contributed to the > upstream Xalan project to keep the changes in the JDK in sync, or maybe the > JDK has diverged too much already for this to matter. It seems that both repositories for Xalan on Apache's GH (https://github.com/apache/xalan-java and https://github.com/apache/xalan-j) are inactive for two and a half years. - PR: https://git.openjdk.java.net/jdk/pull/5331
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Wed, 1 Sep 2021 01:05:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor cleanup and more test case. This is great work! There is more specialization than I would have expected but it looks maintainable so I think should be okay to integrate with that and investigate a special spreader later. I think it would be good to get this into JDK 18 early so that there is lots of time to test and identify corner cases with performance. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Fri, 3 Sep 2021 09:53:53 GMT, Aleksey Shipilev wrote: > This test runs a lot of configurations, and spends a lot of time serially. > This is especially pronounced when run in prospective tier4 runs > (JDK-8273314). There are reports of multi-hour runs (see JDK-8271613). We can > parallelize the test configurations for this test to make it hurt less. Also, > timeouts need to be increased for `TestUpcall` test configs, because some of > them are very slow in fastdebug mode. > > Sample run: > > > $ time CONF=linux-x86_64-server-fastdebug make run-test > TEST=java/foreign/TestMatrix.java | ts -s > 00:00:00 Building target 'run-test' in configuration > 'linux-x86_64-server-fastdebug' > 00:00:03 Test selection 'java/foreign/TestMatrix.java', will run: > 00:00:03 * jtreg:test/jdk/java/foreign/TestMatrix.java > 00:00:03 > 00:00:03 Running test 'jtreg:test/jdk/java/foreign/TestMatrix.java' > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTF > 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFF > 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFF > 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTT > 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFF > 00:00:38 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFT > 00:01:50 Passed: java/foreign/TestMatrix.java#Downcall-FF > 00:02:27 Passed: java/foreign/TestMatrix.java#Downcall-TF > 00:03:03 Passed: java/foreign/TestMatrix.java#Downcall-FT > 00:03:47 Passed: java/foreign/TestMatrix.java#Downcall-TT > 00:04:17 Passed: java/foreign/TestMatrix.java#Upcall-FTFF > 00:04:23 Passed: java/foreign/TestMatrix.java#Upcall-TFFF > 00:05:46 Passed: java/foreign/TestMatrix.java#Upcall-TTFF > 00:06:03 Passed: java/foreign/TestMatrix.java#Upcall-TFFT > 00:06:44 Passed: java/foreign/TestMatrix.java#Upcall-FTFT > 00:08:24 Passed: java/foreign/TestMatrix.java#Upcall-TFTF > 00:08:39 Passed: java/foreign/TestMatrix.java#Upcall-TTFT > 00:09:16 Passed: java/foreign/TestMatrix.java#Upcall-FTTF > 00:09:19 Passed: java/foreign/TestMatrix.java#Upcall-TFTT > 00:10:01 Passed: java/foreign/TestMatrix.java#Upcall-FTTT > 00:10:37 Passed: java/foreign/TestMatrix.java#Upcall-TTTF > 00:12:16 Passed: java/foreign/TestMatrix.java#Upcall- > 00:12:17 Test results: passed: 32 > 00:12:21 > 00:12:21 == > 00:12:21 Test summary > 00:12:21 == > 00:12:21TEST TOTAL PASS > FAIL ERROR > 00:12:21jtreg:test/jdk/java/foreign/TestMatrix.java 3232 > 0 0 > 00:12:21 == > > real 12m20.538s > user 131m54.043s > sys 0m59.944s > > > If we don't parallelize, then those 130 minutes are spent serially. How is this test executed? I think when this was added the idea was that this had to be an advanced test which had to be run explicitly by users, but would not be picked up in the various test groups/tiers. I'm defo not seeing it running in the `jdk_foreign` group, so it shouldn't run on tier4. I'm not sure what's happening? - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: RFR: 8231640: (prop) Canonical property storage
On 05/09/21 6:01 pm, Jaikiran Pai wrote: Hello Alan, On 05/09/21 1:46 pm, Alan Bateman wrote: On 04/09/2021 16:50, Jaikiran Pai wrote: The commit in this PR implements the proposal for enhancement that was discussed in the core-libs-dev mailing list recently[1], for https://bugs.openjdk.java.net/browse/JDK-8231640 At a high level - the `store()` APIs in `Properties` have been modified to now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env variable is set, then instead of writing out the current date time as a date comment, the `store()` APIs instead will use the value set for this env variable to parse it to a `Date` and write out the string form of such a date. The implementation here uses the `d MMM HH:mm:ss 'GMT'` date format and `Locale.ROOT` to format and write out such a date. This should provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. Furthermore, intentionally, no changes in the date format of the "current date" have been done. These modified `store()` APIs work in the presence of the `SecurityManager` too. The caller is expected to have a read permission on the `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that permission, then the implementation of these `store()` APIs will write out the "current date" and will ignore any value that has been set for the `SOURCE_DATE_EPOCH` env variable. This should allow for backward compatibility of existing applications, where, when they run under a `SecurityManager` and perhaps with an existing restrictive policy file, the presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` APIs. The modified `store()` APIs will also ignore any value for `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such cases, the `store()` APIs will write out the "current date" and ignore the value set for this environment variable. No exceptions will be thrown for such invalid values. This is an additional backward compatibility precaution to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking applications. In the discussion on this option then I think we were talking about changing the specification of the store methods to allow for an implementation specific means to override the date and put the discussion on SOURCE_DATE_EPOCH in an @implNote. I will move the updated javadoc to an @implNote then. I guess, the existing part where it explains how the current date comment is written out, should stay where it is currently? I've now updated the PR to move the new text of this javadoc into a @implNote. -Jaikiran
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Tue, 7 Sep 2021 13:58:48 GMT, Maurizio Cimadamore wrote: > How is this test executed? I think when this was added the idea was that this > had to be an advanced test which had to be run explicitly by users, but would > not be picked up in the various test groups/tiers. I'm defo not seeing it > running in the `jdk_foreign` group, so it shouldn't run on tier4. I'm not > sure what's happening? As I wrote in PR, "This is especially pronounced when run in prospective tier4 runs (JDK-8273314)." -- that group was supposed to run all tests not caught by other tiers. That work seems to be stalled a bit. See also JDK-8271613 that is submitted by SAP folks who run it as part of all JDK tests, I guess. But nevertheless, I think speeding up the test would be even more useful for those who run this test by hand: waiting for 12 minutes instead of 1.5 hours ;) - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Fri, 3 Sep 2021 09:53:53 GMT, Aleksey Shipilev wrote: > This test runs a lot of configurations, and spends a lot of time serially. > This is especially pronounced when run in prospective tier4 runs > (JDK-8273314). There are reports of multi-hour runs (see JDK-8271613). We can > parallelize the test configurations for this test to make it hurt less. Also, > timeouts need to be increased for `TestUpcall` test configs, because some of > them are very slow in fastdebug mode. > > Sample run: > > > $ time CONF=linux-x86_64-server-fastdebug make run-test > TEST=java/foreign/TestMatrix.java | ts -s > 00:00:00 Building target 'run-test' in configuration > 'linux-x86_64-server-fastdebug' > 00:00:03 Test selection 'java/foreign/TestMatrix.java', will run: > 00:00:03 * jtreg:test/jdk/java/foreign/TestMatrix.java > 00:00:03 > 00:00:03 Running test 'jtreg:test/jdk/java/foreign/TestMatrix.java' > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTF > 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFF > 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFF > 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTT > 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFF > 00:00:38 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFT > 00:01:50 Passed: java/foreign/TestMatrix.java#Downcall-FF > 00:02:27 Passed: java/foreign/TestMatrix.java#Downcall-TF > 00:03:03 Passed: java/foreign/TestMatrix.java#Downcall-FT > 00:03:47 Passed: java/foreign/TestMatrix.java#Downcall-TT > 00:04:17 Passed: java/foreign/TestMatrix.java#Upcall-FTFF > 00:04:23 Passed: java/foreign/TestMatrix.java#Upcall-TFFF > 00:05:46 Passed: java/foreign/TestMatrix.java#Upcall-TTFF > 00:06:03 Passed: java/foreign/TestMatrix.java#Upcall-TFFT > 00:06:44 Passed: java/foreign/TestMatrix.java#Upcall-FTFT > 00:08:24 Passed: java/foreign/TestMatrix.java#Upcall-TFTF > 00:08:39 Passed: java/foreign/TestMatrix.java#Upcall-TTFT > 00:09:16 Passed: java/foreign/TestMatrix.java#Upcall-FTTF > 00:09:19 Passed: java/foreign/TestMatrix.java#Upcall-TFTT > 00:10:01 Passed: java/foreign/TestMatrix.java#Upcall-FTTT > 00:10:37 Passed: java/foreign/TestMatrix.java#Upcall-TTTF > 00:12:16 Passed: java/foreign/TestMatrix.java#Upcall- > 00:12:17 Test results: passed: 32 > 00:12:21 > 00:12:21 == > 00:12:21 Test summary > 00:12:21 == > 00:12:21TEST TOTAL PASS > FAIL ERROR > 00:12:21jtreg:test/jdk/java/foreign/TestMatrix.java 3232 > 0 0 > 00:12:21 == > > real 12m20.538s > user 131m54.043s > sys 0m59.944s > > > If we don't parallelize, then those 130 minutes are spent serially. Ok, I see - there's a new configuration for tier4 which runs all tests excluded from tier1/2/3. To me that seems to be the issue. This particular test was never meant to be executed as part of automated build infra. So I'd suggest we disable the test from tier4 first, and then, if you still want to pursue the improvement, we can do that too. On my machine the test doesn't take 1.5 hours. - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: RFR: 8231640: (prop) Canonical property storage [v2]
> The commit in this PR implements the proposal for enhancement that was > discussed in the core-libs-dev mailing list recently[1], for > https://bugs.openjdk.java.net/browse/JDK-8231640 > > At a high level - the `store()` APIs in `Properties` have been modified to > now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env > variable is set, then instead of writing out the current date time as a date > comment, the `store()` APIs instead will use the value set for this env > variable to parse it to a `Date` and write out the string form of such a > date. The implementation here uses the `d MMM HH:mm:ss 'GMT'` date > format and `Locale.ROOT` to format and write out such a date. This should > provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. Furthermore, > intentionally, no changes in the date format of the "current date" have been > done. > > These modified `store()` APIs work in the presence of the `SecurityManager` > too. The caller is expected to have a read permission on the > `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that > permission, then the implementation of these `store()` APIs will write out > the "current date" and will ignore any value that has been set for the > `SOURCE_DATE_EPOCH` env variable. This should allow for backward > compatibility of existing applications, where, when they run under a > `SecurityManager` and perhaps with an existing restrictive policy file, the > presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` > APIs. > > The modified `store()` APIs will also ignore any value for > `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such cases, > the `store()` APIs will write out the "current date" and ignore the value set > for this environment variable. No exceptions will be thrown for such invalid > values. This is an additional backward compatibility precaution to prevent > any rogue value for `SOURCE_DATE_EPOCH` from breaking applications. > > An additional change in the implementation of these `store()` APIs and > unrelated to the date comment, is that these APIs will now write out the > property keys in a deterministic order. The keys will be written out in the > natural ordering as specified by `java.lang.String#compareTo()` API. > > The combination of the ordering of the property keys when written out and the > usage of `SOURCE_DATE_EPOCH` environment value to determine the date comment > should together allow for reproducibility of the output generated by these > `store()` APIs. > > New jtreg test classes have been introduced to verify these changes. The > primary focus of `PropertiesStoreTest` is the ordering aspects of the > property keys that are written out. On the other hand > `StoreReproducibilityTest` focuses on the reproducibility of the output > generated by these APIs. The `StoreReproducibilityTest` runs these tests > both in the presence and absence of `SecurityManager`. Plus, in the presence > of SecurityManager, it tests both the scenarios where the caller is granted > the requisite permission and in other case not granted that permission. > > These new tests and existing tests under `test/jdk/java/util/Properties/` > pass with these changes. > > [1] > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html > [2] https://reproducible-builds.org/specs/source-date-epoch/ Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: use @implNote to explain the use of the environment variable - Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/85748cf4..641864e2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5372&range=00-01 Stats: 19 lines in 1 file changed: 10 ins; 9 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Tue, 7 Sep 2021 14:05:46 GMT, Maurizio Cimadamore wrote: > if you still want to pursue the improvement, we can do that too. Yes, I still want to pursue this test improvement. - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: RFR: 8231640: (prop) Canonical property storage
On Sun, 5 Sep 2021 12:38:20 GMT, Jaikiran Pai wrote: > Do you mean that converting the keySet() of an >existing Map into an array and then sorting that array and then using >that sorted array to iterate and using these keys to do an additional >lookup for value against the original Map would be more efficient in >this case? You can convert entrySet() to array. Not a keySet. In this case there is no need to lookup for values. - PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8273435: Remove redundant zero-length check in ClassDesc.of
On Wed, 18 Aug 2021 07:28:57 GMT, Andrey Turbanov wrote: > After [JDK-8215510](https://bugs.openjdk.java.net/browse/JDK-8215510) > (eed3a536c0) this condition is always `false`. Empty package name is handled > separately. > Found by IntelliJ inspection. Marked as reviewed by stsypa...@github.com (no known OpenJDK username). Hi @turbanoff the change looks good and reasonable, I've filed an issue for this: https://bugs.openjdk.java.net/browse/JDK-8273435, so you can integrate now - PR: https://git.openjdk.java.net/jdk/pull/5157
RFR: 8273435: Remove redundant zero-length check in ClassDesc.of
After [JDK-8215510](https://bugs.openjdk.java.net/browse/JDK-8215510) (eed3a536c0) this condition is always `false`. Empty package name is handled separately. Found by IntelliJ inspection. - Commit messages: - [PATCH] Remove redundant zero-length check in ClassDesc.of Changes: https://git.openjdk.java.net/jdk/pull/5157/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5157&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273435 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5157.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5157/head:pull/5157 PR: https://git.openjdk.java.net/jdk/pull/5157
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Fri, 3 Sep 2021 09:53:53 GMT, Aleksey Shipilev wrote: > This test runs a lot of configurations, and spends a lot of time serially. > This is especially pronounced when run in prospective tier4 runs > (JDK-8273314). There are reports of multi-hour runs (see JDK-8271613). We can > parallelize the test configurations for this test to make it hurt less. Also, > timeouts need to be increased for `TestUpcall` test configs, because some of > them are very slow in fastdebug mode. > > Sample run: > > > $ time CONF=linux-x86_64-server-fastdebug make run-test > TEST=java/foreign/TestMatrix.java | ts -s > 00:00:00 Building target 'run-test' in configuration > 'linux-x86_64-server-fastdebug' > 00:00:03 Test selection 'java/foreign/TestMatrix.java', will run: > 00:00:03 * jtreg:test/jdk/java/foreign/TestMatrix.java > 00:00:03 > 00:00:03 Running test 'jtreg:test/jdk/java/foreign/TestMatrix.java' > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTF > 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFF > 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFF > 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTT > 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFF > 00:00:38 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFT > 00:01:50 Passed: java/foreign/TestMatrix.java#Downcall-FF > 00:02:27 Passed: java/foreign/TestMatrix.java#Downcall-TF > 00:03:03 Passed: java/foreign/TestMatrix.java#Downcall-FT > 00:03:47 Passed: java/foreign/TestMatrix.java#Downcall-TT > 00:04:17 Passed: java/foreign/TestMatrix.java#Upcall-FTFF > 00:04:23 Passed: java/foreign/TestMatrix.java#Upcall-TFFF > 00:05:46 Passed: java/foreign/TestMatrix.java#Upcall-TTFF > 00:06:03 Passed: java/foreign/TestMatrix.java#Upcall-TFFT > 00:06:44 Passed: java/foreign/TestMatrix.java#Upcall-FTFT > 00:08:24 Passed: java/foreign/TestMatrix.java#Upcall-TFTF > 00:08:39 Passed: java/foreign/TestMatrix.java#Upcall-TTFT > 00:09:16 Passed: java/foreign/TestMatrix.java#Upcall-FTTF > 00:09:19 Passed: java/foreign/TestMatrix.java#Upcall-TFTT > 00:10:01 Passed: java/foreign/TestMatrix.java#Upcall-FTTT > 00:10:37 Passed: java/foreign/TestMatrix.java#Upcall-TTTF > 00:12:16 Passed: java/foreign/TestMatrix.java#Upcall- > 00:12:17 Test results: passed: 32 > 00:12:21 > 00:12:21 == > 00:12:21 Test summary > 00:12:21 == > 00:12:21TEST TOTAL PASS > FAIL ERROR > 00:12:21jtreg:test/jdk/java/foreign/TestMatrix.java 3232 > 0 0 > 00:12:21 == > > real 12m20.538s > user 131m54.043s > sys 0m59.944s > > > If we don't parallelize, then those 130 minutes are spent serially. So, what is the policy for defining developers tests that are not meant to be ran on every build infra, but are meant to be run on a more casual basis by developers working in a particular area? When we added TestMatrix we made sure to exclude it from the relevant groups. I suspect other tests might have followed the same approach. But if we have a test that automatically catches all excluded tests, and folks start running this "excluded tests" group by default, what are developers supposed to do? I guess there's a reason why tests might not have been included in a tier. Defining a blanket rule which re-adds all excluded tests seems like a questionable move to me. Surely in the future, keeping this in mind, developers will probably refrain from pushing these tests to OpenJDK altogether, and store them somewhere private - which doesn't seem great. So, I understand you have a fix which parallelize the test execution (great!), but it seems like we're talking past each other a bit, in the sense that you (or any other) should have never picked up this test in an automatic test run in the first place. Also, execution time is part of the picture, albeit the most visible one, since it causes timeouts and failures. But what about CPU cycles? Sure, if we parallelize, we can get better execution time - but you still end up wasting CPU cycles on a test which you are not meant to run in the first place. Is this the right thing to do? I believe that, ultimately, this is
Re: RFR: 8273435: Remove redundant zero-length check in ClassDesc.of
On Wed, 18 Aug 2021 07:28:57 GMT, Andrey Turbanov wrote: > After [JDK-8215510](https://bugs.openjdk.java.net/browse/JDK-8215510) > (eed3a536c0) this condition is always `false`. Empty package name is handled > separately. > Found by IntelliJ inspection. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5157
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Tue, 7 Sep 2021 14:30:41 GMT, Maurizio Cimadamore wrote: > So, what is the policy for defining developers tests that are not meant to be > ran on every build infra, but are meant to be run on a more casual basis by > developers working in a particular area? When we added TestMatrix we made > sure to exclude it from the relevant groups. Honestly, I don't know. Maybe there is some jtreg keyword that gates the automatic execution? From the jtreg perspective, either automatic system or human invocation must look the same, so it has to be some external input. Personally, I was always under impression that if I add a regression test to the source tree, somebody would eventually run it. Hotspot even has the catch-all `hotspot_all` test group for this. JDK does not have a catch-all test group, but I would not be surprised if such test group existed. > So, I understand you have a fix which parallelize the test execution > (great!), but it seems like we're talking past each other a bit, in the sense > that you (or any other) should have never picked up this test in an automatic > test run in the first place. Right. But I don't think the test execution policy discussion is that relevant for the test improvement in question. Do we want those who *do* run this test manually/occasionally enjoy the improved run time due to better parallelism? If so, please consider this PR on this merit alone. - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: RFR: 8231640: (prop) Canonical property storage
Hi,
The value of SOURCE_DATE_EPOCH is not so sensitive that it needs the
protections you are applying.
The doPriv only exposes the value of that specific environment variable
and in the usual case, it is undefined.
The complexity in the specification and implementation seem unnecessary
in this case.
Though java.util.Date is used in the current implementation, its use is
discouraged in new code
in favor of java.time.ZonedDateTime.
Consider if java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME can be
used to parse and format the time.
Thanks, Roger
On 9/5/21 8:31 AM, Jaikiran Pai wrote:
Hello Alan,
On 05/09/21 1:46 pm, Alan Bateman wrote:
On 04/09/2021 16:50, Jaikiran Pai wrote:
The commit in this PR implements the proposal for enhancement that
was discussed in the core-libs-dev mailing list recently[1], for
https://bugs.openjdk.java.net/browse/JDK-8231640
At a high level - the `store()` APIs in `Properties` have been
modified to now look for the `SOURCE_DATE_EPOCH` environment
variable[2]. If that env variable is set, then instead of writing
out the current date time as a date comment, the `store()` APIs
instead will use the value set for this env variable to parse it to
a `Date` and write out the string form of such a date. The
implementation here uses the `d MMM HH:mm:ss 'GMT'` date format
and `Locale.ROOT` to format and write out such a date. This should
provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set.
Furthermore, intentionally, no changes in the date format of the
"current date" have been done.
These modified `store()` APIs work in the presence of the
`SecurityManager` too. The caller is expected to have a read
permission on the `SOURCE_DATE_EPOCH` environment variable. If the
caller doesn't have that permission, then the implementation of
these `store()` APIs will write out the "current date" and will
ignore any value that has been set for the `SOURCE_DATE_EPOCH` env
variable. This should allow for backward compatibility of existing
applications, where, when they run under a `SecurityManager` and
perhaps with an existing restrictive policy file, the presence of
`SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` APIs.
The modified `store()` APIs will also ignore any value for
`SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In
such cases, the `store()` APIs will write out the "current date" and
ignore the value set for this environment variable. No exceptions
will be thrown for such invalid values. This is an additional
backward compatibility precaution to prevent any rogue value for
`SOURCE_DATE_EPOCH` from breaking applications.
In the discussion on this option then I think we were talking about
changing the specification of the store methods to allow for an
implementation specific means to override the date and put the
discussion on SOURCE_DATE_EPOCH in an @implNote.
I will move the updated javadoc to an @implNote then. I guess, the
existing part where it explains how the current date comment is
written out, should stay where it is currently?
The SM case probably needs discussion as I was expecting to see
something like this:
PrivilegedAction pa = () -> System.getenv("SOURCE_DATE_EPOCH");
String value = AccessController.doPrivileged(pa);
as a caller of the store method (in a library for example) is
unlikely to have this permission. I assume your concern is that
untrusted code would have a way to read this specific env variable
without a permission (by way of creating and storing an empty
Properties)? In this case I'm mostly wondering if it's really worth
having a behavior difference in this mode.
I had initially started off with using a doPrivileged code in this
change. However, I soon realized that it might be a security hole,
like in the example you state. Unlike other parts of the JDK code
where the doPrivileged makes sense, since the "action" part of it does
things that are internal implementation details to the JDK, this new
piece of code that we are introducing, does a System.getenv(...) in
its "action" but will then additionally hand out the value (in a
formatted manner of course) of that environment variable to the
callers, through the OutputStream/Writer instances that the callers
pass in to the store() APIs. Effectively, this means that even when
getenv.SOURCE_DATE_EPOCH (or getenv.* for that matter) haven't been
granted to the callers, they will _always_ be able to get hold of that
environment variable's value through this approach. Like you state,
they could just call Properties.store() (which by the way, doesn't
necessarily have to be empty) to bypass the security checks that are
put in place for the direct System.getenv(...) calls from their code.
In such a doPrivelged case, the only way the administrator can prevent
this security bypass, would then be to _not_ grant this permission to
the JDK code itself, which then effectively means that th
Re: RFR: 8231640: (prop) Canonical property storage
On 07/09/21 8:35 pm, Roger Riggs wrote: Hi, The value of SOURCE_DATE_EPOCH is not so sensitive that it needs the protections you are applying. The doPriv only exposes the value of that specific environment variable and in the usual case, it is undefined. The complexity in the specification and implementation seem unnecessary in this case. Given the inputs so far, the doPriveleged approach appears to be acceptable. So I'll go ahead and update the PR shortly to change this part. Though java.util.Date is used in the current implementation, its use is discouraged in new code in favor of java.time.ZonedDateTime. Consider if java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME can be used to parse and format the time. Noted. I'll take a look at these and update the PR as necessary. Thank you all for the inputs so far. -Jaikiran
Re: RFR: 8231640: (prop) Canonical property storage
On 07/09/2021 16:05, Roger Riggs wrote: Hi, The value of SOURCE_DATE_EPOCH is not so sensitive that it needs the protections you are applying. The doPriv only exposes the value of that specific environment variable and in the usual case, it is undefined. The complexity in the specification and implementation seem unnecessary in this case. I agree. Given the complexity then it makes your suggestion/option to just drop the date from the comment somewhat tempting. -Alan
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Fri, 3 Sep 2021 09:53:53 GMT, Aleksey Shipilev wrote: > This test runs a lot of configurations, and spends a lot of time serially. > This is especially pronounced when run in prospective tier4 runs > (JDK-8273314). There are reports of multi-hour runs (see JDK-8271613). We can > parallelize the test configurations for this test to make it hurt less. Also, > timeouts need to be increased for `TestUpcall` test configs, because some of > them are very slow in fastdebug mode. > > Sample run: > > > $ time CONF=linux-x86_64-server-fastdebug make run-test > TEST=java/foreign/TestMatrix.java | ts -s > 00:00:00 Building target 'run-test' in configuration > 'linux-x86_64-server-fastdebug' > 00:00:03 Test selection 'java/foreign/TestMatrix.java', will run: > 00:00:03 * jtreg:test/jdk/java/foreign/TestMatrix.java > 00:00:03 > 00:00:03 Running test 'jtreg:test/jdk/java/foreign/TestMatrix.java' > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTF > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFT > 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTF > 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFF > 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFF > 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTT > 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFF > 00:00:38 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFT > 00:01:50 Passed: java/foreign/TestMatrix.java#Downcall-FF > 00:02:27 Passed: java/foreign/TestMatrix.java#Downcall-TF > 00:03:03 Passed: java/foreign/TestMatrix.java#Downcall-FT > 00:03:47 Passed: java/foreign/TestMatrix.java#Downcall-TT > 00:04:17 Passed: java/foreign/TestMatrix.java#Upcall-FTFF > 00:04:23 Passed: java/foreign/TestMatrix.java#Upcall-TFFF > 00:05:46 Passed: java/foreign/TestMatrix.java#Upcall-TTFF > 00:06:03 Passed: java/foreign/TestMatrix.java#Upcall-TFFT > 00:06:44 Passed: java/foreign/TestMatrix.java#Upcall-FTFT > 00:08:24 Passed: java/foreign/TestMatrix.java#Upcall-TFTF > 00:08:39 Passed: java/foreign/TestMatrix.java#Upcall-TTFT > 00:09:16 Passed: java/foreign/TestMatrix.java#Upcall-FTTF > 00:09:19 Passed: java/foreign/TestMatrix.java#Upcall-TFTT > 00:10:01 Passed: java/foreign/TestMatrix.java#Upcall-FTTT > 00:10:37 Passed: java/foreign/TestMatrix.java#Upcall-TTTF > 00:12:16 Passed: java/foreign/TestMatrix.java#Upcall- > 00:12:17 Test results: passed: 32 > 00:12:21 > 00:12:21 == > 00:12:21 Test summary > 00:12:21 == > 00:12:21TEST TOTAL PASS > FAIL ERROR > 00:12:21jtreg:test/jdk/java/foreign/TestMatrix.java 3232 > 0 0 > 00:12:21 == > > real 12m20.538s > user 131m54.043s > sys 0m59.944s > > > If we don't parallelize, then those 130 minutes are spent serially. test/jdk/java/foreign/TestMatrix.java line 7: > 5: * @build NativeTestHelper CallGeneratorHelper TestUpcallHighArity > 6: * > 7: * @run testng/othervm/native can you please throw in a `/manual` in here as well - so that this test will be disabled when running tests with jtreg `-a` option (which should be the norm in headless mode). - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Tue, 7 Sep 2021 15:04:31 GMT, Maurizio Cimadamore wrote: >> This test runs a lot of configurations, and spends a lot of time serially. >> This is especially pronounced when run in prospective tier4 runs >> (JDK-8273314). There are reports of multi-hour runs (see JDK-8271613). We >> can parallelize the test configurations for this test to make it hurt less. >> Also, timeouts need to be increased for `TestUpcall` test configs, because >> some of them are very slow in fastdebug mode. >> >> Sample run: >> >> >> $ time CONF=linux-x86_64-server-fastdebug make run-test >> TEST=java/foreign/TestMatrix.java | ts -s >> 00:00:00 Building target 'run-test' in configuration >> 'linux-x86_64-server-fastdebug' >> 00:00:03 Test selection 'java/foreign/TestMatrix.java', will run: >> 00:00:03 * jtreg:test/jdk/java/foreign/TestMatrix.java >> 00:00:03 >> 00:00:03 Running test 'jtreg:test/jdk/java/foreign/TestMatrix.java' >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFT >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTF >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity- >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTT >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFTT >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FFFT >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTTF >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTTF >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFT >> 00:00:31 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTF >> 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFFF >> 00:00:32 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFF >> 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TFTT >> 00:00:35 Passed: java/foreign/TestMatrix.java#UpcallHighArity-TTFF >> 00:00:38 Passed: java/foreign/TestMatrix.java#UpcallHighArity-FTFT >> 00:01:50 Passed: java/foreign/TestMatrix.java#Downcall-FF >> 00:02:27 Passed: java/foreign/TestMatrix.java#Downcall-TF >> 00:03:03 Passed: java/foreign/TestMatrix.java#Downcall-FT >> 00:03:47 Passed: java/foreign/TestMatrix.java#Downcall-TT >> 00:04:17 Passed: java/foreign/TestMatrix.java#Upcall-FTFF >> 00:04:23 Passed: java/foreign/TestMatrix.java#Upcall-TFFF >> 00:05:46 Passed: java/foreign/TestMatrix.java#Upcall-TTFF >> 00:06:03 Passed: java/foreign/TestMatrix.java#Upcall-TFFT >> 00:06:44 Passed: java/foreign/TestMatrix.java#Upcall-FTFT >> 00:08:24 Passed: java/foreign/TestMatrix.java#Upcall-TFTF >> 00:08:39 Passed: java/foreign/TestMatrix.java#Upcall-TTFT >> 00:09:16 Passed: java/foreign/TestMatrix.java#Upcall-FTTF >> 00:09:19 Passed: java/foreign/TestMatrix.java#Upcall-TFTT >> 00:10:01 Passed: java/foreign/TestMatrix.java#Upcall-FTTT >> 00:10:37 Passed: java/foreign/TestMatrix.java#Upcall-TTTF >> 00:12:16 Passed: java/foreign/TestMatrix.java#Upcall- >> 00:12:17 Test results: passed: 32 >> 00:12:21 >> 00:12:21 == >> 00:12:21 Test summary >> 00:12:21 == >> 00:12:21TEST TOTAL PASS >> FAIL ERROR >> 00:12:21jtreg:test/jdk/java/foreign/TestMatrix.java 3232 >> 0 0 >> 00:12:21 == >> >> real 12m20.538s >> user 131m54.043s >> sys 0m59.944s >> >> >> If we don't parallelize, then those 130 minutes are spent serially. > > test/jdk/java/foreign/TestMatrix.java line 7: > >> 5: * @build NativeTestHelper CallGeneratorHelper TestUpcallHighArity >> 6: * >> 7: * @run testng/othervm/native > > can you please throw in a `/manual` in here as well - so that this test will > be disabled when running tests with jtreg `-a` option (which should be the > norm in headless mode). I can, but it seems to me that `/manual` is for marking GUI tests that require user interaction. Let me see if there are keywords we can use. Something like "developer", so that you can pass `jtreg -k:developer` or `JTREG_KEYWORDS=developer make test ...` when you really want to run them. I'll take a closer look tomorrow. - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: Suspicious duplicate condition in java.util.regex.Grapheme#isExcludedSpacingMark
It does look incorrect. I will take a look.
Naoto
On 9/6/21 12:16 AM, Andrey Turbanov wrote:
Hello.
I found suspicious condition in the method
java.util.regex.Grapheme#isExcludedSpacingMark
It's detected by IntelliJ IDEA inspection 'Condition is covered by
further condition'
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/regex/Grapheme.java#L157
```
private static boolean isExcludedSpacingMark(int cp) {
return cp == 0x102B || cp == 0x102C || cp == 0x1038 ||
cp >= 0x1062 && cp <= 0x1064 ||
cp >= 0x1062 && cp <= 0x106D || // <== here is the warning
cp == 0x1083 ||
cp >= 0x1087 && cp <= 0x108C ||
cp == 0x108F ||
cp >= 0x109A && cp <= 0x109C ||
cp == 0x1A61 || cp == 0x1A63 || cp == 0x1A64 ||
cp == 0xAA7B || cp == 0xAA7D;
}
```
There are 2 sub-conditions in this complex condition:
cp >= 0x1062 && cp <= 0x1064 ||
cp >= 0x1062 && cp <= 0x106D ||
The second condition is _wider_ than the first one.
I believe it's a bug. The second condition (according to
https://www.compart.com/en/unicode/category/Mc) should look like this:
cp >= 0x1067 && cp <= 0x106D ||
0x1065, 0x1066 are not from the Spacing_Mark category.
Andrey Turbanov
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v5]
On Tue, 7 Sep 2021 11:26:01 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: some tests in jdk/tools/launcher/ fails on localized Windows > platform Thanks. Looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8273401: Remove JarIndex support in URLClassPath
On Tue, 7 Sep 2021 07:03:20 GMT, Alan Bateman wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> With some discussions about the bug, the current priority is to remove the >> JAR index support in URLClassPath, >> and don’t need to do anything to the jar tool in the short term, except just >> to move JarIndex to the jdk.jartool module. >> >> The PR includes: >> 1. remove the JarIndex support in URLClassPath >> 2. move JarIndex into jdk.jartool module. > > src/java.base/share/classes/java/util/jar/JarFile.java line 220: > >> 218: * The index file name. >> 219: */ >> 220: public static final String INDEX_NAME = "META-INF/INDEX.LIST"; > > Adding this as a public field means it becomes part of the API, so it > shouldn't be public here. Agree - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Tue, 7 Sep 2021 15:53:20 GMT, Aleksey Shipilev wrote: >> test/jdk/java/foreign/TestMatrix.java line 7: >> >>> 5: * @build NativeTestHelper CallGeneratorHelper TestUpcallHighArity >>> 6: * >>> 7: * @run testng/othervm/native >> >> can you please throw in a `/manual` in here as well - so that this test will >> be disabled when running tests with jtreg `-a` option (which should be the >> norm in headless mode). > > I can, but it seems to me that `/manual` is for marking GUI tests that > require user interaction. Let me see if there are keywords we can use. > Something like "developer", so that you can pass `jtreg -k:developer` or > `JTREG_KEYWORDS=developer make test ...` when you really want to run them. > I'll take a closer look tomorrow. you are right about /manual being for GUI - but @AlanBateman pointed me at few tests with libzip using it for similar reasons (e.g. long running tests). - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: RFR: 8273315: Parallelize and increase timeouts for java/foreign/TestMatrix.java test
On Tue, 7 Sep 2021 16:38:25 GMT, Maurizio Cimadamore wrote: >> I can, but it seems to me that `/manual` is for marking GUI tests that >> require user interaction. Let me see if there are keywords we can use. >> Something like "developer", so that you can pass `jtreg -k:developer` or >> `JTREG_KEYWORDS=developer make test ...` when you really want to run them. >> I'll take a closer look tomorrow. > > you are right about /manual being for GUI - but @AlanBateman pointed me at > few tests with libzip using it for similar reasons (e.g. long running tests). I don't think /manual is strictly UI but its usage is discouraged as manual tests are rarely run. The tests for the SmartCard I/O API come to mind as they need special setup. There is one or two ZIP tests that take a long time and have historically been manual tests as a result. There are also a few tests dotted around that do not have the `@Test` tag, one needs to be working in a specific area to know about these. Introducing a new test group or a keyword for these tests could be an option. In passing, I should mention the jdk_sctp test group. These tests may require kernel or other configuration and are deliberately not in any tier. That may be a comment for the other PR that is proposing a jdk tier4. - PR: https://git.openjdk.java.net/jdk/pull/5358
Re: RFR: 8273401: Remove JarIndex support in URLClassPath
On Tue, 7 Sep 2021 07:12:29 GMT, Alan Bateman wrote:
>> There is a bug for URLClassPath.findResources with JarIndex.
>> With some discussions about the bug, the current priority is to remove the
>> JAR index support in URLClassPath,
>> and don’t need to do anything to the jar tool in the short term, except just
>> to move JarIndex to the jdk.jartool module.
>>
>> The PR includes:
>> 1. remove the JarIndex support in URLClassPath
>> 2. move JarIndex into jdk.jartool module.
>
> src/java.base/share/classes/java/util/jar/JarVerifier.java line 147:
>
>> 145:
>> 146: if (uname.equals(JarFile.MANIFEST_NAME) ||
>> 147: uname.equals(JarFile.INDEX_NAME)) {
>
> It would be useful if someone from security-libs could comment on this. The
> interaction between signed JAR and JAR index isn't very clear. The change you
> have is safe but it might be that we can drop the checking for INDEX.LIST
> here.
I am thinking this line should not be removed for compatibility with existing
JARs that have indexes.
-
PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8231640: (prop) Canonical property storage [v2]
On Sat, 4 Sep 2021 18:30:06 GMT, Andrey Turbanov
wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> use @implNote to explain the use of the environment variable
>
> src/java.base/share/classes/java/util/Properties.java line 924:
>
>> 922: writeDateComment(bw);
>> 923: synchronized (this) {
>> 924: for (Map.Entry e : new
>> TreeMap<>(map).entrySet()) {
>
> Is this sorting intentionally added? It's not clear from issue description or
> PR description that order of properties should be changed too.
> Anyway I think copying `entrySet()` to array and then sorting should be
> faster, than creating a TreeMap
In case of reproducibility it should be at least ordered, i.e. keep original
input order.
-
PR: https://git.openjdk.java.net/jdk/pull/5372
RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases
Please review the fix to the issue. Avoiding overflow by not calling nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference in nano unit. - Commit messages: - 8273369: Computing micros between two instants unexpectedly overflows for some cases Changes: https://git.openjdk.java.net/jdk/pull/5396/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273369 Stats: 17 lines in 2 files changed: 15 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases
On Tue, 7 Sep 2021 18:18:49 GMT, Naoto Sato wrote: > Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Looks OK to me - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases
On Tue, 7 Sep 2021 18:18:49 GMT, Naoto Sato wrote: > Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Marked as reviewed by rriggs (Reviewer). src/java.base/share/classes/java/time/Instant.java line 1170: > 1168: long secsDiff = Math.subtractExact(end.seconds, seconds); > 1169: long totalMicros = Math.multiplyExact(secsDiff, > NANOS_PER_SECOND / 1000); > 1170: return Math.addExact(totalMicros, (end.nanos - nanos) / 1000); Can you define NANOS_PER_MICRO, the others conversions use predefined constants. - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v2]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added a constant for nanos per micro. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/09d9b257..4b874ff6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=00-01 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v2]
On Tue, 7 Sep 2021 18:46:56 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added a constant for nanos per micro. > > src/java.base/share/classes/java/time/Instant.java line 1170: > >> 1168: long secsDiff = Math.subtractExact(end.seconds, seconds); >> 1169: long totalMicros = Math.multiplyExact(secsDiff, >> NANOS_PER_SECOND / 1000); >> 1170: return Math.addExact(totalMicros, (end.nanos - nanos) / 1000); > > Can you define NANOS_PER_MICRO, the others conversions use predefined > constants. Defined it as a private field in `Instant`. - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v3]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Corrected the constant. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/4b874ff6..7d567e8b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v3]
On Tue, 7 Sep 2021 19:21:21 GMT, Naoto Sato wrote: >> Please review the fix to the issue. Avoiding overflow by not calling >> nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference >> in nano unit. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Corrected the constant. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v4]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Moved the constant to LocalTime for consistency. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/7d567e8b..bbd6abc3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=02-03 Stats: 10 lines in 2 files changed: 5 ins; 4 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v4]
On Tue, 7 Sep 2021 20:29:29 GMT, Naoto Sato wrote: >> Please review the fix to the issue. Avoiding overflow by not calling >> nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference >> in nano unit. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Moved the constant to LocalTime for consistency. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273242: (test) Refactor to use TestNG for RuntimeTests ExecCommand tests
On Wed, 1 Sep 2021 16:37:24 GMT, Roger Riggs wrote: > The ExecCommand test of Runtime.exec is difficult to maintain; the parallel > arrays are hard to keep in sync. > This cleanup converts to use TestNG DataProviders and other improvements. LGTM. test/jdk/java/lang/RuntimeTests/exec/ExecCommand.java line 270: > 268: } > 269: } > 270: } No newline at the eof. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5335
RFR: 8273450: Fix the copyright header of SVML files
Fix the copyright header of SVML files to match others. This was brought up on jdk-dev mailing list: https://mail.openjdk.java.net/pipermail/jdk-dev/2021-September/005992.html - Commit messages: - 8273450: Fix the copyright header of SVML file Changes: https://git.openjdk.java.net/jdk/pull/5399/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5399&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273450 Stats: 288 lines in 72 files changed: 144 ins; 0 del; 144 mod Patch: https://git.openjdk.java.net/jdk/pull/5399.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5399/head:pull/5399 PR: https://git.openjdk.java.net/jdk/pull/5399
Re: RFR: 8273242: (test) Refactor to use TestNG for RuntimeTests ExecCommand tests
On Wed, 1 Sep 2021 16:37:24 GMT, Roger Riggs wrote:
> The ExecCommand test of Runtime.exec is difficult to maintain; the parallel
> arrays are hard to keep in sync.
> This cleanup converts to use TestNG DataProviders and other improvements.
Looks good Roger.
A couple trivial questions/suggestions below but feel free to ignore :-)
test/jdk/java/lang/RuntimeTests/exec/ExecCommand.java line 107:
> 105: private static void deleteOut(String path) {
> 106: try {
> 107: Files.delete(FileSystems.getDefault().getPath(path));
More of a curious question, is there a reason you couldn't use
Files::deleteIfExists or Path.of() instead of FileSystems.getDefault
test/jdk/java/lang/RuntimeTests/exec/ExecCommand.java line 199:
> 197: System.setSecurityManager(null);
> 198:
> 199: testCommandMode(command, "Ambiguous Unset", testFile,
> perModeExpected.get(0));
Any thought to creating a constant for the index value for
perModeExpected.get(XX) for extra clarity?
-
Marked as reviewed by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5335
Integrated: JDK-8273246 Amend the test java/nio/channels/DatagramChannel/ManySourcesAndTargets.java to execute in othervm mode
On Fri, 3 Sep 2021 20:32:20 GMT, Mark Sheppard wrote: > A number of nio DatagramChannel tests are intermittently failing on > macosx-aarch64. > In some instances this is a receive call blocking indefinitely waiting on > data which has > already been sent, and should be available immediately to the receive method > call. > Other test failure scenarios are problems during the test compilation phase > with a SocketException being thrown and the message: > "test result: Error. Agent communication error: java.net.SocketException: No > buffer space available; check console log for any additional details" > > The ManySourcesAndTargets and other tests execute in agentvm mode. This > results in certain test diagnostic > Output being lost during the test failure handling capture process. To > mitigate this lost diagnostics, the > ManySourcesAndTargets test has been amended to execute in othervm mode. > > Additionally, to assist in the buffer allocation issue, the netstat command > executed by the test > failure_handler has an extra argument added to obtain additional details on > mbuf usage. > The failure handler will now execute with netstat -mm This pull request has now been integrated. Changeset: d6d6c069 Author:Mark Sheppard URL: https://git.openjdk.java.net/jdk/commit/d6d6c0692bff77bd18127ed61455aac39370a089 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod 8273246: Amend the test java/nio/channels/DatagramChannel/ManySourcesAndTargets.java to execute in othervm mode Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/5366
Re: RFR: 8273450: Fix the copyright header of SVML files
On Tue, 7 Sep 2021 20:25:25 GMT, Sandhya Viswanathan wrote: > Fix the copyright header of SVML files to match others. > > This was brought up on jdk-dev mailing list: > https://mail.openjdk.java.net/pipermail/jdk-dev/2021-September/005992.html Hi Sandhya, You must not change another company's copyright line, so "All rights reserved" needs to be removed again. Thanks, David - Changes requested by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5399
Re: RFR: 8273450: Fix the copyright header of SVML files
On Tue, 7 Sep 2021 23:08:08 GMT, David Holmes wrote: >> Fix the copyright header of SVML files to match others. >> >> This was brought up on jdk-dev mailing list: >> https://mail.openjdk.java.net/pipermail/jdk-dev/2021-September/005992.html > > Hi Sandhya, > > You must not change another company's copyright line, so "All rights > reserved" needs to be removed again. > > Thanks, > David @dholmes-ora I am from Intel so editing the Intel copyright line should be ok? - PR: https://git.openjdk.java.net/jdk/pull/5399
Re: RFR: 8273450: Fix the copyright header of SVML files
On Tue, 7 Sep 2021 23:19:37 GMT, Sandhya Viswanathan wrote: >> Hi Sandhya, >> >> You must not change another company's copyright line, so "All rights >> reserved" needs to be removed again. >> >> Thanks, >> David > > @dholmes-ora I am from Intel so editing the Intel copyright line should be ok? @sviswa7 My apologies, I hadn't realized you worked for Intel. But note that other Intel files i.e. ./hotspot/cpu/x86/macroAssembler_x86_*.cpp also do not have "All rights reserved". David - PR: https://git.openjdk.java.net/jdk/pull/5399
Re: RFR: 8273450: Fix the copyright header of SVML files
On Tue, 7 Sep 2021 20:25:25 GMT, Sandhya Viswanathan wrote: > Fix the copyright header of SVML files to match others. > > This was brought up on jdk-dev mailing list: > https://mail.openjdk.java.net/pipermail/jdk-dev/2021-September/005992.html Marked as reviewed by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5399
Re: RFR: 8273278: Support XSLT on GraalVM Native Image--deterministic bytecode generation in XSLT
On Wed, 1 Sep 2021 13:28:34 GMT, Jovan Stevanovic wrote: > GraalVM Native Image supports loading classes at runtime if they are known > during image build (class predefinition). This is achieved by the JVMTI agent > that registers dynamically generated classes in a regular HotSpot run. The > Native Image build uses these registered classes to embed them into the > produced binary so they can be loaded at runtime. Loading at runtime is > achieved by matching the unique hash of generated classes. > > If the generated bytecode is unstable across runs, the generated native image > can not match the hash of the runtime-generated bytecode to the pre-defined > classes. The execution failure happens here: > > https://github.com/oracle/graal/blob/master/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/PredefinedClassesSupport.java#L127-L131. > > In XSLT the produced bytecode is unstable for the following reasons: > > - Methods like ` HashMap#values` and `HashMap#keySet` result in different > traversal orders of its elements yielding a different order of methods and > fields. > > - The default `Object#toString` includes the current memory reference of > `com.sun.org.apache.xalan.internal.xsltc.compiler.Number` in the generated > class. Yeah, there hasn't been any major activities (i.e. a minor release) for 7 years. It's also true that the JDK has diverged. - PR: https://git.openjdk.java.net/jdk/pull/5331
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v4]
On Tue, 7 Sep 2021 20:29:29 GMT, Naoto Sato wrote:
>> Please review the fix to the issue. Avoiding overflow by not calling
>> nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference
>> in nano unit.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Moved the constant to LocalTime for consistency.
Marked as reviewed by joehw (Reviewer).
test/jdk/java/time/test/java/time/TestInstant.java line 102:
> 100:
> 101: @Test
> 102: public void test_microsUntil() {
A comment about the test might be helpful.
-
PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273314: Add tier4 test groups [v3]
On Mon, 6 Sep 2021 13:22:03 GMT, Aleksey Shipilev wrote:
>> During the review of JDK-8272914 that added hotspot:tier{2,3} groups,
>> @iignatev suggested to create tier4 groups that capture all tests not in
>> tiers{1,2,3}. I have excluded `vmTestbase` and `hotspot:tier4,` because they
>> take 10+ hours on my highly parallel machine. I have also excluded
>> `applications` from `hotspot:tier4`, because they require test dependencies
>> (e.g. jcstress).
>>
>> Sample run:
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:tier4 426 425 1 0
<<
jtreg:test/jdk:tier4 2891 2885 4 2
<<
>>jtreg:test/langtools:tier40 0 0 0
>>
>>jtreg:test/jaxp:tier4 0 0 0 0
>>
>> ==
>>
>> real 64m13.994s
>> user 1462m1.213s
>> sys 39m38.032s
>>
>>
>> There are interesting test failures on my machine, which I would address
>> separately.
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Drop applications and fix the comment
Hi Aleksey,
I've discussed this with Igor and while I don't agree with the rationale I
won't "block it".
Cheers,
David
-
PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8273278: Support XSLT on GraalVM Native Image--deterministic bytecode generation in XSLT
On Wed, 1 Sep 2021 13:28:34 GMT, Jovan Stevanovic wrote: > GraalVM Native Image supports loading classes at runtime if they are known > during image build (class predefinition). This is achieved by the JVMTI agent > that registers dynamically generated classes in a regular HotSpot run. The > Native Image build uses these registered classes to embed them into the > produced binary so they can be loaded at runtime. Loading at runtime is > achieved by matching the unique hash of generated classes. > > If the generated bytecode is unstable across runs, the generated native image > can not match the hash of the runtime-generated bytecode to the pre-defined > classes. The execution failure happens here: > > https://github.com/oracle/graal/blob/master/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/PredefinedClassesSupport.java#L127-L131. > > In XSLT the produced bytecode is unstable for the following reasons: > > - Methods like ` HashMap#values` and `HashMap#keySet` result in different > traversal orders of its elements yielding a different order of methods and > fields. > > - The default `Object#toString` includes the current memory reference of > `com.sun.org.apache.xalan.internal.xsltc.compiler.Number` in the generated > class. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5331
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v5]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added comments for the test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/bbd6abc3..ccf73bf7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5396&range=03-04 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v4]
On Wed, 8 Sep 2021 00:15:46 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moved the constant to LocalTime for consistency.
>
> test/jdk/java/time/test/java/time/TestInstant.java line 102:
>
>> 100:
>> 101: @Test
>> 102: public void test_microsUntil() {
>
> A comment about the test might be helpful.
Indeed. Added some comments to the test.
-
PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273259: Character.getName doesn't follow Unicode spec for ideographs [v2]
> Simple spec clarification. A CSR has also been drafted > (https://bugs.openjdk.java.net/browse/JDK-8273296). Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Refined wordings. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5354/files - new: https://git.openjdk.java.net/jdk/pull/5354/files/294e3d72..dc36e741 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5354&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5354&range=00-01 Stats: 10 lines in 1 file changed: 4 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/5354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5354/head:pull/5354 PR: https://git.openjdk.java.net/jdk/pull/5354
Re: RFR: 8273450: Fix the copyright header of SVML files
On Tue, 7 Sep 2021 23:39:54 GMT, David Holmes wrote: >> @dholmes-ora I am from Intel so editing the Intel copyright line should be >> ok? > > @sviswa7 My apologies, I hadn't realized you worked for Intel. But note that > other Intel files i.e. ./hotspot/cpu/x86/macroAssembler_x86_*.cpp also do not > have "All rights reserved". > > David Thanks a lot @dholmes-ora for the review. - PR: https://git.openjdk.java.net/jdk/pull/5399
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v4]
On Fri, 3 Sep 2021 14:41:45 GMT, Vladimir Ivanov wrote: >> `MethodHandle.asTypeCache` keeps a strong reference to adapted >> `MethodHandle` and it can introduce a class loader leak through its >> `MethodType`. >> >> Proposed fix introduces a 2-level cache (1 element each) where 1st level can >> only contain `MethodHandle`s which are guaranteed to not introduce any >> dependencies on new class loaders compared to the original `MethodHandle`. >> 2nd level is backed by a `SoftReference` and is used as a backup when the >> result of `MethodHandle.asType()` conversion can't populate the higher level >> cache. >> >> The fix is based on [the >> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) >> made by Peter Levart @plevart back in 2015. >> >> Testing: tier1 - tier6 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments I don't have a strong opinion on whether this should use a SoftReference or a WeakReference. (In fact, one might say that I have a phantom opinion.) The main thing was that the bug report mentioned WeakReference but the commit here uses SoftReferences, and I didn't see any discussion about this change. You gave some reasons above for why SoftReference is preferable, and that's ok with me. - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8273259: Character.getName doesn't follow Unicode spec for ideographs [v2]
On Wed, 8 Sep 2021 00:37:41 GMT, Naoto Sato wrote: >> Simple spec clarification. A CSR has also been drafted >> (https://bugs.openjdk.java.net/browse/JDK-8273296). > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Refined wordings. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5354
Re: RFR: 8231640: (prop) Canonical property storage
On 9/7/21 8:27 AM, Jaikiran Pai wrote: On 07/09/21 8:35 pm, Roger Riggs wrote: Though java.util.Date is used in the current implementation, its use is discouraged in new code in favor of java.time.ZonedDateTime. Consider if java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME can be used to parse and format the time. Noted. I'll take a look at these and update the PR as necessary. Unless there's an overriding reason, it might be nice to have the output format match the format used in the Debian patch that adds SOURCE_DATE_EPOCH: https://salsa.debian.org/openjdk-team/openjdk/-/blob/master/debian/patches/reproducible-properties-timestamp.diff (See JDK-8272157 for the journey that led us here, in particular, lack of doPrivileged when reading the environment. The Debian patch also doesn't provide a reproducible order, which we've already agreed is necessary.) s'marks
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v5]
On Wed, 8 Sep 2021 00:36:29 GMT, Naoto Sato wrote: >> Please review the fix to the issue. Avoiding overflow by not calling >> nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference >> in nano unit. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added comments for the test. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8231640: (prop) Canonical property storage [v2]
Hello Robert,
On 07/09/21 11:24 pm, Robert Scholte wrote:
On Sat, 4 Sep 2021 18:30:06 GMT, Andrey Turbanov
wrote:
Jaikiran Pai has updated the pull request incrementally with one additional
commit since the last revision:
use @implNote to explain the use of the environment variable
src/java.base/share/classes/java/util/Properties.java line 924:
922: writeDateComment(bw);
923: synchronized (this) {
924: for (Map.Entry e : new
TreeMap<>(map).entrySet()) {
Is this sorting intentionally added? It's not clear from issue description or
PR description that order of properties should be changed too.
Anyway I think copying `entrySet()` to array and then sorting should be faster,
than creating a TreeMap
In case of reproducibility it should be at least ordered, i.e. keep original
input order.
As discussed in the mailing list, it is agreed upon that these property
keys will be oredered when they are written out by the store() APIs.
Thus providing reproducibility. However, the order will not be the
insertion order, instead it will be the natural order of the property
keys and this order will only be applicable/maintained when using the
store() APIs. Trying to store them in a original input order will be a
much bigger change and won't just be applicable for the store() APIs but
the entire internal implementation of the Properties class itself.
-Jaikiran
Re: RFR: 8273450: Fix the copyright header of SVML files
On Tue, 7 Sep 2021 20:25:25 GMT, Sandhya Viswanathan wrote: > Fix the copyright header of SVML files to match others. > > This was brought up on jdk-dev mailing list: > https://mail.openjdk.java.net/pipermail/jdk-dev/2021-September/005992.html Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5399
Re: RFR: 8231640: (prop) Canonical property storage
Hello Stuart, On 08/09/21 6:49 am, Stuart Marks wrote: On 9/7/21 8:27 AM, Jaikiran Pai wrote: On 07/09/21 8:35 pm, Roger Riggs wrote: Though java.util.Date is used in the current implementation, its use is discouraged in new code in favor of java.time.ZonedDateTime. Consider if java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME can be used to parse and format the time. Noted. I'll take a look at these and update the PR as necessary. Unless there's an overriding reason, it might be nice to have the output format match the format used in the Debian patch that adds SOURCE_DATE_EPOCH: https://salsa.debian.org/openjdk-team/openjdk/-/blob/master/debian/patches/reproducible-properties-timestamp.diff So the current patch implementation uses the format "d MMM HH:mm:ss 'GMT'", with a Locale.ROOT (for locale neutral formatting). I chose this format since that was the one that the (deprecated) java.util.Date#toGMTString() was using. Roger's suggestion is to use DateTimeFormatter#RFC_1123_DATE_TIME date format which is "dow, d MMM HH:mm:ss GMT" (where dow == day of week) IMO, either of these formats are "well known", since they are/were used within the JDK, especially the DateTimeFormatter#RFC_1123_DATE_TIME which Roger suggested, since that's even a public spec. The one in the debian patch is "-MM-dd HH:mm:ss z" which although is fine to use, it however feels a bit "less known". I was leaning towards Roger's suggestion to use the RFC_1123_DATE_TIME in my upcoming patch update. Is there a reason why the one in debian's patch is preferable compared to a spec backed format? -Jaikiran
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Tue, 7 Sep 2021 10:39:01 GMT, Lance Andersen wrote: >> src/java.base/share/classes/java/util/jar/JarFile.java line 220: >> >>> 218: * The index file name. >>> 219: */ >>> 220: public static final String INDEX_NAME = "META-INF/INDEX.LIST"; >> >> Adding this as a public field means it becomes part of the API, so it >> shouldn't be public here. > > Agree remove public，but recover the same definition in JarIndex - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
> There is a bug for URLClassPath.findResources with JarIndex. > With some discussions about the bug, the current priority is to remove the > JAR index support in URLClassPath, > and don’t need to do anything to the jar tool in the short term, except just > to move JarIndex to the jdk.jartool module. > > The PR includes: > 1. remove the JarIndex support in URLClassPath > 2. move JarIndex into jdk.jartool module. wxiang has updated the pull request incrementally with one additional commit since the last revision: Some minor changes Include: 1. remove public for INDEX_NAME in JarFile 2. recover the definition for INDEX_NAME in JarIndex 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar - Changes: - all: https://git.openjdk.java.net/jdk/pull/5383/files - new: https://git.openjdk.java.net/jdk/pull/5383/files/7e11c607..cba9047d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5383&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5383&range=00-01 Stats: 12 lines in 4 files changed: 5 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5383.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5383/head:pull/5383 PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Tue, 7 Sep 2021 17:39:20 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/java/util/jar/JarVerifier.java line 147:
>>
>>> 145:
>>> 146: if (uname.equals(JarFile.MANIFEST_NAME) ||
>>> 147: uname.equals(JarFile.INDEX_NAME)) {
>>
>> It would be useful if someone from security-libs could comment on this. The
>> interaction between signed JAR and JAR index isn't very clear. The change
>> you have is safe but it might be that we can drop the checking for
>> INDEX.LIST here.
>
> I am thinking this line should not be removed for compatibility with existing
> JARs that have indexes.
still keep the code
-
PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:22:38 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> With some discussions about the bug, the current priority is to remove the >> JAR index support in URLClassPath, >> and don’t need to do anything to the jar tool in the short term, except just >> to move JarIndex to the jdk.jartool module. >> >> The PR includes: >> 1. remove the JarIndex support in URLClassPath >> 2. move JarIndex into jdk.jartool module. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > Some minor changes > > Include: > 1. remove public for INDEX_NAME in JarFile > 2. recover the definition for INDEX_NAME in JarIndex > 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar Create CSR: https://bugs.openjdk.java.net/browse/JDK-8273473 - PR: https://git.openjdk.java.net/jdk/pull/5383
