Re: RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v5]
On Mon, 28 Jun 2021 17:48:37 GMT, Brian Burkhalter wrote: >> Modify `java.io.ByteArrayInputStream` methods `read(byte[])` and >> `read(byte[],int,int)` to return zero per the `InputStream` specification >> when the byte array actual or specified length is zero. > > Brian Burkhalter has updated the pull request incrementally with two > additional commits since the last revision: > > - 6766844: Move API note of read(byte[],int,int) to normative text > - 6766844: Change "equivalent" to "overridden" [CSR](https://bugs.openjdk.java.net/browse/JDK-8269399) revised to match. - PR: https://git.openjdk.java.net/jdk/pull/4591
Re: RFR: Merge jdk17 [v2]
> Forwardport JDK 17 -> JDK 18 Jesper Wilhelmsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 113 commits: - Merge - 8269615: Fix for 8263640 broke Windows build Reviewed-by: iklam, dcubed - 8269268: JDWP: Properly fix thread lookup assert in findThread() Reviewed-by: kevinw, amenkov, sspitsyn - 8260540: serviceability/jdwp/AllModulesCommandTest.java failed with "Debuggee error: 'ERROR: transport error 202: bind failed: Address already in use'" Reviewed-by: sspitsyn, kevinw - 8263640: hs_err improvement: handle class path longer than O_BUFLEN Reviewed-by: iklam, minqi, dholmes - 8269417: Minor clarification on NonblockingQueue utility Reviewed-by: kbarrett, iwalulya - 8269530: runtime/ParallelLoad/ParallelSuperTest.java timeout Reviewed-by: dholmes, coleenp - 8269126: Rename G1AllowPreventiveGC option to G1UsePreventiveGC Reviewed-by: iwalulya, kbarrett - 8261579: AArch64: Support for weaker memory ordering in Atomic Reviewed-by: adinn, shade - 8268821: Split systemDictionaryShared.cpp Reviewed-by: erikj, ccheung, iklam - ... and 103 more: https://git.openjdk.java.net/jdk/compare/0d745ae8...be964f2e - Changes: https://git.openjdk.java.net/jdk/pull/4630/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4630=01 Stats: 30219 lines in 619 files changed: 17676 ins; 10608 del; 1935 mod Patch: https://git.openjdk.java.net/jdk/pull/4630.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4630/head:pull/4630 PR: https://git.openjdk.java.net/jdk/pull/4630
Integrated: Merge jdk17
On Wed, 30 Jun 2021 00:31:34 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 17 -> JDK 18 This pull request has now been integrated. Changeset: ee526a2e Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/ee526a2ea840aedb97b23538f9d624acbccebc97 Stats: 127 lines in 11 files changed: 70 ins; 17 del; 40 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/4630
RFR: Merge jdk17
Forwardport JDK 17 -> JDK 18
-
Commit messages:
- Merge
- 8269034: AccessControlException for SunPKCS11 daemon threads
- 8269529: javax/swing/reliability/HangDuringStaticInitialization.java fails
in Windows debug build
- 8269232: assert(!is_jweak(handle)) failed: wrong method for detroying jweak
- 8268884: C2: Compile::remove_speculative_types must iterate top-down
- 8249646: Runtime.exec(String, String[], File) documentation contains literal
{@link ...}
- 8268699: Shenandoah: Add test for JDK-8268127
- 8269517: compiler/loopopts/TestPartialPeelingSinkNodes.java crashes with
-XX:+VerifyGraphEdges
- 8269126: Rename G1AllowPreventiveGC option to G1UsePreventiveGC
The merge commit only contains trivial merges, so no merge-specific webrevs
have been generated.
Changes: https://git.openjdk.java.net/jdk/pull/4630/files
Stats: 127 lines in 11 files changed: 70 ins; 17 del; 40 mod
Patch: https://git.openjdk.java.net/jdk/pull/4630.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4630/head:pull/4630
PR: https://git.openjdk.java.net/jdk/pull/4630
[jdk17] Integrated: 8269034: AccessControlException for SunPKCS11 daemon threads
On Tue, 22 Jun 2021 13:26:41 GMT, Sean Coffey wrote: > Sufficient permissions missing if this code was ever to run with > SecurityManager. > > Cleanest approach appears to be use of InnocuousThread to create the > cleaner/poller threads. > Test case coverage extended to cover the SecurityManager scenario. > > Reviewer request: @valeriepeng This pull request has now been integrated. Changeset: 0d745ae8 Author:Sean Coffey URL: https://git.openjdk.java.net/jdk17/commit/0d745ae8fde5cab290dc8c695d2906f9a98c491c Stats: 95 lines in 5 files changed: 53 ins; 17 del; 25 mod 8269034: AccessControlException for SunPKCS11 daemon threads Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk17/pull/117
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 19:57:24 GMT, Roger Riggs wrote: >> If I switch to a "non-weak" set or map, then it seems I can safely use the >> source string as the key. Will using the Class object as a key prevent them >> from unloading? > > Using a synchronized WeakHashMap with the class as the key would not prevent > class unloading. > Using a non-weak set or map to strings would keep the strings around for the > life of the runtime. I hope this is uncommon but if that class is created by a `ClassLoader` again and again then it will be different each time. I'll investigate more. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 19:35:40 GMT, Weijun Wang wrote: >> Using a HashSet could use the callerClass as the key and be a stable >> reference for having given the message. >> or use a ConcurrentHashMap>, boolean> and avoid any separate >> synchronization that would be needed with a HashSet or HashMap. > > If I switch to a "non-weak" set or map, then it seems I can safely use the > source string as the key. Will using the Class object as a key prevent them > from unloading? Using a synchronized WeakHashMap with the class as the key would not prevent class unloading. Using a non-weak set or map to strings would keep the strings around for the life of the runtime. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 19:23:26 GMT, Roger Riggs wrote: >> src/java.base/share/classes/java/lang/System.java line 337: >> >>> 335: = Collections.synchronizedMap(new WeakHashMap<>()); >>> 336: } >>> 337: >> >> I wonder about the use of a WeakHashMap here. That may work well when the >> source is an interned string (a class name) which will be strongly >> referenced elsewhere and may be garbage collected if the class is unloaded, >> but in the case where it contains the name of the source jar then that >> string will only be referenced by the weak hashmap, and therefore it could >> be garbage collected any time - which would cause the mapping to be removed. >> In that case you cannot guarantee that the warning will be emitted only once. > > Using a HashSet could use the callerClass as the key and be a stable > reference for having given the message. > or use a ConcurrentHashMap>, boolean> and avoid any separate > synchronization that would be needed with a HashSet or HashMap. If I switch to a "non-weak" set or map, then it seems I can safely use the source string as the key. Will using the Class object as a key prevent them from unloading? - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform
On Fri, 25 Jun 2021 12:10:18 GMT, Masanori Yano wrote: > Hi all, > > Could you please review the 8269373 bug fixes? > > These tests call java.lang.ProcessBuilder in direct, so not used jtreg > command option. To run non-localized tests, -Duser.language=en and > -Duser.country=US options should be added in ProcessBuilder. >From a peek at the bug report, I could not find out the exact cause why they >are failing. Have you figured it out? As to the fix itself, I would rather avoid hardcoding `en-US` in the argument. Would that be replaced with the launching process' locale? - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 18:47:23 GMT, Daniel Fuchs wrote: >> Add a cache to record which sources have called `System::setSecurityManager` >> and only print out warning lines once for each. > > src/java.base/share/classes/java/lang/System.java line 337: > >> 335: = Collections.synchronizedMap(new WeakHashMap<>()); >> 336: } >> 337: > > I wonder about the use of a WeakHashMap here. That may work well when the > source is an interned string (a class name) which will be strongly referenced > elsewhere and may be garbage collected if the class is unloaded, but in the > case where it contains the name of the source jar then that string will only > be referenced by the weak hashmap, and therefore it could be garbage > collected any time - which would cause the mapping to be removed. In that > case you cannot guarantee that the warning will be emitted only once. Using a HashSet could use the callerClass as the key and be a stable reference for having given the message. or use a ConcurrentHashMap>, boolean> and avoid any separate synchronization that would be needed with a HashSet or HashMap. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Mon, 28 Jun 2021 21:09:43 GMT, Weijun Wang wrote: > Add a cache to record which sources have called `System::setSecurityManager` > and only print out warning lines once for each. src/java.base/share/classes/java/lang/System.java line 337: > 335: = Collections.synchronizedMap(new WeakHashMap<>()); > 336: } > 337: I wonder about the use of a WeakHashMap here. That may work well when the source is an interned string (a class name) which will be strongly referenced elsewhere and may be garbage collected if the class is unloaded, but in the case where it contains the name of the source jar then that string will only be referenced by the weak hashmap, and therefore it could be garbage collected any time - which would cause the mapping to be removed. In that case you cannot guarantee that the warning will be emitted only once. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v2]
On Mon, 28 Jun 2021 09:13:31 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this proposed fix for the issue reported in
>> https://bugs.openjdk.java.net/browse/JDK-8190753?
>>
>> The commit here checks for the size of the zip entry before trying to create
>> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has
>> been included in this commit to reproduce the issue and verify the fix.
>>
>> P.S: It's still a bit arguable whether it's a good idea to create the
>> `ByteArrayOutputStream` with an initial size potentially as large as the
>> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default
>> value. However, I think that can be addressed separately while dealing with
>> https://bugs.openjdk.java.net/browse/JDK-8011146
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add @requires to the new test to selectively decide where the test runs
Thank you for looking into this issue.
I ran your current test 150 times and the max runtime was 25 seconds, most
cases were in the 18-20 second range on our slower test boxes.
As part of looking at what happens with a file whose deflated size is > 2gb, I
would add a specific test which is a manual test to validate that there is no
issue when we cross the 2gb threshold.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1954:
> 1952: } else {
> 1953: os = new ByteArrayOutputStream((e.size > 0 && e.size <=
> MAX_ARRAY_SIZE)? (int)e.size : 8192);
> 1954: }
The proposed change will address the specific issue shown in the bug. As Alan
points out, there could be an issue if the deflated size is > 2gb. It would be
good to look into that as part of your proposed fix.
-
PR: https://git.openjdk.java.net/jdk/pull/4607
Re: [jdk17] RFR: 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes [v3]
On Tue, 29 Jun 2021 17:36:30 GMT, Jim Laskey wrote: >> The wording of the @implSpec referred to internal methods in the >> description. The patch rewords the @implSpec to be more descriptive of the >> algorithm than the methods used. > > Jim Laskey has updated the pull request incrementally with two additional > commits since the last revision: > > - Clean up follow up > - Clean up wording Thanks for the updates. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/151
Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v3]
On Tue, 29 Jun 2021 00:07:41 GMT, Sean Coffey wrote: >> Sufficient permissions missing if this code was ever to run with >> SecurityManager. >> >> Cleanest approach appears to be use of InnocuousThread to create the >> cleaner/poller threads. >> Test case coverage extended to cover the SecurityManager scenario. >> >> Reviewer request: @valeriepeng > > Sean Coffey has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Edits from review > - Merge remote-tracking branch 'origin/master' into pkcs11-perms > - Move TokenPoller to Runnable > - 8269034: AccessControlException for SunPKCS11 daemon threads Update looks good. Thanks, Valerie - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/117
Re: [jdk17] RFR: 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes [v2]
On Mon, 28 Jun 2021 19:07:09 GMT, Roger Riggs wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Not averaging
>
> src/java.base/share/classes/java/util/random/RandomGenerator.java line 644:
>
>> 642: *
>> 643: * @implSpec The default implementation checks that {@code bound}
>> is a
>> 644: * positive int. Then invokes {@code nextInt()}, limiting the
>> result to
>
> I think if you are refering to the Java type it should be in {@ code xxx }.
> "int" - > {@ code int}. Or use "integer".
> Ditto "ints", "long", and "longs" in the overridden methods below.
Updated
> src/java.base/share/classes/java/util/random/RandomGenerator.java line 674:
>
>> 672: * If {@code bound} is a power of two then limiting is a simple
>> masking
>> 673: * operation. Otherwise, the result is re-calculated by invoking
>> 674: * {@code nextInt()} until the result is greater equal {@code
>> origin}
>
> greater equal -> greater than or equal
> and in the overloads below (and above)
Updated
-
PR: https://git.openjdk.java.net/jdk17/pull/151
Re: [jdk17] RFR: 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes [v3]
> The wording of the @implSpec referred to internal methods in the description. > The patch rewords the @implSpec to be more descriptive of the algorithm than > the methods used. Jim Laskey has updated the pull request incrementally with two additional commits since the last revision: - Clean up follow up - Clean up wording - Changes: - all: https://git.openjdk.java.net/jdk17/pull/151/files - new: https://git.openjdk.java.net/jdk17/pull/151/files/bfb61c8d..b87d22fb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=151=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=151=01-02 Stats: 24 lines in 1 file changed: 3 ins; 0 del; 21 mod Patch: https://git.openjdk.java.net/jdk17/pull/151.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/151/head:pull/151 PR: https://git.openjdk.java.net/jdk17/pull/151
Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v5]
- Original Message -
> From: "Roger Riggs"
> To: "core-libs-dev"
> Sent: Mardi 29 Juin 2021 18:44:04
> Subject: Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method
> [v5]
> On Tue, 29 Jun 2021 05:47:03 GMT, Tagir F. Valeev wrote:
>
>>> Roger Riggs has updated the pull request incrementally with one additional
>>> commit since the last revision:
>>>
>>> Add test synchronizing on return value of Objecst.newIdentity()
>>
>> Probably it would be better to have an inner class named like `Identity`
>> instead
>> of anonymous class? When debugging or analyzing memory dumps, it would be
>> more
>> user-friendly to see `Objects$Identity` than `Objects$1`.
>>
>> Probably, not the part of this feature request, but it would be nice to add
>> another method with string parameter, like `Objects.newIdentity("MY
>> SENTINEL")`. The string should be stored in the field and returned from
>> toString(). Again, this would make it easier to find where the object comes
>> from during debugging or memory dump analysis. For the record, here's what we
>> have in IntelliJ IDEA sources (Apache 2.0 licensed):
>>
>>
>> public final class ObjectUtils {
>> private ObjectUtils() { }
>>
>> ...
>>
>> /**
>>* Creates a new object which could be used as sentinel value (special
>> value to
>>distinguish from any other object). It does not equal
>>* to any other object. Usually should be assigned to the static final
>> field.
>>*
>>* @param name an object name, returned from {@link #toString()} to
>> simplify the
>>debugging or heap dump analysis
>>* (guaranteed to be stored as sentinel object field). If
>> sentinel is
>>assigned to the static final field,
>>* it's recommended to supply that field name (possibly
>> qualified
>>with the class name).
>>* @return a new sentinel object
>>*/
>> public static @NotNull Object sentinel(@NotNull @NonNls String name) {
>> return new Sentinel(name);
>> }
>>
>> private static final class Sentinel {
>> private final String myName;
>>
>> Sentinel(@NotNull String name) {
>> myName = name;
>> }
>>
>> @Override
>> public String toString() {
>> return myName;
>> }
>> }
>
> @amaembo Good suggestion for the Valhalla API and implementation. Today, only
> the hashCode() identifies an Object.
> For JDK17, it will continue to be a raw Object() instance.
We should try to have a very limited API surface for now.
We want to let the door open to having Object being both an identity class
(when used with "new") and an abstract class (when inherited) by using two
different generic specialization of the same class or whatever other scheme we
come before Valhalla is complete.
>
> -
>
> PR: https://git.openjdk.java.net/jdk17/pull/112
Rémi
Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v5]
On Tue, 29 Jun 2021 05:47:03 GMT, Tagir F. Valeev wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add test synchronizing on return value of Objecst.newIdentity()
>
> Probably it would be better to have an inner class named like `Identity`
> instead of anonymous class? When debugging or analyzing memory dumps, it
> would be more user-friendly to see `Objects$Identity` than `Objects$1`.
>
> Probably, not the part of this feature request, but it would be nice to add
> another method with string parameter, like `Objects.newIdentity("MY
> SENTINEL")`. The string should be stored in the field and returned from
> toString(). Again, this would make it easier to find where the object comes
> from during debugging or memory dump analysis. For the record, here's what we
> have in IntelliJ IDEA sources (Apache 2.0 licensed):
>
>
> public final class ObjectUtils {
> private ObjectUtils() { }
>
> ...
>
> /**
>* Creates a new object which could be used as sentinel value (special
> value to distinguish from any other object). It does not equal
>* to any other object. Usually should be assigned to the static final
> field.
>*
>* @param name an object name, returned from {@link #toString()} to
> simplify the debugging or heap dump analysis
>* (guaranteed to be stored as sentinel object field). If
> sentinel is assigned to the static final field,
>* it's recommended to supply that field name (possibly
> qualified with the class name).
>* @return a new sentinel object
>*/
> public static @NotNull Object sentinel(@NotNull @NonNls String name) {
> return new Sentinel(name);
> }
>
> private static final class Sentinel {
> private final String myName;
>
> Sentinel(@NotNull String name) {
> myName = name;
> }
>
> @Override
> public String toString() {
> return myName;
> }
> }
@amaembo Good suggestion for the Valhalla API and implementation. Today, only
the hashCode() identifies an Object.
For JDK17, it will continue to be a raw Object() instance.
-
PR: https://git.openjdk.java.net/jdk17/pull/112
[jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
Add a cache to record which source has called `System::setSecurityManager` and only print out warning lines once for each. - Commit messages: - renaming variables, two callers in test - one warning for each caller Changes: https://git.openjdk.java.net/jdk17/pull/166/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=166=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269543 Stats: 47 lines in 2 files changed: 34 ins; 0 del; 13 mod Patch: https://git.openjdk.java.net/jdk17/pull/166.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/166/head:pull/166 PR: https://git.openjdk.java.net/jdk17/pull/166
[jdk17] Integrated: JDK-8249646: Runtime.exec(String, String[], File) documentation contains literal {@link ...}
On Tue, 29 Jun 2021 04:39:28 GMT, Jonathan Gibbons wrote:
> Please review a trivial `noreg-doc` fix for some javadoc tags for
> `java.lang.Runtime` for JDK17
This pull request has now been integrated.
Changeset: 25f9f19a
Author:Jonathan Gibbons
URL:
https://git.openjdk.java.net/jdk17/commit/25f9f19af9831e151a39518020aefa2c18fd7217
Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
8249646: Runtime.exec(String, String[], File) documentation contains literal
{@link ...}
Reviewed-by: sundar, iris
-
PR: https://git.openjdk.java.net/jdk17/pull/167
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v2]
On Wed, 23 Jun 2021 13:37:59 GMT, Matthias Baesken wrote:
>> Hello, please review this PR; it extend the OSContainer API in order to also
>> support the pids controller of cgroups.
>>
>> I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct",
>> "memory" on some older Linux distros (SLES 12.1, RHEL 7.1) the pids
>> controller might not be there (or not fully supported) so it was added as
>> optional , see the coding
>>
>>
>> if (!cg_infos[PIDS_IDX]._data_complete) {
>> log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
>> // keep the other controller info, pids is optional
>> }
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Adjustments following Severins comments
This looks pretty good now. Looking forward to seeing container tests for this
new code.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 559:
> 557: return OSCONTAINER_ERROR;
> 558: }
> 559: // Unlimited memory in Cgroups V2 is the literal string 'max'
Please don't add version specific comments to version agnostic code.
Suggestion: "Unlimited memory in cgroups is the literal string 'max' for some
controllers, for example the pids controller."
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 268:
> 266: char * pidsmax_str = pids_max_val();
> 267: jlong pidsmax = limit_from_str(pidsmax_str);
> 268: return pidsmax;
Do we need this local variable? Consider using `return
limit_from_str(pidsmax_str);` instead.
src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 250:
> 248: char * pidsmax_str = pids_max_val();
> 249: jlong pidsmax = limit_from_str(pidsmax_str);
> 250: return pidsmax;
Same here. Use `return limit_from_str(pidsmax_str);`
-
PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. To share the information... I contacted one of the original authors of Java annotations, but he doesn't remember anything about the option. Here's the conversation... On Thu, Jun 3, 2021 at 11:53 PM Peter Levart wrote: > > There is an undocumented JVM flag called > "-XX+PreserveAllAnnotations" > which according to JVM code affects the way > "RuntimeInvisibleAnnotations" kind of class attributes are > treated at > runtime. Normally they are ignored (since they normally contain > only > metadata for annotation types with CLASS retention), but when > this flag > is passed to JVM, such attributes are also passed to > AnnotationParser > together with "RuntimeVisibleAnnotations" kind of class > attributes. > > So the question is: "What was the original intent of this JVM > flag". A > debate sprung in a review thread on JDK GitHub and some say that > this is > probably just a "leftover from an early implementation" and are > proposing to deprecate and remove it. Others (me mostly) see some > sense > in the feature as it allows migrating an annotation type from > CLASS > retention to RUNTIME without recompiling all the classes that use > the > annotation. Do you happen to know the original intent of this JVM > flag? On Fri, Jun 4, 2021 at 6:54 AM Neal Gafter wrote: > > I don’t recall that flag, but my suspicion is that it was used to > assist testing. > If you ask me, I think the ecosystem would be better without it. On 04/06/2021 15:56, Neal Gafter wrote: > > One more thing. You don’t have to recompile uses of an annotation to > migrate > from class to runtime retention. You only have to recompile the > attribute declaration. On Fri, Jun 4, 2021 at 7:15 AM Peter Levart wrote: > > Not really. The AnnotationParser does filter out annotations based on > current "retention" > of annotation type which is just a meta-annotation on the annotation > type, so it would appear > that only the annotation declaration has to be recompiled, but as I > described, the annotation > uses in classes that use the annotation get compiled into two kinds of > class attributes: > RuntimeVisibleAnnotations (those with RUNTIME retention) and > RuntimeInvisibleAnnotations > (those with CLASS retention). Only those from RuntimeVisibleAnnotations > are passed to > AnnotationParser at runtime unless this VM flag is given which makes the > VM pass a concatenation > of RuntimeVisibleAnnotations and RuntimeInvisibleAnnotations to the > AnnotationParser. > So merely changing the annotation retention and recompiling the > annotation declaration is not enough. > You have to also either recompile annotation uses or pass this VM flag. > So I was wondering if the flag was actually meant for this purpose. On 04/06/2021 17:30, Neal Gafter wrote: > > Got it. I don't remember anything about the VM flag, but I doubt it was > designed to be used for this purpose. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. Well, if you are willing to take this further, I would suggest you modify the test so that it would only test the "correct" behavior and not also the "incorrect" one. Meaning, you should test annotated elements (class in you case) that are annotated with CLASS retention annotations only (at the time the annotation uses are compiled) and not with mixed set (CLASS and RUNTIME retention) annotations. In that case the outcome is sensible (and useful). If others agree that adding such test is OK, I can sponsor it. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. Is there anyone willing to sponsor this PR? What changes shall I make to get your sponsorship? - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: 8173970: jar tool should have a way to extract to a directory
A word about "compatibility" with other jdk tools... jar is a specific tool. It was initially conceived to be user-friendly with users that are already familiar with tar tool: "The syntax for the jar command resembles the syntax for the tar command." Mixing-in behaviors not consistent with tar will make it a strange hybrid. Trying to find common ground with other JDK tools will make it less flexible. Regards, Peter On 29/06/2021 09:05, Peter Levart wrote: Hi, On 29/03/2021 12:46, Jaikiran Pai wrote: Given the support of -C/-dir is new and -P is there for backwards compatibility then I see the choices are: * Issue an error when -P and -C/-dir are specified together with -x * Silently ignore the -P option * Ignore -C/-dir and the behavior is if -xfP was specified. I am leaning towards an error given this is a new feature when -P and -C/-dir are specified with -x That sounds reasonable to me. I'll update the PR accordingly to take this into account. -Jaikiran If we are trying to follow the semantics of tar -C option (gnutar and other Unix tar(s)) then I think -P may be combined with -C when extracting. The manpage for gnutar says: -C, --directory=DIR Change to DIR before performing any operations. This option is order-sensitive, i.e. it affects all options that follow. ...the meaning is that relative paths from archive get extracted relative to current dir (possibly modified by -C) and that absolute paths get extracted to their absolute positions (i.e. not affected by current dir). The -C option (as implemented by tar) is also useful when gathering files/dirs from different base paths into common archive structure. Say you have the following (relative) directory/file structure: dir1/subdir1/a.txt dir1/subdir1/b.txt dir1/file1.txt dir2/subdir2/a.txt dir2/subdir2/b.txt dir2/file2.txt The command: tar -cf archive.tar -C dir1 . -C ../dir2 . ...will create archive.tar with the following content (notice the 2nd -C option is dependent on the 1st: -C literally changes the current directory before executing further options): ./ ./subdir1/ ./subdir1/a.txt ./subdir1/b.txt ./file1.txt ./ ./subdir2/ ./subdir2/a.txt ./subdir2/b.txt ./file2.txt When extracting, -C may be used to scatter different relative archive files/dirs into different base paths. For example, the archive.tar from above can be extracted into original directory structure with the following command (empty dir1 and dir2 have to be already present): tar -xf archive.tar -C dir1 ./file1.txt ./subdir1 -C ../dir2 ./file2.txt ./subdir2 So here's a hint about the behavior of multiple -C options: if the option is -C, it should follow tar -C behavior. Regards, Peter
Re: RFR: 8173970: jar tool should have a way to extract to a directory
Hi, On 29/03/2021 12:46, Jaikiran Pai wrote: Given the support of -C/-dir is new and -P is there for backwards compatibility then I see the choices are: * Issue an error when -P and -C/-dir are specified together with -x * Silently ignore the -P option * Ignore -C/-dir and the behavior is if -xfP was specified. I am leaning towards an error given this is a new feature when -P and -C/-dir are specified with -x That sounds reasonable to me. I'll update the PR accordingly to take this into account. -Jaikiran If we are trying to follow the semantics of tar -C option (gnutar and other Unix tar(s)) then I think -P may be combined with -C when extracting. The manpage for gnutar says: -C, --directory=DIR Change to DIR before performing any operations. This option is order-sensitive, i.e. it affects all options that follow. ...the meaning is that relative paths from archive get extracted relative to current dir (possibly modified by -C) and that absolute paths get extracted to their absolute positions (i.e. not affected by current dir). The -C option (as implemented by tar) is also useful when gathering files/dirs from different base paths into common archive structure. Say you have the following (relative) directory/file structure: dir1/subdir1/a.txt dir1/subdir1/b.txt dir1/file1.txt dir2/subdir2/a.txt dir2/subdir2/b.txt dir2/file2.txt The command: tar -cf archive.tar -C dir1 . -C ../dir2 . ...will create archive.tar with the following content (notice the 2nd -C option is dependent on the 1st: -C literally changes the current directory before executing further options): ./ ./subdir1/ ./subdir1/a.txt ./subdir1/b.txt ./file1.txt ./ ./subdir2/ ./subdir2/a.txt ./subdir2/b.txt ./file2.txt When extracting, -C may be used to scatter different relative archive files/dirs into different base paths. For example, the archive.tar from above can be extracted into original directory structure with the following command (empty dir1 and dir2 have to be already present): tar -xf archive.tar -C dir1 ./file1.txt ./subdir1 -C ../dir2 ./file2.txt ./subdir2 So here's a hint about the behavior of multiple -C options: if the option is -C, it should follow tar -C behavior. Regards, Peter
