Re: RFR: 8231640: (prop) Canonical property storage [v15]
On Tue, 14 Sep 2021 18:11:00 GMT, Roger Riggs wrote:
> Since there is no longer a need to format an arbitrary date, I'd suggest
> going back to the original Date.toString() code. It removes the need to
> replicate the format using DateTimeBuilder and is known to be the same as
> before.
Done. I pushed an update to the PR which switches back to using Date.toString()
for the date comment. It also does a minor adjustment to the javadoc to clarify
this behaviour.
> test/jdk/java/util/Properties/StoreReproducibilityTest.java line 429:
>
>> 427: try {
>> 428: parsedDate = new
>> SimpleDateFormat(dateCommentFormat).parse(dateComment);
>> 429: } catch (ParseException pe) {
>
> Its slightly better to use the same date formatting and parsing APIs
> consistently.
> DateTimeFormatter.parse could be used here since DateTimeFormatter was used
> above.
> (Though the pattern uses "" instead of "" for the year.)
Done. I've updated the tests to use a consistent API for parsing these date
comments in the stored files. They now use the DateTimeFormatter APIs along
with the right pattern ("") to match the output of Date.toString().
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v16]
> 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: Address review suggestions: - Switch back to using Date.toString() for writing out the date comment - Update the javadoc to match this change - Update the tests to consistently use DateTimeFormatter while parsing dates - Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/6f5f1be2..7098a2c5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5372=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5372=14-15 Stats: 22 lines in 3 files changed: 2 ins; 8 del; 12 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: 8245095: Implementation of JEP 408: Simple Web Server
On Wed, 15 Sep 2021 01:54:40 GMT, Florent Guillaume wrote: > FWIW `.z` is the extension of the old Unix `compress` program. Thank you Florent, I wasn't aware of that. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 15:56:53 GMT, Julia Boes wrote: >> I think you can ignore my comment above. I went and checked the >> `content-types.properties` in their current state for both unix and windows >> and they already have a separate `application/zip` which is mapped to >> `.zip`. So I think this above change shouldn't impact anything. > > That's right, there was a duplicate entry for `.zip` in the Windows > properties file only, which I removed. > > I'm not sure if `.z` in the Unix properties file is intentional, but I do > have a PR in progress in the same area, which I will link here shortly. FWIW `.z` is the extension of the old Unix `compress` program. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:30:34 GMT, wxiang wrote: >> 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 > @shiyuexw I discussed this issue with other maintainers in this area. There > was agreement on dropping the support from the URLClassLoader implementation > but it was suggested that it should be disabled for a release or two before > the code is removed. A system property can be used to re-enable. I've updated > the CSR to reflect this proposal. Are you okay to continue with this new > proposal? It would mean that we wouldn't remove the tests but instead change > them to run with the system property. Yes, I will take care of it. Need I create a new PR? - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"
On Tue, 14 Sep 2021 15:49:36 GMT, Aleksey Shipilev wrote: >> It isn't the "formal governance" I'm concerned about, more about the folk >> who use/rely on Zero being the ones to evaluate the impact of a change like >> this. People, like myself, who do not use Zero in any way cannot evaluate >> whether this change is appropriate - it's that simple. :) > > I'll be happy to integrate as long as @dholmes-ora has no objections. @shipilev no objections from me. I will leave it to the Zero developers to worry, or not, about the Zero users. Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/5440
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=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. Hi Peter, thanks for the patch and performance measurement. Yesterday I did a quick hack and observed similar result. I'm excited to see the performance improvement in the `Const` cases without the need of spinning the hidden class and it's a simplification. On the other hand, I'm concerned of the regression of the `Var` case for field access (80%) whereas the regression of the `Var` case for method may be tolerable (9-10%) [1]. We don't have a representative benchmark for core reflection that represents the performance charactistics of real-world applications/frameworks. I can't make a conclusion whether regression of `Var` cases matter in real-world applications/frameworks until customers run their benchmarks. We have to take the conservative side to ensure no significant regression based on micro-benchmarks result. I'm afraid it's not a good state to accept. Claes is on vacation and we should get his opinion. I tried one suggestion from Vladimir Ivanov to ensure `MethodHandle::customize` on top of your patch. But it does not seem to help. In any case, the patch to make `methodAccessor` and `fieldAccessor` not volatile is a good improvement by itself (not to be a volatile field). I would suggest to fix that separately from JEP 416. For JEP 416, two possible options: 1. Keep it as is and no significant performance regression. Continue to work on the performance improvement to remove the need of spinning class. 2. Disable the spinning hidden class and have a flag to enable it so that customers can workaround if having performance issue. However, performance regression on field access is not desirable. So I would prefer option 1 and continue work on the performance improvement as follow-up work. What do you think? [1] https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,b11842fbd48ebbd9234251fded50d52e - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8272815: jpackage --type rpm produces an error: Invalid or unsupported type: [null] [v2]
On Tue, 14 Sep 2021 21:47:36 GMT, Alexey Semenyuk wrote: >> Fix jpackage error output when "--type" CLI option is missing and jpackage >> detects that it can't build native packages in the environment. > > Alexey Semenyuk has updated the pull request incrementally with one > additional commit since the last revision: > > dummy commit Marked as reviewed by almatvee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5512
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=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. Ok, here are comparisons with jdk18+9: jdk18+9 vs. mandy: https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,80203847700b9ab8baeb346a02efc804 jdk18+9 vs. peter: https://jmh.morethan.io/?gists=7b2399cea71d12dfd3434701ddeed026,b11842fbd48ebbd9234251fded50d52e mandy vs. peter: https://jmh.morethan.io/?gists=80203847700b9ab8baeb346a02efc804,b11842fbd48ebbd9234251fded50d52e jdk16 vs. jdk18+9: https://jmh.morethan.io/?gists=9219d79a09a805eb39607830270d1513,7b2399cea71d12dfd3434701ddeed026 As can be seen from the jdk16 vs. jdk18+9, jdk18+9 has improved mainly on `staticMethodConst`, but peter's variant still beats it by large margin. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8272815: jpackage --type rpm produces an error: Invalid or unsupported type: [null] [v2]
> Fix jpackage error output when "--type" CLI option is missing and jpackage > detects that it can't build native packages in the environment. Alexey Semenyuk has updated the pull request incrementally with one additional commit since the last revision: dummy commit - Changes: - all: https://git.openjdk.java.net/jdk/pull/5512/files - new: https://git.openjdk.java.net/jdk/pull/5512/files/caac338f..44cf9fde Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5512=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5512=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5512.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5512/head:pull/5512 PR: https://git.openjdk.java.net/jdk/pull/5512
Re: RFR: 8231640: (prop) Canonical property storage [v15]
On Tue, 14 Sep 2021 20:34:21 GMT, Daniel Fuchs wrote:
>> src/java.base/share/classes/java/util/Properties.java line 929:
>>
>>> 927: @SuppressWarnings("unchecked")
>>> 928: var entries = new ArrayList<>(((Map) (Map)
>>> map).entrySet());
>>> 929: entries.sort(Map.Entry.comparingByKey());
>>
>> Since Properties can be subclassed and the `entrySet()` method overridden,
>> should the set of entries to be sorted be taken from this.entrySet() instead
>> of bypassing the public API?
>
> Hmm, so if someone has subclassed `Properties` and overridden `entrySet` for
> the purpose of ordering entries, that trick will no longer work with the new
> code. I wonder - should we sort entries only when the
> `java.util.Properties.storeDate` is also defined and not empty? Or should we
> simply state in the release notes that such tricks will no longer work?
I think we want the entries to be sorted by default; that is the most useful to
the most people.
Checking for the overloaded method seems like trying too hard.
Changing the entrySet to return them sorted (always) would add overhead in a
lot of cases but would allow a subclass to re-sort them as they see fit.
A half measure would be to sort only if the properties instance was not
subclassed and spec it that way as an implementation detail.
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v15]
On Tue, 14 Sep 2021 18:53:27 GMT, Roger Riggs wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Introduce a test to make sure backslash character in the system property
>> value doesn't cause unexpected output.
>> Plus minor updates to tests to add additional checks.
>
> src/java.base/share/classes/java/util/Properties.java line 929:
>
>> 927: @SuppressWarnings("unchecked")
>> 928: var entries = new ArrayList<>(((Map) (Map)
>> map).entrySet());
>> 929: entries.sort(Map.Entry.comparingByKey());
>
> Since Properties can be subclassed and the `entrySet()` method overridden,
> should the set of entries to be sorted be taken from this.entrySet() instead
> of bypassing the public API?
Hmm, so if someone has subclassed `Properties` and overridden `entrySet` for
the purpose of ordering entries, that trick will no longer work with the new
code. I wonder - should we sort entries only when the
`java.util.Properties.storeDate` is also defined and not empty? Or should we
simply state in the release notes that such tricks will no longer work?
-
PR: https://git.openjdk.java.net/jdk/pull/5372
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=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. > Ah, I found some late copy-paste mistakes. Here's the commit (on top of > peter's variant commit mentioned above): > [plevart@791a9a6](https://github.com/plevart/jdk/commit/791a9a69967674836fcfc41746dc731396f3bb3e) Thanks for catching it. Will fix. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
On Tue, 14 Sep 2021 19:33:38 GMT, Peter Levart wrote: > Just another question: Is migrating from Unsafe based FieldAccessor(s) to > VarHandle based an important step to final goal of Unsafe elimination because > I don't think it is a necessary step for Loom. No. We could continue to use Unsafe but moving to VarHandle reduces the development cost when extending core reflection for a new feature for example for Project Valhalla, which is one of the goals. - PR: https://git.openjdk.java.net/jdk/pull/5027
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=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. For the performance comparison, the current baseline for this PR is jdk-18+9 that will better reflect the performance gain/regression. - PR: https://git.openjdk.java.net/jdk/pull/5027
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=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. Ah, I found some late copy-paste mistakes. Here's the commit (on top of peter's variant commit mentioned above): https://github.com/plevart/jdk/commit/791a9a69967674836fcfc41746dc731396f3bb3e - PR: https://git.openjdk.java.net/jdk/pull/5027
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=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. Just another question: Is migrating from Unsafe based FieldAccessor(s) to VarHandle based an important step to final goal of Unsafe elimination because I don't think it is a necessary step for Loom. - PR: https://git.openjdk.java.net/jdk/pull/5027
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=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. Hi Mandy, So, here's the mentioned approach: https://github.com/plevart/jdk/commit/1a31508b7d73bd9fa40aae845c06b24c3cbcfd16 This is a commit above your changes that I have picked from your branch and squashed as a single commit above current master: https://github.com/plevart/jdk/commit/a2e4a1f9dad9e6951949ce82af54639ed2da9ddf I think you could chery-pick the former commit into your branch if you wanted to. What I have done is mainly removing all code around MHInvoker and VHInvoker and calling the target MethodHandle/VarHandle directly in the XxxAccessors. I also made sure that the path of references going from Method/Constructor/Field via MethodAccessor(s)/ConstructorAccessor(s)/FieldAccessor(s) and to MethodHandle/VarHandle is marked with @Stable. So if the Method/Constructor/Field is found to be constant by JIT, so is its associated MethodHandle/VarHandle. I also "fixed" existing MethodAccessor(s)/ConstructorAccessor(s)/FieldAccessor(s) so that they are safe to be "published" via data-race as the fields holding them in Method/Constructor are no longer volatile (in Field they never have been volatile). In Field I had to make two distinct call-sites for fieldAccessor vs. overrideFieldAccessor in order for JIT to propagate @Stable-ness down the chain. I then ran the following JMH benchmarks (not included in my variant of patch yet) on jdk16, mandy's variant and peter's variant: https://gist.github.com/plevart/b9c62c8cac9784e2c6d5f51a6386fa84 The results can be visualized via the following links: jdk16 vs. mandy: https://jmh.morethan.io/?gists=9219d79a09a805eb39607830270d1513,80203847700b9ab8baeb346a02efc804 jdk16 vs. peter: https://jmh.morethan.io/?gists=9219d79a09a805eb39607830270d1513,b11842fbd48ebbd9234251fded50d52e mandy vs. peter: https://jmh.morethan.io/?gists=80203847700b9ab8baeb346a02efc804,b11842fbd48ebbd9234251fded50d52e Comparing mandy vs. peter, there are pros and cons. If Method/Field instance is found to be constant by JIT, peter's variant is a little better (16...36%). It Method/Field is not constant as found by JIT, but still a single instance of Method/Field is invoked in a particular call-site, peter's variant is worse (22...48%). I added another variant called "Poly" (for static methods/fields only, but I think results would be similar for instance too). In this variant a single call-site operates with distinct instances of Method/Field and peter's variant is a little better there (3...11%). The cold-start is a little better for peter's variant too (5%). I think the "Poly" variants are an important representative of real-world usage. Every usage happens in the real world (Const, Var, Poly), but most applications/frameworks that use reflection and where performance matters (such as JPA, various serialization frameworks, etc.) are generic algorithm(s) that abstract over various runtime types, accessing methods/fields/constructors via call-sites that deal with multiple instances of Mehod/Field/Constructor each. Top speed is possible with peter's variant and even better than with mandys's variant when each call-site
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v2]
> Sorting: > > - adopt radix sort for sequential and parallel sorts on int/long/float/double > arrays (almost random and length > 6K) > - fix tryMergeRuns() to better handle case when the last run is a single > element > - minor javadoc and comment changes > > Testing: > - add new data inputs in tests for sorting > - add min/max/infinity values to float/double testing > - add tests for radix sort iaroslavski has updated the pull request incrementally with one additional commit since the last revision: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) Update target version - Changes: - all: https://git.openjdk.java.net/jdk/pull/3938/files - new: https://git.openjdk.java.net/jdk/pull/3938/files/2d972887..189f547a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3938=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3938=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3938/head:pull/3938 PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v42]
On Tue, 14 Sep 2021 18:58:16 GMT, stefan-zobel wrote: > The package javadoc of java.util.random says that `mixLea32` is used as a > mixing function in the L64 and L128 generators which doesn't seem to be > correct. That should probably read `mixLea64` See https://bugs.openjdk.java.net/browse/JDK-8272866 - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v42]
On Mon, 5 Apr 2021 14:20:56 GMT, Jim Laskey wrote: >> This PR is to introduce a new random number API for the JDK. The primary API >> is found in RandomGenerator and RandomGeneratorFactory. Further description >> can be found in the JEP https://openjdk.java.net/jeps/356 . >> >> javadoc can be found at >> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html >> >> old PR: https://github.com/openjdk/jdk/pull/1273 > > Jim Laskey has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 78 commits: > > - Merge branch 'master' into 8248862 > - Fix NotCompliantCauseTest to not rely on Random not being a bean > - Merge branch 'master' into 8248862 > - Correct return type of RandomGeneratorFactory.of > - Merge branch 'master' into 8248862 > - CSR review revisions > - CSR review updates > - Removed @since from nextDouble ThreadLocalRandom > - RandomGeneratorFactory::all(Class category) @implNote was out of date > - Clarify all() > - ... and 68 more: > https://git.openjdk.java.net/jdk/compare/a8005efd...ffd982b0 The package javadoc of java.util.random says that `mixLea32` is used as a mixing function in the L64 and L128 generators which doesn't seem to be correct. That should probably read `mixLea64` - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8231640: (prop) Canonical property storage [v15]
On Tue, 14 Sep 2021 14:02:10 GMT, 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.
>>
>> 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:
>
> Introduce a test to make sure backslash character in the system property
> value doesn't cause unexpected output.
> Plus minor updates to tests to add additional checks.
src/java.base/share/classes/java/util/Properties.java line 929:
> 927: @SuppressWarnings("unchecked")
> 928: var entries = new ArrayList<>(((Map) (Map)
> map).entrySet());
> 929: entries.sort(Map.Entry.comparingByKey());
Since Properties can be subclassed and the `entrySet()` method overridden,
should the set of entries to be sorted be taken from this.entrySet() instead of
bypassing the public API?
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v15]
On Tue, 14 Sep 2021 14:02:10 GMT, 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.
>>
>> 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:
>
> Introduce a test to make sure backslash character in the system property
> value doesn't cause unexpected output.
> Plus minor updates to tests to add additional checks.
Looks good.
src/java.base/share/classes/java/util/Properties.java line 955:
> 953: bw.write("#" +
> DateTimeFormatter.ofPattern(dateFormatPattern).format(ZonedDateTime.now()));
> 954: bw.newLine();
> 955: }
Since there is no longer a need to format an arbitrary date, I'd suggest going
back to the original Date.toString() code. It removes the need to replicate the
format using DateTimeBuilder and is known to be the same as before.
If you keep the DateTimeFormat version, you would want "" instead of
"". In java.time.
DateTimeFormatterBuilder uses slightly different pattern letter conventions as
compared to SimpleDateFormat.
test/jdk/java/util/Properties/StoreReproducibilityTest.java line 429:
> 427: try {
> 428: parsedDate = new
> SimpleDateFormat(dateCommentFormat).parse(dateComment);
> 429: } catch (ParseException pe) {
Its slightly better to use the same date formatting and parsing APIs
consistently.
DateTimeFormatter.parse could be used here since DateTimeFormatter was used
above.
(Though the pattern uses "" instead of
Re: RFR: JDK-8273526: Extend the OSContainer API pids controller with pids.current [v3]
On Tue, 14 Sep 2021 17:25:05 GMT, Ioi Lam wrote: > Maybe it should be "any_integer"? +1 - PR: https://git.openjdk.java.net/jdk/pull/5437
Re: RFR: 8272815: jpackage --type rpm produces an error: Invalid or unsupported type: [null]
On Tue, 14 Sep 2021 17:21:29 GMT, Alexey Semenyuk wrote: > Fix jpackage error output when "--type" CLI option is missing and jpackage > detects that it can't build native packages in the environment. Marked as reviewed by herrick (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5512
Re: RFR: 8231640: (prop) Canonical property storage [v15]
On Tue, 14 Sep 2021 14:02:10 GMT, 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. >> >> 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: > > Introduce a test to make sure backslash character in the system property > value doesn't cause unexpected output. > Plus minor updates to tests to add additional checks. Marked as reviewed by smarks (Reviewer). OK, after all the twists and turns, this looks good! src/java.base/share/classes/java/util/Properties.java line 173: > 171: // used to format the date comment written out by the store() APIs. > 172: // This format matches the one used by java.util.Date.toString() > 173: private static final String dateFormatPattern = "EEE MMM dd HH:mm:ss > zzz "; I'd rename this something like DATE_FORMAT_PATTERN to conform to naming conventions for constants. - PR: https://git.openjdk.java.net/jdk/pull/5372
RFR: 8272815: jpackage --type rpm produces an error: Invalid or unsupported type: [null]
Fix jpackage error output when "--type" CLI option is missing and jpackage detects that it can't build native packages in the environment. - Commit messages: - 8272815: jpackage --type rpm produces an error: Invalid or unsupported type: [null] Changes: https://git.openjdk.java.net/jdk/pull/5512/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5512=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272815 Stats: 18 lines in 2 files changed: 2 ins; 4 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/5512.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5512/head:pull/5512 PR: https://git.openjdk.java.net/jdk/pull/5512
Integrated: JDK-8273040: Turning off JpAllowDowngrades (or Upgrades)
On Tue, 14 Sep 2021 13:38:42 GMT, Andy Herrick wrote: > JDK-8273040: Turning off JpAllowDowngrades (or Upgrades) This pull request has now been integrated. Changeset: 22a7191f Author:Andy Herrick URL: https://git.openjdk.java.net/jdk/commit/22a7191f700c6966c59dcd12476f01452243542b Stats: 17 lines in 3 files changed: 7 ins; 5 del; 5 mod 8273040: Turning off JpAllowDowngrades (or Upgrades) Reviewed-by: asemenyuk - PR: https://git.openjdk.java.net/jdk/pull/5509
Re: RFR: JDK-8273526: Extend the OSContainer API pids controller with pids.current [v3]
On Tue, 14 Sep 2021 16:36:24 GMT, Ioi Lam wrote:
>> Matthias Baesken has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Simplify coding following Severins advice
>
> test/hotspot/jtreg/containers/docker/TestPids.java line 97:
>
>> 95: System.out.println("DEBUG: parts.length = " +
>> parts.length);
>> 96: if (expectedValue.equals("no_value_expected")) {
>> 97: Asserts.assertEquals(parts.length, 2);
>
> Is "no_value_expected" generated by Docker? I searched the entire HotSpot
> source code and couldn't find it. I also couldn't find "WARNING: Your kernel
> does not support pids limit capabilities".
>
> To make it easier to understand this test, I would suggest grouping all
> messages that were generated outside of HotSpot into something like:
>
>
> // These messages are generated by Docker
> static final String warning_kernel_no_pids_support = "WARNING: Your kernel
> does not support pids limit capabilities";
> static final String no_value_expected = "no_value_expected";
Oh, I misunderstood the test. "no_value_expected" was passed in from
`testPids()` in this file, but that's confusing because you are expecting an
integer value. Maybe it should be "any_integer"?
-
PR: https://git.openjdk.java.net/jdk/pull/5437
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 16:47:45 GMT, Daniel Fuchs wrote:
>> This change implements a simple web server that can be run on the
>> command-line with `java -m jdk.httpserver`.
>>
>> This is facilitated by adding an entry point for the `jdk.httpserver`
>> module, an implementation class whose main method is run when the above
>> command is executed. This is the first such module entry point in the JDK.
>>
>> The server is a minimal HTTP server that serves the static files of a given
>> directory, similar to existing alternatives on other platforms and
>> convenient for testing, development, and debugging.
>>
>> Additionally, a small API is introduced for programmatic creation and
>> customization.
>>
>> Testing: tier1-3.
>
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpHandlers.java
> line 129:
>
>> 127: * response body bytes are a {@code UTF-8} encoded byte
>> sequence of
>> 128: * {@code body}. The response {@linkplain
>> HttpExchange#sendResponseHeaders(int, long) is sent}
>> 129: * with the given {@code statusCode} and the body bytes' length.
>> The body
>
> That might give the impression that chunked encoding will be used if the body
> length is 0. I wonder if it should say instead:
>
>
> with the given {@code statusCode} and a {@code Content-Length} field set to
> the body bytes' length.
Or maybe - which would be more accurate:
with the given {@code statusCode} and the body bytes' length (or {@code -1} if
the body is empty).
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 08:52:37 GMT, Julia Boes wrote:
> This change implements a simple web server that can be run on the
> command-line with `java -m jdk.httpserver`.
>
> This is facilitated by adding an entry point for the `jdk.httpserver` module,
> an implementation class whose main method is run when the above command is
> executed. This is the first such module entry point in the JDK.
>
> The server is a minimal HTTP server that serves the static files of a given
> directory, similar to existing alternatives on other platforms and convenient
> for testing, development, and debugging.
>
> Additionally, a small API is introduced for programmatic creation and
> customization.
>
> Testing: tier1-3.
src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpHandlers.java line
129:
> 127: * response body bytes are a {@code UTF-8} encoded byte
> sequence of
> 128: * {@code body}. The response {@linkplain
> HttpExchange#sendResponseHeaders(int, long) is sent}
> 129: * with the given {@code statusCode} and the body bytes' length. The
> body
That might give the impression that chunked encoding will be used if the body
length is 0. I wonder if it should say instead:
with the given {@code statusCode} and a {@code Content-Length} field set to the
body bytes' length.
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 12:38:19 GMT, Jim Laskey wrote:
>> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpHandlers.java
>> line 128:
>>
>>> 126: * {@code headers} are the effective headers of the response.
>>> The
>>> 127: * response body bytes are a {@code UTF-8} encoded byte
>>> sequence of
>>> 128: * {@code body}. The response {@linkplain
>>> HttpExchange#sendResponseHeaders(int, long) is sent}
>>
>> Not sure what the javadoc looks like here, but it looks like the link is "is
>> sent". Curious.
>
> Just a rewording, maybe.
Hmm... Maye that should be "The response headers *are sent*". The non-obvious
technical details is that the response headers are sent before the body - as
soon as you call `sendResponseHeaders`. The link is here to provide a better
understanding of the workings of the new Handler. The headers are sent by
calling `sendResponseHeaders` on the HttpExchange.
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: JDK-8273526: Extend the OSContainer API pids controller with pids.current [v3]
On Tue, 14 Sep 2021 14:27:36 GMT, Matthias Baesken wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8266490
>> extended the OSContainer API in order to also support the pids controller of
>> cgroups. However only pids.max output was added with 8266490.
>> There is a second parameter pids.current , see
>> https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#pid
>> that would be helpful too and can be added to the OSContainer API .
>> pids.current :
>> A read-only single value file which exists on all cgroups.
>> The number of processes currently in the cgroup and its descendants.
>>
>> Best regards, Matthias
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Simplify coding following Severins advice
HotSpot changes look good to me. I have a comment on the test.
test/hotspot/jtreg/containers/docker/TestPids.java line 97:
> 95: System.out.println("DEBUG: parts.length = " +
> parts.length);
> 96: if (expectedValue.equals("no_value_expected")) {
> 97: Asserts.assertEquals(parts.length, 2);
Is "no_value_expected" generated by Docker? I searched the entire HotSpot
source code and couldn't find it. I also couldn't find "WARNING: Your kernel
does not support pids limit capabilities".
To make it easier to understand this test, I would suggest grouping all
messages that were generated outside of HotSpot into something like:
// These messages are generated by Docker
static final String warning_kernel_no_pids_support = "WARNING: Your kernel does
not support pids limit capabilities";
static final String no_value_expected = "no_value_expected";
-
PR: https://git.openjdk.java.net/jdk/pull/5437
Re: RFR: JDK-8273526: Extend the OSContainer API pids controller with pids.current [v3]
On Tue, 14 Sep 2021 14:27:36 GMT, Matthias Baesken wrote: >> https://bugs.openjdk.java.net/browse/JDK-8266490 >> extended the OSContainer API in order to also support the pids controller of >> cgroups. However only pids.max output was added with 8266490. >> There is a second parameter pids.current , see >> https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#pid >> that would be helpful too and can be added to the OSContainer API . >> pids.current : >> A read-only single value file which exists on all cgroups. >> The number of processes currently in the cgroup and its descendants. >> >> Best regards, Matthias > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Simplify coding following Severins advice Looks fine. I suggest to move the debug printing from line 99 to after line 101 as suggested here: https://github.com/openjdk/jdk/pull/5437#discussion_r706145165 - Marked as reviewed by sgehwolf (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5437
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 12:36:28 GMT, Jim Laskey wrote:
>> This change implements a simple web server that can be run on the
>> command-line with `java -m jdk.httpserver`.
>>
>> This is facilitated by adding an entry point for the `jdk.httpserver`
>> module, an implementation class whose main method is run when the above
>> command is executed. This is the first such module entry point in the JDK.
>>
>> The server is a minimal HTTP server that serves the static files of a given
>> directory, similar to existing alternatives on other platforms and
>> convenient for testing, development, and debugging.
>>
>> Additionally, a small API is introduced for programmatic creation and
>> customization.
>>
>> Testing: tier1-3.
>
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpHandlers.java
> line 128:
>
>> 126: * {@code headers} are the effective headers of the response.
>> The
>> 127: * response body bytes are a {@code UTF-8} encoded byte
>> sequence of
>> 128: * {@code body}. The response {@linkplain
>> HttpExchange#sendResponseHeaders(int, long) is sent}
>
> Not sure what the javadoc looks like here, but it looks like the link is "is
> sent". Curious.
Just a rewording, maybe.
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 08:52:37 GMT, Julia Boes wrote:
> This change implements a simple web server that can be run on the
> command-line with `java -m jdk.httpserver`.
>
> This is facilitated by adding an entry point for the `jdk.httpserver` module,
> an implementation class whose main method is run when the above command is
> executed. This is the first such module entry point in the JDK.
>
> The server is a minimal HTTP server that serves the static files of a given
> directory, similar to existing alternatives on other platforms and convenient
> for testing, development, and debugging.
>
> Additionally, a small API is introduced for programmatic creation and
> customization.
>
> Testing: tier1-3.
Very nice. LGTM.
src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpHandlers.java line
128:
> 126: * {@code headers} are the effective headers of the response. The
> 127: * response body bytes are a {@code UTF-8} encoded byte
> sequence of
> 128: * {@code body}. The response {@linkplain
> HttpExchange#sendResponseHeaders(int, long) is sent}
Not sure what the javadoc looks like here, but it looks like the link is "is
sent". Curious.
src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsServer.java line
152:
> 150: return server;
> 151: }
> 152:
Too bad we couldn't simplify the setting up a basic certificate for https.
-
Marked as reviewed by jlaskey (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 16:07:21 GMT, Julia Boes wrote: >> src/jdk.httpserver/share/classes/com/sun/net/httpserver/Headers.java line >> 288: >> >>> 286: } >>> 287: >>> 288: private static final Headers EMPTY = new UnmodifiableHeaders(new >>> Headers()); >> >> IDEA warns here: >>>Referencing subclass UnmodifiableHeaders from superclass Headers initializer >>>might lead to class loading deadlock >>>Such references can cause JVM-level deadlocks in multithreaded environment, >>>when one thread tries to load the superclass and another thread tries to >>>load the subclass at the same time. > > Interesting, thanks for noting. We can return a new instance instead on line > 307, the only place the constant is used. Or you could possibly define the constant in UnmodifiableHeaders instead. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: JDK-8273040: Turning off JpAllowDowngrades (or Upgrades)
On Tue, 14 Sep 2021 13:38:42 GMT, Andy Herrick wrote: > JDK-8273040: Turning off JpAllowDowngrades (or Upgrades) Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5509
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 13:47:48 GMT, Andrey Turbanov wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > src/jdk.httpserver/share/classes/com/sun/net/httpserver/Headers.java line 288: > >> 286: } >> 287: >> 288: private static final Headers EMPTY = new UnmodifiableHeaders(new >> Headers()); > > IDEA warns here: >>Referencing subclass UnmodifiableHeaders from superclass Headers initializer >>might lead to class loading deadlock >>Such references can cause JVM-level deadlocks in multithreaded environment, >>when one thread tries to load the superclass and another thread tries to load >>the subclass at the same time. Interesting, thanks for noting. We can return a new instance instead on line 307, the only place the constant is used. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 15:42:06 GMT, Jaikiran Pai wrote: >> src/java.base/windows/classes/sun/net/www/content-types.properties line 30: >> >>> 28: application/octet-stream: \ >>> 29: description=Generic Binary Stream;\ >>> 30: file_extensions=.saveme,.dump,.hqx,.arc,.obj,.lib,.bin,.exe,.gz >> >> Hello Julia, >> Is this an intentional change, to remove the mapping of `.zip` to >> `application/octet-stream`? In a later part of this patch there's a >> commented out test `testCommonExtensions` which deals with these extension >> types and that has a link to >> https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types >> which states that `.zip` should be mapped to `application/zip` instead of >> the current `application/octet-stream`, so I'm guessing this changed line is >> intentional. >> >> On an unrelated note, the unix variant of this file >> `src/java.base/unix/classes/sun/net/www/content-types.properties` >> interestingly uses `.z` for `.zip`? Commit history on that file doesn't >> provide any hint on whether that is intentional either. > > I think you can ignore my comment above. I went and checked the > `content-types.properties` in their current state for both unix and windows > and they already have a separate `application/zip` which is mapped to `.zip`. > So I think this above change shouldn't impact anything. That's right, there was a duplicate entry for `.zip` in the Windows properties file only, which I removed. I'm not sure if `.z` in the Unix properties file is intentional, but I do have a PR in progress in the same area, which I will link here shortly. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"
On Fri, 10 Sep 2021 10:02:43 GMT, David Holmes wrote: >> Currently, the build system defaults the libjvm.so location to "server". >> This makes looking for `libjvm.so` awkward, see JDK-8273487 for example. We >> need to see if moving the libjvm.so to a proper location breaks anything. >> >> Additional testing: >> - [x] Linux x86_64 Zero build >> - [x] Linux x86_64 Zero bootcycle-images >> - [x] Linux x86_64 Zero `tier1` > > It isn't the "formal governance" I'm concerned about, more about the folk who > use/rely on Zero being the ones to evaluate the impact of a change like this. > People, like myself, who do not use Zero in any way cannot evaluate whether > this change is appropriate - it's that simple. :) I'll be happy to integrate as long as @dholmes-ora has no objections. - PR: https://git.openjdk.java.net/jdk/pull/5440
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 15:30:31 GMT, Jaikiran Pai wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > src/java.base/windows/classes/sun/net/www/content-types.properties line 30: > >> 28: application/octet-stream: \ >> 29: description=Generic Binary Stream;\ >> 30: file_extensions=.saveme,.dump,.hqx,.arc,.obj,.lib,.bin,.exe,.gz > > Hello Julia, > Is this an intentional change, to remove the mapping of `.zip` to > `application/octet-stream`? In a later part of this patch there's a commented > out test `testCommonExtensions` which deals with these extension types and > that has a link to > https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types > which states that `.zip` should be mapped to `application/zip` instead of > the current `application/octet-stream`, so I'm guessing this changed line is > intentional. > > On an unrelated note, the unix variant of this file > `src/java.base/unix/classes/sun/net/www/content-types.properties` > interestingly uses `.z` for `.zip`? Commit history on that file doesn't > provide any hint on whether that is intentional either. I think you can ignore my comment above. I went and checked the `content-types.properties` in their current state for both unix and windows and they already have a separate `application/zip` which is mapped to `.zip`. So I think this above change shouldn't impact anything. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 08:52:37 GMT, Julia Boes wrote: > This change implements a simple web server that can be run on the > command-line with `java -m jdk.httpserver`. > > This is facilitated by adding an entry point for the `jdk.httpserver` module, > an implementation class whose main method is run when the above command is > executed. This is the first such module entry point in the JDK. > > The server is a minimal HTTP server that serves the static files of a given > directory, similar to existing alternatives on other platforms and convenient > for testing, development, and debugging. > > Additionally, a small API is introduced for programmatic creation and > customization. > > Testing: tier1-3. src/java.base/windows/classes/sun/net/www/content-types.properties line 30: > 28: application/octet-stream: \ > 29: description=Generic Binary Stream;\ > 30: file_extensions=.saveme,.dump,.hqx,.arc,.obj,.lib,.bin,.exe,.gz Hello Julia, Is this an intentional change, to remove the mapping of `.zip` to `application/octet-stream`? In a later part of this patch there's a commented out test `testCommonExtensions` which deals with these extension types and that has a link to https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types which states that `.zip` should be mapped to `application/zip` instead of the current `application/octet-stream`, so I'm guessing this changed line is intentional. On an unrelated note, the unix variant of this file `src/java.base/unix/classes/sun/net/www/content-types.properties` interestingly uses `.z` for `.zip`? Commit history on that file doesn't provide any hint on whether that is intentional either. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8273711: Remove redundant stream() call before forEach in jdk.jlink
On Mon, 13 Sep 2021 20:06:08 GMT, Andrey Turbanov
wrote:
> 8273711: Remove redundant stream() call before forEach in jdk.jlink
In some cases you can use a method ref (no need for another review if you
update those).
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/ReleaseInfoPlugin.java
line 102:
> 100: throw new IllegalArgumentException("No key specified
> for delete");
> 101: }
> 102: Utils.parseList(keys).forEach((k) -> {
Use method ref: `release::remove`
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5500
Re: RFR: 8273402: Use derived NamingExceptions in com.sun.jndi.ldap.Connection#readReply
On Thu, 9 Sep 2021 22:02:55 GMT, Aleksei Efimov wrote: > Hi, > The following fix changes type of exception thrown when an LDAP operation > fails for reasons like read timeout or connection closure/cancellation: > instead of throwing a general `NamingException`, the internal LDAP connection > class will throw a > [`CommunicationException`](https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/javax/naming/CommunicationException.java#L29) > that better matches the reasons described. > > Testing shows no problem with the proposed fix. Thanks, will do. - PR: https://git.openjdk.java.net/jdk/pull/5456
Re: RFR: JDK-8273526: Extend the OSContainer API pids controller with pids.current [v2]
On Fri, 10 Sep 2021 11:07:52 GMT, Matthias Baesken wrote: >> https://bugs.openjdk.java.net/browse/JDK-8266490 >> extended the OSContainer API in order to also support the pids controller of >> cgroups. However only pids.max output was added with 8266490. >> There is a second parameter pids.current , see >> https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#pid >> that would be helpful too and can be added to the OSContainer API . >> pids.current : >> A read-only single value file which exists on all cgroups. >> The number of processes currently in the cgroup and its descendants. >> >> Best regards, Matthias > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Some simplifications and test adjustment suggested by Severin Hi Severin, I adjusted my change following your latest comments. Thanks, Matthias - PR: https://git.openjdk.java.net/jdk/pull/5437
Re: RFR: JDK-8273526: Extend the OSContainer API pids controller with pids.current [v3]
> https://bugs.openjdk.java.net/browse/JDK-8266490 > extended the OSContainer API in order to also support the pids controller of > cgroups. However only pids.max output was added with 8266490. > There is a second parameter pids.current , see > https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#pid > that would be helpful too and can be added to the OSContainer API . > pids.current : > A read-only single value file which exists on all cgroups. > The number of processes currently in the cgroup and its descendants. > > Best regards, Matthias Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Simplify coding following Severins advice - Changes: - all: https://git.openjdk.java.net/jdk/pull/5437/files - new: https://git.openjdk.java.net/jdk/pull/5437/files/422aef68..afe0efd4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5437=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5437=01-02 Stats: 31 lines in 6 files changed: 1 ins; 27 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5437/head:pull/5437 PR: https://git.openjdk.java.net/jdk/pull/5437
Re: RFR: 8273402: Use derived NamingExceptions in com.sun.jndi.ldap.Connection#readReply
On Fri, 10 Sep 2021 17:04:58 GMT, Michael Osipov wrote: >> Hi, >> The following fix changes type of exception thrown when an LDAP operation >> fails for reasons like read timeout or connection closure/cancellation: >> instead of throwing a general `NamingException`, the internal LDAP >> connection class will throw a >> [`CommunicationException`](https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/javax/naming/CommunicationException.java#L29) >> that better matches the reasons described. >> >> Testing shows no problem with the proposed fix. > > @AlekseiEfimov Thanks for making this happen so fast. Do you see any chance > to have this backported to LTS releases where this is required? I have > reported the issue initially and like to make this available in Tomcat 8.5+. Hi @michael-o, I'm not involved in the backporting process to older releases. You'll need to find the right contact for a particular release you need a backport for, and ask there. - PR: https://git.openjdk.java.net/jdk/pull/5456
Re: RFR: 8231640: (prop) Canonical property storage [v14]
On Tue, 14 Sep 2021 13:28:24 GMT, Daniel Fuchs wrote:
>> There has been no change in how we deal with this aspect. The existing
>> specification (stated in the `load` method) says:
>>
>>
>>> * Properties are processed in terms of lines. There are two
>>> * kinds of line, natural lines and logical lines.
>>> * A natural line is defined as a line of
>>> * characters that is terminated either by a set of line terminator
>>> * characters ({@code \n} or {@code \r} or {@code \r\n})
>>> * or by the end of the stream. A natural line may be either a blank
>>> line,
>>> * a comment line, or hold all or some of a key-element pair. A logical
>>> * line holds all the data of a key-element pair, which may be spread
>>> * out across several adjacent natural lines by escaping
>>> * the line terminator sequence with a backslash character
>>> * {@code }. **Note that a comment line cannot be extended
>>> * in this manner;**
>> (emphasis on that last sentence).
>> I'll anyway go ahead and add new tests around this to be sure that this
>> works as advertised.
>
> Oh - great - thanks - verifying by a test that it also applies to the comment
> specified by `java.util.Properties.storeDate` would be good.
Done. A new test `StoreReproducibilityTest#testBackSlashInStoreDateValue` has
been added in the latest updated version of this PR. This test passes. Plus I
checked the written out properties file, from these tests, for such values in
`java.util.Properties.storeDate` and the content matches what the spec says.
Just for quick reference - a run of that test case with the
"newline-plus-backslash" system property value (cannot paste that exact
string value from that test case because GitHub editor is messing up the
special characters) generates output like below:
#some user specified comment
#newline-plus-backslash\
#c=d
a=b
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v15]
> 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: Introduce a test to make sure backslash character in the system property value doesn't cause unexpected output. Plus minor updates to tests to add additional checks. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/14711a92..6f5f1be2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5372=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5372=13-14 Stats: 65 lines in 1 file changed: 64 ins; 0 del; 1 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: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 08:52:37 GMT, Julia Boes wrote: > This change implements a simple web server that can be run on the > command-line with `java -m jdk.httpserver`. > > This is facilitated by adding an entry point for the `jdk.httpserver` module, > an implementation class whose main method is run when the above command is > executed. This is the first such module entry point in the JDK. > > The server is a minimal HTTP server that serves the static files of a given > directory, similar to existing alternatives on other platforms and convenient > for testing, development, and debugging. > > Additionally, a small API is introduced for programmatic creation and > customization. > > Testing: tier1-3. src/jdk.httpserver/share/classes/com/sun/net/httpserver/Headers.java line 288: > 286: } > 287: > 288: private static final Headers EMPTY = new UnmodifiableHeaders(new > Headers()); IDEA warns here: >Referencing subclass UnmodifiableHeaders from superclass Headers initializer >might lead to class loading deadlock >Such references can cause JVM-level deadlocks in multithreaded environment, >when one thread tries to load the superclass and another thread tries to load >the subclass at the same time. - PR: https://git.openjdk.java.net/jdk/pull/5505
RFR: JDK-8273040: Turning off JpAllowDowngrades (or Upgrades)
JDK-8273040: Turning off JpAllowDowngrades (or Upgrades) - Commit messages: - JDK-8273040: Turning off JpAllowDowngrades (or Upgrades) - JDK-8273040: Turning off JpAllowDowngrades (or Upgrades) Changes: https://git.openjdk.java.net/jdk/pull/5509/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5509=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273040 Stats: 17 lines in 3 files changed: 7 ins; 5 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5509.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5509/head:pull/5509 PR: https://git.openjdk.java.net/jdk/pull/5509
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 13:19:17 GMT, Andrey Turbanov wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > src/jdk.httpserver/share/classes/com/sun/net/httpserver/Headers.java line 106: > >> 104: var h = headers.entrySet().stream() >> 105: .collect(Collectors.toUnmodifiableMap( >> 106: Entry::getKey, e -> new >> LinkedList<>(e.getValue(; > > I wonder, what the reason of `LinkedList` usages here? > As I know ArrayList is faster in all real-world scenarios. Hi Andrey, IIRC from when I reviewed this code the current implementation of `Headers` already uses `LinkedList` in other places - so this preserves the concrete list implementation class that `Headers` uses (see `Headers::add`). Not that it matters much - but if we wanted to replace `LinkedList` with `ArrayList` I believe we should do it consistently in this class - and probably outside of the realm of this JEP. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8231640: (prop) Canonical property storage [v14]
On Tue, 14 Sep 2021 13:24:55 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/java/util/Properties.java line 822:
>>
>>> 820: * {@link System#lineSeparator() line separator} and if the next
>>> 821: * character in comments is not character {@code #} or character
>>> {@code !} then
>>> 822: * an ASCII {@code #} is written out after that line separator.
>>
>> Should something be done for comments ending with \ (backslash) ? It might
>> otherwise suppress the first property assignment that follows.
>
> There has been no change in how we deal with this aspect. The existing
> specification (stated in the `load` method) says:
>
>
>> * Properties are processed in terms of lines. There are two
>> * kinds of line, natural lines and logical lines.
>> * A natural line is defined as a line of
>> * characters that is terminated either by a set of line terminator
>> * characters ({@code \n} or {@code \r} or {@code \r\n})
>> * or by the end of the stream. A natural line may be either a blank line,
>> * a comment line, or hold all or some of a key-element pair. A logical
>> * line holds all the data of a key-element pair, which may be spread
>> * out across several adjacent natural lines by escaping
>> * the line terminator sequence with a backslash character
>> * {@code }. **Note that a comment line cannot be extended
>> * in this manner;**
> (emphasis on that last sentence).
> I'll anyway go ahead and add new tests around this to be sure that this works
> as advertised.
Oh - great - thanks - verifying by a test that it also applies to the comment
specified by `java.util.Properties.storeDate` would be good.
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v14]
On Tue, 14 Sep 2021 13:17:42 GMT, Daniel Fuchs wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Add a @implNote to specify the order in which the properties are written
>> out
>> - update the javadoc to clarify how line terminator characters are handled
>> in the value of the java.util.Properties.storeDate system property
>
> src/java.base/share/classes/java/util/Properties.java line 822:
>
>> 820: * {@link System#lineSeparator() line separator} and if the next
>> 821: * character in comments is not character {@code #} or character
>> {@code !} then
>> 822: * an ASCII {@code #} is written out after that line separator.
>
> Should something be done for comments ending with \ (backslash) ? It might
> otherwise suppress the first property assignment that follows.
There has been no change in how we deal with this aspect. The existing
specification (stated in the `load` method) says:
> * Properties are processed in terms of lines. There are two
> * kinds of line, natural lines and logical lines.
> * A natural line is defined as a line of
> * characters that is terminated either by a set of line terminator
> * characters ({@code \n} or {@code \r} or {@code \r\n})
> * or by the end of the stream. A natural line may be either a blank line,
> * a comment line, or hold all or some of a key-element pair. A logical
> * line holds all the data of a key-element pair, which may be spread
> * out across several adjacent natural lines by escaping
> * the line terminator sequence with a backslash character
> * {@code }. **Note that a comment line cannot be extended
> * in this manner;**
(emphasis on that last sentence).
I'll anyway go ahead and add new tests around this to be sure that this works
as advertised.
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 13:15:18 GMT, Jaikiran Pai wrote: >> I would leave it as an `@implNote` - or possibly `@implSpec`: depending on >> whether or not we want all implementations of the spec to behave in this >> way. However I don't think we would want to prevent subclasses from >> overriding this behavior and using their own business-logic ordering. So I >> believe the default behavior should be specified, if only so that subclasses >> can decide whether or not to override this method, without invalidating what >> subclasses might currently have implemented - or might wish to implement in >> the future. The CSR will be a good way to get feedback on whether >> `@implNote` or `@implSpec` is more appropriate. Also this is a change in >> behavior that needs to be made visible somewhere - and nobody reads release >> notes ;-) > > Done. I've updated the PR to include a `@implNote` specifying the order of > the properties. The CSR has been updated too to reflect this. Thanks Jaikiran, - the `@implNote` looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 08:52:37 GMT, Julia Boes wrote: > This change implements a simple web server that can be run on the > command-line with `java -m jdk.httpserver`. > > This is facilitated by adding an entry point for the `jdk.httpserver` module, > an implementation class whose main method is run when the above command is > executed. This is the first such module entry point in the JDK. > > The server is a minimal HTTP server that serves the static files of a given > directory, similar to existing alternatives on other platforms and convenient > for testing, development, and debugging. > > Additionally, a small API is introduced for programmatic creation and > customization. > > Testing: tier1-3. src/jdk.httpserver/share/classes/com/sun/net/httpserver/Headers.java line 106: > 104: var h = headers.entrySet().stream() > 105: .collect(Collectors.toUnmodifiableMap( > 106: Entry::getKey, e -> new > LinkedList<>(e.getValue(; I wonder, what the reason of `LinkedList` usages here? As I know ArrayList is faster in all real-world scenarios. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 12:56:28 GMT, Daniel Fuchs wrote: >> Hello Daniel, you are right - I missed discussing whether or not to include >> a `@implNote` to explain the natural sorted order of the property keys. When >> we started this whole PR proposal, Alan had hinted that maybe we should >> "specify" this behaviour. Should this be a `@implNote` or should this be >> part of the formal spec like we did for the system property handling? > > I would leave it as an `@implNote` - or possibly `@implSpec`: depending on > whether or not we want all implementations of the spec to behave in this way. > However I don't think we would want to prevent subclasses from overriding > this behavior and using their own business-logic ordering. So I believe the > default behavior should be specified, if only so that subclasses can decide > whether or not to override this method, without invalidating what subclasses > might currently have implemented - or might wish to implement in the future. > The CSR will be a good way to get feedback on whether `@implNote` or > `@implSpec` is more appropriate. Also this is a change in behavior that needs > to be made visible somewhere - and nobody reads release notes ;-) Done. I've updated the PR to include a `@implNote` specifying the order of the properties. The CSR has been updated too to reflect this. - PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 11:43:25 GMT, Magnus Ihse Bursie wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> unused imports
>
> src/java.base/share/classes/java/util/Properties.java line 828:
>
>> 826: * a comment line is written as follows.
>> 827: * First, a {@code #} character is written, followed by the contents
>> 828: * of the property, followed by a line separator.
>
> Maybe you should refer to how the comment is written out above, since you use
> the same method. The spec changes as written above does not specify how
> newlines in the property would be handled (which is possible to get, I
> believe, even if it means an intricate command line escape dance)
Hello Magnus,
> Maybe you should refer to how the comment is written out above, since you use
> the same method.
I've updated the javadoc to capture this part of the detail.
> The spec changes as written above does not specify how newlines in the
> property would be handled (which is possible to get, I believe, even if it
> means an intricate command line escape dance)
The new tests that were added in this PR has one specific test to verify how
line terminators are dealt with, if the system property value has them
https://github.com/openjdk/jdk/pull/5372/files#diff-220f7bcc6d1a7ec33764f81eb95ccb0f69444e1eb923b015025e2140c3ffe145R280
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v14]
On Tue, 14 Sep 2021 13:19:25 GMT, 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.
>>
>> 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 two additional
> commits since the last revision:
>
> - Add a @implNote to specify the order in which the properties are written
> out
> - update the javadoc to clarify how line terminator characters are handled
> in the value of the java.util.Properties.storeDate system property
src/java.base/share/classes/java/util/Properties.java line 822:
> 820: * {@link System#lineSeparator() line separator} and if the next
> 821: * character in comments is not character {@code #} or character
> {@code !} then
> 822: * an ASCII {@code #} is written out after that line separator.
Should something be done for comments ending with \ (backslash) ? It might
otherwise suppress the first property assignment that follows.
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v14]
> 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 two additional commits since the last revision: - Add a @implNote to specify the order in which the properties are written out - update the javadoc to clarify how line terminator characters are handled in the value of the java.util.Properties.storeDate system property - Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/92374664..14711a92 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5372=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5372=12-13 Stats: 12 lines in 1 file changed: 5 ins; 0 del; 7 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: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 11:59:56 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/java/util/Properties.java line 836:
>>
>>> 834: *
>>> 835: * Then every entry in this {@code Properties} table is
>>> 836: * written out, one per line. For each entry the key string is
>>
>> Sorry for coming late to the party: I don't remember if this was already
>> asked for and rejected - but shouldn't there be a non-normative `@implNote`
>> here to state that the default implementation of this method will write the
>> properties sorted by their keys in alphanumeric ordering?
>> Otherwise this looks very good!
>
> Hello Daniel, you are right - I missed discussing whether or not to include a
> `@implNote` to explain the natural sorted order of the property keys. When we
> started this whole PR proposal, Alan had hinted that maybe we should
> "specify" this behaviour. Should this be a `@implNote` or should this be part
> of the formal spec like we did for the system property handling?
I would leave it as an `@implNote` - or possibly `@implSpec`: depending on
whether or not we want all implementations of the spec to behave in this way.
However I don't think we would want to prevent subclasses from overriding this
behavior and using their own business-logic ordering. So I believe the default
behavior should be specified, if only so that subclasses can decide whether or
not to override this method, without invalidating what subclasses might
currently have implemented - or might wish to implement in the future. The CSR
will be a good way to get feedback on whether `@implNote` or `@implSpec` is
more appropriate. Also this is a change in behavior that needs to be made
visible somewhere - and nobody reads release notes ;-)
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Integrated: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict
On Thu, 9 Sep 2021 23:29:24 GMT, Naoto Sato wrote: > Small spec clarification. Corresponding CSR has also been drafted. This pull request has now been integrated. Changeset: 31667daa Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/31667daa50b2faf82943821ee02071d222e38268 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict Reviewed-by: joehw, lancea - PR: https://git.openjdk.java.net/jdk/pull/5457
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 02:30:01 GMT, 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. >> >> 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: > > unused imports I've created a CSR https://bugs.openjdk.java.net/browse/JDK-8273727 and added the details. I'll update that javadoc to include the properties order specification too once we decide whether it makes sense to have it in `@implNote` or as the main text. - PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"
On Thu, 9 Sep 2021 10:17:02 GMT, Aleksey Shipilev wrote: > Currently, the build system defaults the libjvm.so location to "server". > This makes looking for `libjvm.so` awkward, see JDK-8273487 for example. We > need to see if moving the libjvm.so to a proper location breaks anything. > > Additional testing: > - [x] Linux x86_64 Zero build > - [x] Linux x86_64 Zero bootcycle-images > - [ ] Linux x86_64 Zero `tier1` Looks fine to me. - Marked as reviewed by sgehwolf (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5440
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:30:34 GMT, wxiang wrote: >> 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 @shiyuexw I discussed this issue with other maintainers in this area. There was agreement on dropping the support from the URLClassLoader implementation but it was suggested that it should be disabled for a release or two before the code is removed. A system property can be used to re-enable. I've updated the CSR to reflect this proposal. Are you okay to continue with this new proposal? It would mean that we wouldn't remove the tests but instead change them to run with the system property. - PR: https://git.openjdk.java.net/jdk/pull/5383
Withdrawn: 8270057: Use Objects.checkFromToIndex for j.u.c.CopyOnWriteArrayList
On Thu, 8 Jul 2021 11:51:18 GMT, Yi Yang wrote: > After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. > > As Mandy suggested, I create this PR for changes involving JUC changes. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4723
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 10:47:34 GMT, Daniel Fuchs wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> unused imports
>
> src/java.base/share/classes/java/util/Properties.java line 836:
>
>> 834: *
>> 835: * Then every entry in this {@code Properties} table is
>> 836: * written out, one per line. For each entry the key string is
>
> Sorry for coming late to the party: I don't remember if this was already
> asked for and rejected - but shouldn't there be a non-normative `@implNote`
> here to state that the default implementation of this method will write the
> properties sorted by their keys in alphanumeric ordering?
> Otherwise this looks very good!
Hello Daniel, you are right - I missed discussing whether or not to include a
`@implNote` to explain the natural sorted order of the property keys. When we
started this whole PR proposal, Alan had hinted that maybe we should "specify"
this behaviour. Should this be a `@implNote` or should this be part of the
formal spec like we did for the system property handling?
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
On Tue, 14 Sep 2021 10:57:17 GMT, Alan Bateman wrote: >>> Hi @iaroslavski I'm unconvinced that this work was from 14/06/2020 - I >>> believe this work derives from an unsigned radix sort I implemented on >>> 10/04/2021 >>> [richardstartin/radix-sort-benchmark@ab4da23#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226](https://github.com/richardstartin/radix-sort-benchmark/commit/ab4da230e1d0ac68e5ee2cee38d71c7e7d50f49b#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226) >>> which has numerous structural similarities to this work: >>> Moreover, @bourgesl forked my repository on 11/04/2021 and communicated >>> with me about doing so. On 25/04/2021 there was a new implementation of >>> `DualPivotQuicksort` with a signed radix sort but the same structural >>> similarities, and with the same method and variable names in places >>> [bourgesl/radix-sort-benchmark@90ff7e4#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609](https://github.com/bourgesl/radix-sort-benchmark/commit/90ff7e427da0fa49f374bff0241fb2487bd87bde#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609) >> >> @iaroslavski The attribution is not clear here. Can you provide a summary as >> to who is contributing to this patch? I can't tell if all involved have >> signed the OCA or not. I'm sure there will be questions about space/time >> trade-offs with radix sort but I think it's important to first establish the >> origins of this patch first. > >> @AlanBateman Vertical pipeline of PR hides comments in the middle and you >> have to click on "Show more..." to see all comments. There are no claims >> related to the origin of my patch, it doesn't violate any rights. > > There is a comment from richardstartin suggesting that something was derived > from code in his repo. Is this a benchmark that is not part of this PR? Only > asking because I can't find him on OCA signatories. You can use the Skara > /contributor command to list the contributors. @AlanBateman my claim was that the implementation was derived from my implementation, and demonstrated a sequence of name changes after @bourgesl forked my repository containing a structurally similar radix sort implementation and benchmarks, in order to provide circumstantial evidence for my claim. Via email @iaroslavski told me that this was not the case, which I decided to accept at face value. So please judge this PR on its merits, and disregard the claims made in these comments. I have not signed an OCA but do not want to block this PR if the space time tradeoff is deemed acceptable. - PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 02:30:01 GMT, 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.
>>
>> 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:
>
> unused imports
src/java.base/share/classes/java/util/Properties.java line 828:
> 826: * a comment line is written as follows.
> 827: * First, a {@code #} character is written, followed by the contents
> 828: * of the property, followed by a line separator.
Maybe you should refer to how the comment is written out above, since you use
the same method. The spec changes as written above does not specify how
newlines in the property would be handled (which is possible to get, I believe,
even if it means an intricate command line escape dance)
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"
On Thu, 9 Sep 2021 10:17:02 GMT, Aleksey Shipilev wrote: > Currently, the build system defaults the libjvm.so location to "server". > This makes looking for `libjvm.so` awkward, see JDK-8273487 for example. We > need to see if moving the libjvm.so to a proper location breaks anything. > > Additional testing: > - [x] Linux x86_64 Zero build > - [x] Linux x86_64 Zero bootcycle-images > - [ ] Linux x86_64 Zero `tier1` I'm changing my response to "approve". I agree that we can change zero only in this patch to facilitate backporting, and fix the remaining odds and ends in a separate PR. And also let me be clear that I see no reason to delay pushing this any further. Everyone who has any vested interest in zero has had ample chance to comment on the PR. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5440
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server
On Tue, 14 Sep 2021 08:52:37 GMT, Julia Boes wrote: > This change implements a simple web server that can be run on the > command-line with `java -m jdk.httpserver`. > > This is facilitated by adding an entry point for the `jdk.httpserver` module, > an implementation class whose main method is run when the above command is > executed. This is the first such module entry point in the JDK. > > The server is a minimal HTTP server that serves the static files of a given > directory, similar to existing alternatives on other platforms and convenient > for testing, development, and debugging. > > Additionally, a small API is introduced for programmatic creation and > customization. > > Testing: tier1-3. Build changes look good. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8273710: Remove redundant stream() call before forEach in jdk.jdeps
On Mon, 13 Sep 2021 19:57:14 GMT, Andrey Turbanov wrote: > 8273710: Remove redundant stream() call before forEach in jdk.jdeps LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5498
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
On Tue, 14 Sep 2021 10:57:17 GMT, Alan Bateman wrote: >>> Hi @iaroslavski I'm unconvinced that this work was from 14/06/2020 - I >>> believe this work derives from an unsigned radix sort I implemented on >>> 10/04/2021 >>> [richardstartin/radix-sort-benchmark@ab4da23#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226](https://github.com/richardstartin/radix-sort-benchmark/commit/ab4da230e1d0ac68e5ee2cee38d71c7e7d50f49b#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226) >>> which has numerous structural similarities to this work: >>> Moreover, @bourgesl forked my repository on 11/04/2021 and communicated >>> with me about doing so. On 25/04/2021 there was a new implementation of >>> `DualPivotQuicksort` with a signed radix sort but the same structural >>> similarities, and with the same method and variable names in places >>> [bourgesl/radix-sort-benchmark@90ff7e4#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609](https://github.com/bourgesl/radix-sort-benchmark/commit/90ff7e427da0fa49f374bff0241fb2487bd87bde#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609) >> >> @iaroslavski The attribution is not clear here. Can you provide a summary as >> to who is contributing to this patch? I can't tell if all involved have >> signed the OCA or not. I'm sure there will be questions about space/time >> trade-offs with radix sort but I think it's important to first establish the >> origins of this patch first. > >> @AlanBateman Vertical pipeline of PR hides comments in the middle and you >> have to click on "Show more..." to see all comments. There are no claims >> related to the origin of my patch, it doesn't violate any rights. > > There is a comment from richardstartin suggesting that something was derived > from code in his repo. Is this a benchmark that is not part of this PR? Only > asking because I can't find him on OCA signatories. You can use the Skara > /contributor command to list the contributors. Mentioned benchmarking is not a part of this PR. Our benchmarking was done by Laurent. - PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
On Tue, 14 Sep 2021 09:23:23 GMT, Alan Bateman wrote: >> @richardstartin And one more addon: my first version of Radix sort, see my >> github https://github.com/iaroslavski/sorting/tree/master/radixsort uses >> another name, like skipBytes, then renamed to passLevel. >> So, the common part is "skip". And this method has different number of >> parameters. I don't see any collision with your code. > >> Hi @iaroslavski I'm unconvinced that this work was from 14/06/2020 - I >> believe this work derives from an unsigned radix sort I implemented on >> 10/04/2021 >> [richardstartin/radix-sort-benchmark@ab4da23#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226](https://github.com/richardstartin/radix-sort-benchmark/commit/ab4da230e1d0ac68e5ee2cee38d71c7e7d50f49b#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226) >> which has numerous structural similarities to this work: >> Moreover, @bourgesl forked my repository on 11/04/2021 and communicated with >> me about doing so. On 25/04/2021 there was a new implementation of >> `DualPivotQuicksort` with a signed radix sort but the same structural >> similarities, and with the same method and variable names in places >> [bourgesl/radix-sort-benchmark@90ff7e4#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609](https://github.com/bourgesl/radix-sort-benchmark/commit/90ff7e427da0fa49f374bff0241fb2487bd87bde#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609) > > @iaroslavski The attribution is not clear here. Can you provide a summary as > to who is contributing to this patch? I can't tell if all involved have > signed the OCA or not. I'm sure there will be questions about space/time > trade-offs with radix sort but I think it's important to first establish the > origins of this patch first. > @AlanBateman Vertical pipeline of PR hides comments in the middle and you > have to click on "Show more..." to see all comments. There are no claims > related to the origin of my patch, it doesn't violate any rights. There is a comment from richardstartin suggesting that something was derived from code in his repo. Is this a benchmark that is not part of this PR? Only asking because I can't find him on OCA signatories. You can use the Skara /contributor command to list the contributors. - PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: 8231640: (prop) Canonical property storage [v3]
On Wed, 8 Sep 2021 09:26:33 GMT, 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.
>>
>> 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 two additional
> commits since the last revision:
>
> - adjust testcases to verify the new semantics
> - implement review suggestions:
> - Use doPriveleged instead of explicit permission checks, to reduce
> complexity
> - Use DateTimeFormatter and ZonedDateTime instead of Date.toString()
> - Use DateTimeFormatter.RFC_1123_DATE_TIME for formatting
> SOURCE_DATE_EPOCH dates
> - Use Arrays.sort instead of a TreeMap for ordering property keys
src/java.base/share/classes/java/util/Properties.java line 886:
> 884: * In the presence of a SecurityManager, if the caller doesn't have
> permission
> 885: * to read the {@code SOURCE_DATE_EPOCH} environment variable, then
> the current date
> 886: * and time will be written. Similarly, if the value set for {@code
> SOURCE_DATE_EPOCH}
This is no longer true - is it? It seems you changed the code but not the spec
:-)
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v13]
On Tue, 14 Sep 2021 02:30:01 GMT, 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.
>>
>> 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:
>
> unused imports
src/java.base/share/classes/java/util/Properties.java line 836:
> 834: *
> 835: * Then every entry in this {@code Properties} table is
> 836: * written out, one per line. For each entry the key string is
Sorry for coming late to the party: I don't remember if this was already asked
for and rejected - but shouldn't there be a non-normative `@implNote` here to
state that the default implementation of this method will write the properties
sorted by their keys in alphanumeric ordering?
Otherwise this looks very good!
-
PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v10]
On 14/09/2021 03:28, Jaikiran Pai wrote: Hello Roger, I've now updated the PR to implement these suggested changes. I think at this point all suggestions have been implemented and I don't think there are any open questions. If the latest PR looks fine, I think the next step will be a CSR creation. Thanks for hanging in there, I think you've got this to a good place. The system property and resulting behavior look good so you should be able to create the CSR. -Alan
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
On Tue, 14 Sep 2021 09:23:23 GMT, Alan Bateman wrote: >> @richardstartin And one more addon: my first version of Radix sort, see my >> github https://github.com/iaroslavski/sorting/tree/master/radixsort uses >> another name, like skipBytes, then renamed to passLevel. >> So, the common part is "skip". And this method has different number of >> parameters. I don't see any collision with your code. > >> Hi @iaroslavski I'm unconvinced that this work was from 14/06/2020 - I >> believe this work derives from an unsigned radix sort I implemented on >> 10/04/2021 >> [richardstartin/radix-sort-benchmark@ab4da23#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226](https://github.com/richardstartin/radix-sort-benchmark/commit/ab4da230e1d0ac68e5ee2cee38d71c7e7d50f49b#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226) >> which has numerous structural similarities to this work: >> Moreover, @bourgesl forked my repository on 11/04/2021 and communicated with >> me about doing so. On 25/04/2021 there was a new implementation of >> `DualPivotQuicksort` with a signed radix sort but the same structural >> similarities, and with the same method and variable names in places >> [bourgesl/radix-sort-benchmark@90ff7e4#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609](https://github.com/bourgesl/radix-sort-benchmark/commit/90ff7e427da0fa49f374bff0241fb2487bd87bde#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609) > > @iaroslavski The attribution is not clear here. Can you provide a summary as > to who is contributing to this patch? I can't tell if all involved have > signed the OCA or not. I'm sure there will be questions about space/time > trade-offs with radix sort but I think it's important to first establish the > origins of this patch first. @AlanBateman There was discussion with Richard Startin, where Laurent and I explained the origin of my patch. There was misunderstanding based on git changes only. Finally Richard wrote comment in PR on May 13, 2021, I duplicate it here: "In private correspondence with Vladimir, it was explained that where Vladimir's code and Laurent's code are identical, including typos (Vladimir's code, Laurent's code) it is because Vladimir sent the code to Laurent, not the other way around, therefore Vladimir's code does not derive from Laurent's, and it does not derive from mine. I can only trust that this is the case, so please disregard my claim that this is derivative work when reviewing this PR." @AlanBateman Vertical pipeline of PR hides comments in the middle and you have to click on "Show more..." to see all comments. There are no claims related to the origin of my patch, it doesn't violate any rights. - PR: https://git.openjdk.java.net/jdk/pull/3938
RFR: 8245095: Implementation of JEP 408: Simple Web Server
This change implements a simple web server that can be run on the command-line with `java -m jdk.httpserver`. This is facilitated by adding an entry point for the `jdk.httpserver` module, an implementation class whose main method is run when the above command is executed. This is the first such module entry point in the JDK. The server is a minimal HTTP server that serves the static files of a given directory, similar to existing alternatives on other platforms and convenient for testing, development, and debugging. Additionally, a small API is introduced for programmatic creation and customization. Testing: tier1-3. - Commit messages: - fix whitespace - Move changes from sandbox to mainline Changes: https://git.openjdk.java.net/jdk/pull/5505/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5505=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8245095 Stats: 6891 lines in 42 files changed: 6856 ins; 15 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/5505.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5505/head:pull/5505 PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8273711: Remove redundant stream() call before forEach in jdk.jlink
On Mon, 13 Sep 2021 20:06:08 GMT, Andrey Turbanov wrote: > 8273711: Remove redundant stream() call before forEach in jdk.jlink Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5500
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
On Thu, 13 May 2021 11:31:49 GMT, iaroslavski wrote: >> Perhaps we can resolve this issue in private - my email address is on my >> profile (or in the commits in `radix-sort-benchmark`)? > > @richardstartin And one more addon: my first version of Radix sort, see my > github https://github.com/iaroslavski/sorting/tree/master/radixsort uses > another name, like skipBytes, then renamed to passLevel. > So, the common part is "skip". And this method has different number of > parameters. I don't see any collision with your code. > Hi @iaroslavski I'm unconvinced that this work was from 14/06/2020 - I > believe this work derives from an unsigned radix sort I implemented on > 10/04/2021 > [richardstartin/radix-sort-benchmark@ab4da23#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226](https://github.com/richardstartin/radix-sort-benchmark/commit/ab4da230e1d0ac68e5ee2cee38d71c7e7d50f49b#diff-6c13d3fb74f38906677dbfa1a70a123c8e5baf4a39219c81ef121e078d0013bcR226) > which has numerous structural similarities to this work: > Moreover, @bourgesl forked my repository on 11/04/2021 and communicated with > me about doing so. On 25/04/2021 there was a new implementation of > `DualPivotQuicksort` with a signed radix sort but the same structural > similarities, and with the same method and variable names in places > [bourgesl/radix-sort-benchmark@90ff7e4#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609](https://github.com/bourgesl/radix-sort-benchmark/commit/90ff7e427da0fa49f374bff0241fb2487bd87bde#diff-397ce8fd791e2ce508cf9127201bc9ab46264cd2a79fd0487a63569f2e4b59b2R607-R609) @iaroslavski The attribution is not clear here. Can you provide a summary as to who is contributing to this patch? I can't tell if all involved have signed the OCA or not. I'm sure there will be questions about space/time trade-offs with radix sort but I think it's important to first establish the origins of this patch first. - PR: https://git.openjdk.java.net/jdk/pull/3938
RFR: 8273710: Remove redundant stream() call before forEach in jdk.jdeps
8273710: Remove redundant stream() call before forEach in jdk.jdeps - Commit messages: - [PATCH] Remove redundant stream() call before forEach in jdk.jdeps Changes: https://git.openjdk.java.net/jdk/pull/5498/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5498=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273710 Stats: 30 lines in 10 files changed: 0 ins; 5 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/5498.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5498/head:pull/5498 PR: https://git.openjdk.java.net/jdk/pull/5498
RFR: 8273711: Remove redundant stream() call before forEach in jdk.jlink
8273711: Remove redundant stream() call before forEach in jdk.jlink - Commit messages: - [PATCH] Remove redundant stream() call before forEach in jdk.jlink Changes: https://git.openjdk.java.net/jdk/pull/5500/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5500=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273711 Stats: 17 lines in 9 files changed: 0 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/5500.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5500/head:pull/5500 PR: https://git.openjdk.java.net/jdk/pull/5500
