Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes [v2]
On Mon, 19 Apr 2021 22:12:30 GMT, Ian Graves wrote: >> Clarifying note on comments mode to explicitly note that whitespace within >> character classes is ignored. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Adding differences to Perl 5 note A few minor wording adjustments. Please update the CSR accordingly and I'll review it too. src/java.base/share/classes/java/util/regex/Pattern.java line 762: > 760: *character classes. In this class, whitespace inside of character > classes > 761: *must be escaped to be considered as part of the regular expression > when in > 762: *comments mode. Editorial: the run of italicized words makes this a bit hard to follow. Suggest: In Perl, free-spacing mode (which is called comments mode in this class) src/java.base/share/classes/java/util/regex/Pattern.java line 832: > 830: * Note that comments mode ignores whitespace within a character > class > 831: * contained in a pattern string. Such whitespace needs to be escaped > 832: * in order to be treated as if comments mode were not enabled. I think this is good, but 1) it would probably be better placed in the "In this mode" paragraph above, around line 825; and 2) it's normative so it shouldn't say "Note that" (which makes it sound informative). I'd also reword the second sentence a bit, something like Such whitespace needs to be escaped in order to be considered significant. - Changes requested by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3577
Re: RFR: 8262744: Formatter '%g' conversion uses wrong format for BigDecimal rounding up to limits [v3]
On Fri, 16 Apr 2021 16:08:53 GMT, Ian Graves wrote:
>> This fixes a bug where the formatting code for `%g` flags incorrectly tries
>> to round `BigDecimal` after determining whether it should be a decimal
>> numeric format or a scientific numeric format. The solution rounds before
>> determining the correct format.
>
> Ian Graves has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Inlining some single use variables
src/java.base/share/classes/java/util/Formatter.java line 3827:
> 3825: if ((value.equals(BigDecimal.ZERO))
> 3826: || ((value.compareTo(BigDecimal.valueOf(1, 4)) !=
> -1)
> 3827: && (value.compareTo(BigDecimal.valueOf(1,
> -prec)) == -1))) {
Note that `compareTo` in general specifies a negative, zero, or positive return
value, but BigDecimal and BigInteger specify a return value of -1, 0, and 1. So
the code here that compares against -1 is strictly correct. However, the
BigDecimal/BigInteger.compareTo docs say "The suggested idiom..." is a relative
comparison against zero.
Indeed, the BigDecimal::compareTo method does always seem to return -1, 0, or 1
so this code is not incorrect. Well, maybe. I checked quickly and the
BigDecimal comparison logic is fairly intricate (and also runs through
BigInteger) so I might have missed something. Also, BigDecimal is subclassable,
so an override of `compareTo` might return something other than -1, 0, or 1,
even though strictly speaking this would violate the BigDecimal spec.
I'm wondering if there should be a followup bug that changes these tests to `>=
0` and `< 0`.
-
PR: https://git.openjdk.java.net/jdk/pull/3363
Re: RFR: 8265591: Remove vestiages of intermediate JSR 175 annotation format
On Tue, 20 Apr 2021 23:37:15 GMT, Joe Darcy wrote: > During the recent review of JDK-8228988, I noticed again the comments in the > annotation parser about support for the pre-GA annotation format used before > JDK 5.0 shipped. During the development of annotations, there was a late > change to correct a flaw in the annotation encoding, JDK-5020908. > > I don't think it is necessary to carry forward support for this transient > format any longer and this changeset removes support from both core > reflection and javac. > > Clean runs of relevant test; I gauge this fix as no-reg hard. PS CSR for the behavioral change: https://bugs.openjdk.java.net/browse/JDK-8265608 - PR: https://git.openjdk.java.net/jdk/pull/3597
RFR: 8265591: Remove vestiages of intermediate JSR 175 annotation format
During the recent review of JDK-8228988, I noticed again the comments in the annotation parser about support for the pre-GA annotation format used before JDK 5.0 shipped. During the development of annotations, there was a late change to correct a flaw in the annotation encoding, JDK-5020908. I don't think it is necessary to carry forward support for this transient format any longer and this changeset removes support from both core reflection and javac. Clean runs of relevant test; I gauge this fix as no-reg hard. - Commit messages: - 8265591: Remove vestiages of intermediate JSR 175 annotation format Changes: https://git.openjdk.java.net/jdk/pull/3597/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3597&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265591 Stats: 24 lines in 2 files changed: 0 ins; 19 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/3597.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3597/head:pull/3597 PR: https://git.openjdk.java.net/jdk/pull/3597
Re: RFR: 8264208: Console charset API [v11]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato 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 13 additional commits since the last revision: - Refined the test case. - Merge branch 'master' into JDK-8264208 - Changed shell based test into java based - Added link to Charset#defaultChaset() in InputStreamReader. - Modified javadocs per suggestions. - Added @see links. - Added Console::charset() relation with System.in - Added comment to System.out/err init. - Reflected further review comments. - Reverted PrintStream changes - ... and 3 more: https://git.openjdk.java.net/jdk/compare/8cc07521...e585d16f - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/238dbb42..e585d16f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3419&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3419&range=09-10 Stats: 72701 lines in 1955 files changed: 33448 ins; 34099 del; 5154 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Integrated: 8228988: AnnotationParser throws NullPointerException on incompatible member type
On Fri, 5 Feb 2021 21:24:34 GMT, Rafael Winterhalter wrote: > When a class is compiled against a version of an annotation that is later > loaded in an incompatible manner where an enum-typed member is changed into > an annotation or vice versa, the reflection API currently throws a > `NullPointerException` upon accessing the member. Instead an > `AnnotationTypeMismatchException` should be thrown. > > This change adjusts the parsing to trigger the correct exception. This pull request has now been integrated. Changeset: f47faf28 Author:Rafael Winterhalter Committer: Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/f47faf28 Stats: 182 lines in 3 files changed: 178 ins; 0 del; 4 mod 8228988: AnnotationParser throws NullPointerException on incompatible member type Reviewed-by: darcy - PR: https://git.openjdk.java.net/jdk/pull/2439
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v6]
On Tue, 20 Apr 2021 17:46:38 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > Updated single letter pattern variable names Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3170
Re: Image parsing spams stack traces to System.err
I think this post needs to go to 2d-dev (copied). > On Apr 20, 2021, at 9:58 AM, Lapo Luchini wrote: > > In both OpenJDK 8, 11 and 15 I verified that: > > sun/awt/image/InputStreamImageSource.java > > has "e.printStackTrace()" commands that might better be converted to > java.util.logging in order to be able to configure/redirect them to the > proper log file each application might decide to use. > > A little bit of more details as reported here (where they suggested this was > the proper place): > https://github.com/AdoptOpenJDK/openjdk-jdk11u/issues/21 > > cheers, > > -- > Lapo Luchini > l...@lapo.it >
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter wrote: >> To allow agents the definition of auxiliary classes, an API is needed to >> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or >> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed >> from `sun.misc.Unsafe`. > > Rafael Winterhalter has refreshed the contents of this pull request, and > previous commits have been removed. The incremental views will show > differences compared to the previous content of the PR. The pull request > contains one new commit since the last revision: > > 8200559: Java agents doing instrumentation need a means to define auxiliary > classes I fully understand your concerns about ByteBuddyAgent.install(). It is simply a convenience for something that can be meaningful in some contexts where I prefer offering a simple API. I use it mainly for two purposes: a) For testing Java agents and integrations against Instrumentation within the current VM when tests are triggered by tools that do not support javaagents, also because builds do not bundle jars until after tests are executed. b) For purposefully "hacky" test libraries like Mockito that need agent capabilities without this being meant to be used in production environments. I have earlier proposed to offer a "jdk.test" module that offers the Instrumentation instance via a simple API similar to Byte Buddy's. The JVM would not load this module unless requested on the command line. Build tools like Maven's surefire or Gradle's testrunner could then standardize on loading this module as a convention to give access to this test module by default such that libraries like Mockito could continue to function out of the box without the libraries functioning on a standard VM without extra configuration. As far as I know, mainly test libraries need this API. This would also emphasise that Mockito and others are meant for testing and fewer people would abuse it for production applications. People would also have an explicit means of running a JVM for a production application or for executing a test. As for adding the API, my thought is that if the Instrumentation API were to throw exceptions on some methods/arguments for dynamic agents in the future, for example for retransformClasses(Object.class), this breaking change would then simply extend to the proposed "defineClass" method. In this sense, the Instrumentation API already assumes full power, I find it not problematic to add the missing bit to this API even if it was restricted in the future in the same spirit as other methods of the API would be. I mentioned JNI as it is a well-known approach to defining a class today, using a minimal native binding to an interface that directly calls down to JNI's: jclass DefineClass(JNIEnv *env, const char *name, jobject loader, const jbyte *buf, jsize bufLen); This interface can then simply be used to define any class just as I propse, even when not writing an agent or attaching. This method makes class definitions also already trivial for JVMTI agents compared to Java agents. Unless restricting JNI, the defineClass method is already a low hanging fruit, but at the cost of having to maintain a tiny bit of native code. I'd rather see this avoided and a standard API being offered to agents up to the time that Panama is in place and a JNI restriction is possibly also included. As a bonus: Once JNI is restricted, Byte Buddy's "install" would no longer work unless self-attachment (or JNI) is explicitly allowed. The emulation already requires to run native code while the Virtual Machine API explicitly checks for the process id of the current VM against the one that is targeted. With both disabled, self-attachment would no longer be practically be possible without needing to prune the capabilities of dynamic agents which is what I understand would be the desired effect. >From this viewpoint, I think that adding Instrumentation::defineClass method does no harm compared to the status quo. And on the upside, it gives agents an API to migrate to, avoiding the last need of using unsafe. To make the JVM a safe platform, binding native code would anyways need restriction and this would then also solve the problem of dynamic agents attaching from the same VM being used in libraries. This would in my eyes be the cleanest solution to the self-attachment problem without disturbing the existing landscape of dynamic agents. To run Mockito, one would then instead configure Maven surefire or Gradle to run the JVM with -Djdk.attach.allowAttachSelf=true. Ideally, some "jdk.test" module would be added at some point, to avoid the overhead of self-attachment, but I think this better fits into separate debate. Am Di., 20. Apr. 2021 um 15:38 Uhr schrieb mlbridge[bot] < ***@***.***>: > *Mailing list message from Alan Bateman ***@***.***> on > core-libs-dev ***@***.***>:* > > On 19/04/2021 22:20, Rafael Winterhalter wrote: > > : > At the moment, it i
Re: RFR: 8228988: AnnotationParser throws NullPointerException on incompatible member type
On Fri, 5 Feb 2021 21:24:34 GMT, Rafael Winterhalter wrote: > When a class is compiled against a version of an annotation that is later > loaded in an incompatible manner where an enum-typed member is changed into > an annotation or vice versa, the reflection API currently throws a > `NullPointerException` upon accessing the member. Instead an > `AnnotationTypeMismatchException` should be thrown. > > This change adjusts the parsing to trigger the correct exception. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2439
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v6]
On Tue, 20 Apr 2021 17:46:38 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > Updated single letter pattern variable names Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v6]
On Wed, 24 Mar 2021 10:57:11 GMT, Rémi Forax
wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Updated single letter pattern variable names
>
> src/java.base/share/classes/java/time/LocalDateTime.java line 1686:
>
>> 1684: public long until(Temporal endExclusive, TemporalUnit unit) {
>> 1685: LocalDateTime end = LocalDateTime.from(endExclusive);
>> 1686: if (unit instanceof ChronoUnit u) {
>
> `chronoUnit` is perhaps a better variable name than `u`
Thanks for your comments, @forax, and apologizes for the delay in getting back
to you. I was waiting for the boot JDK version to be updated to 16. Certain
files changed in the PR are shared between the build tool and the JDK runtime,
and were causing build issues.
I've addressed the changes you suggested, and you can find them in commit
647bd6b
-
PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v6]
> Hi, > > Could someone please review my code for updating the code in the `java.time` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: Updated single letter pattern variable names - Changes: - all: https://git.openjdk.java.net/jdk/pull/3170/files - new: https://git.openjdk.java.net/jdk/pull/3170/files/2dca4a09..647bd6b1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3170&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3170&range=04-05 Stats: 51 lines in 14 files changed: 0 ins; 0 del; 51 mod Patch: https://git.openjdk.java.net/jdk/pull/3170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3170/head:pull/3170 PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v4]
On Mon, 19 Apr 2021 15:37:24 GMT, Roger Riggs wrote: >> Patrick Concannon 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 six additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin/master' into JDK-8263668 >> - Merge remote-tracking branch 'origin/master' into JDK-8263668 >> - Merge remote-tracking branch 'origin/master' into JDK-8263668 >> - Merge remote-tracking branch 'origin/master' into JDK-8263668 >> - Merge remote-tracking branch 'origin/master' into JDK-8263668 >> - 8263668: Update java.time to use instanceof pattern variable > > src/java.base/share/classes/java/time/Duration.java line 1435: > >> 1433: && this.seconds == other.seconds >> 1434: && this.nanos == other.nanos; >> 1435: } > > Perhaps rename the argument and use `otherDuration` as the refinement. > Otherwise, an inconsistency across various classes will creep in where the > more specific variable has a more general name. In this case, the argument > type is Object, so the argument name `otherDuration` is not strictly true. Parameter and pattern variable names swapped, as suggested. See 647bd6b > src/java.base/share/classes/java/time/Instant.java line 1306: > >> 1304: && this.seconds == other.seconds >> 1305: && this.nanos == other.nanos; >> 1306: } > > Ditto, `otherInstance` is not strictly always an instant and it would be more > consistent to swap the names. > `(other instanceof Instant otherInstant)`. Parameter and pattern variable names swapped, as suggested. See 647bd6b - PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8265356: need code example for getting canonical constructor of a Record
On Sat, 17 Apr 2021 08:55:59 GMT, Tagir F. Valeev wrote:
> I decided to show a complete static method in the example, so it could be
> copied to user utility class as is. Not sure if it's reasonable to add
> `assert cls.isRecord();` there. Also I don't know whether there's a
> limitation on max characters in the sample code. Probable a line break in
> `static \nConstructor getCanonicalConstructor(Class
> cls)` is unnecessary.
>
> ---
> Aside from this PR, I've found a couple of things to clean up in
> `java.lang.Class`:
> 1. There's erroneous JavaDoc link in `getSimpleName()` JavaDoc (introduced by
> @jddarcy in #3038). It should be `#isArray()` instead of `isArray()`.
> 2. Methods Atomic::casAnnotationType and Atomic::casAnnotationData have
> unused type parameters ``.
> 3. Probably too much but AnnotationData can be nicely converted to a record!
> Not sure, probably nobody wants to have `java.lang.Record` initialized too
> early or increasing the footprint of such a basic class in the metaspace, so
> I don't insist on this.
>
>
> private record AnnotationData(
> Map, Annotation> annotations,
> Map, Annotation> declaredAnnotations,
> // Value of classRedefinedCount when we created this AnnotationData
> instance
> int redefinedCount) {
> }
>
>
> Please tell me if it's ok to fix 1 and 2 along with this PR.
Thanks for writing this example.
I think that the example lines can be longer. I'd suggest putting the main part
of the method declaration on the same line as `static `, but
leaving the `throws` clause on the next line.
I think including the small cleanups (1) and (2) in this PR is fine. Changing
`AnnotationData` to be a record seems like it might have other effects, so I'd
leave that one out.
One other thing I'd like to see is a link to this example code from places
where people are likely to look for it. The class doc for `java.lang.Record`
has a definition for "canonical constructor" so it would be nice to link to the
example here. Something like "For further information about how to find the
canonical constructor reflectively, see Class::getRecordComponents." (With
appropriate javadoc markup.) This could either be a parenthetical comment
somewhere in the "canonical constructor" discussion, or possibly a separate
paragraph in the `@apiNote` section below.
-
PR: https://git.openjdk.java.net/jdk/pull/3556
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v5]
On Wed, 24 Mar 2021 13:57:35 GMT, Roger Riggs wrote: > In addition to the other suggestions, java.time could use a round of cleanup > to use switch expressions. Hi Roger, I plan to introduce switch expressions in a follow up issue/PR. - PR: https://git.openjdk.java.net/jdk/pull/3170
Image parsing spams stack traces to System.err
In both OpenJDK 8, 11 and 15 I verified that: sun/awt/image/InputStreamImageSource.java has "e.printStackTrace()" commands that might better be converted to java.util.logging in order to be able to configure/redirect them to the proper log file each application might decide to use. A little bit of more details as reported here (where they suggested this was the proper place): https://github.com/AdoptOpenJDK/openjdk-jdk11u/issues/21 cheers, -- Lapo Luchini l...@lapo.it
Integrated: 8265036: JFR: Remove use of -XX:StartFlightRecording= and -XX:FlightRecorderOptions=
On Sun, 18 Apr 2021 15:17:35 GMT, Erik Gahlin wrote: > Hi, > > Could I have a review of fix that removes the use of "=" together with > -XX:StartFlightRecording and -XX:FlightRecorderOptions. It's been possible to > use "-XX:StartFlightRecording:" and "-XX:FlightRecorderOption:" since JFR was > introduced into OpenJDK (JDK 11), so this is not a change of the > specification, just an update to make the use consistent in tests, comments, > documentation etc. > > I also removed the use of -XX:+FlightRecorder, which is not needed, and has > been deprecated since JDK 13. > > Testing: jdk/jdk/jfr, tier 1-4. > > Thanks > Erik This pull request has now been integrated. Changeset: 4dcaac1f Author:Erik Gahlin URL: https://git.openjdk.java.net/jdk/commit/4dcaac1f Stats: 104 lines in 40 files changed: 0 ins; 0 del; 104 mod 8265036: JFR: Remove use of -XX:StartFlightRecording= and -XX:FlightRecorderOptions= Reviewed-by: cjplummer - PR: https://git.openjdk.java.net/jdk/pull/3561
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v5]
> Hi, > > Could someone please review my code for updating the code in the `java.time` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Patrick Concannon 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 seven additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8263668 - Merge remote-tracking branch 'origin/master' into JDK-8263668 - Merge remote-tracking branch 'origin/master' into JDK-8263668 - Merge remote-tracking branch 'origin/master' into JDK-8263668 - Merge remote-tracking branch 'origin/master' into JDK-8263668 - Merge remote-tracking branch 'origin/master' into JDK-8263668 - 8263668: Update java.time to use instanceof pattern variable - Changes: - all: https://git.openjdk.java.net/jdk/pull/3170/files - new: https://git.openjdk.java.net/jdk/pull/3170/files/6481346a..2dca4a09 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3170&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3170&range=03-04 Stats: 3821 lines in 79 files changed: 2021 ins; 1604 del; 196 mod Patch: https://git.openjdk.java.net/jdk/pull/3170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3170/head:pull/3170 PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName()
On Mon, 19 Apr 2021 15:03:44 GMT, Сергей Цыпанов
wrote:
>> src/java.base/share/classes/java/util/concurrent/atomic/AtomicIntegerFieldUpdater.java
>> line 458:
>>
>>> 456: private static boolean isSamePackage(Class class1, Class
>>> class2) {
>>> 457: return class1.getClassLoader() == class2.getClassLoader()
>>> 458:&&
>>> class1.getPackageName().equals(class2.getPackageName());
>>
>> If the j.u.c.atomic code is changed then it will need to be coordinated with
>> Doug Lea's repo. The rest of the changes are good and thanks for following
>> up from the comments in the other issue.
>
> Hello, how should I do it? Maybe we can just mention him here?
@DougLea could you have a look into the part of this PR related to
java.util.concurrent package?
-
PR: https://git.openjdk.java.net/jdk/pull/3571
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On 19/04/2021 22:20, Rafael Winterhalter wrote: : At the moment, it is required for root to switch to the user that owns the JVM process as the domain socket is only accessible to that user to avoid that users without access to the JVM can inject themselves into a JVM. I am not sure if operations teams would be thrilled to have a monitoring agent required to run as root, even in these times of Kubernetes. I mainly have two comments: 1. The problem is the possibility of self-attach. I think this is the problem to solve, a library getting agent privileges without being an agent. I think this should be prevented while dynamic attach should continue to be possible in today's format. It has proven to be so useful, it would be a shame if the current tooling convenience would disappear from the JVM. As it's my understanding, JNI is supposed to be restricted in the future, in line with Panama. Without this restriction, JNI already allows for random class definition, for example, which similarly to an agent offers surpassing the majority of JVM restrictions. The second restriction would be a control to restrict how a JVM process starts new processes. I think both are reasonable restrictions for a library to face which require explicit enabling. Especially with the security manager on it's way out, certain capabilities should be rethought to begin with. If both are no longer freely available, self-attachment is no longer possible anyways and dynamic agents could retain their capabilities. 2. The question of introducing an Instrumentation::defineClass method is fully independent of that first question. If a dynamic agent was to be restricted, the method could reject classloader/package combinations for dynamically loaded agents the same way that Instrumentation::retransformClasses would need to. At the same time, introducing the method would allow agents to move to an official API with a Java 17 baseline which will be the next long-standing base line. I fully understand it needs a thorough discussion but it is a less complicated problem then (1) and could therefore be decided prior to having found a satisfactory solution for it. I should have been clearer, it's the combination of the two that creates the attractive nuisance. I don't think there are any objections to a defineClass for agents specified on the command line with -javaagent. However we have to be cautious about extending that capability to agents that are loaded into a running VM with the attach mechanism. ByteBuddy looks great for code generation and transforming classes but ByteBuddyAgent makes me nervous. It looks like I can deploy byte-buddy-agent-.jar on my class path and invoke the public static ByteBuddyAgent.install() method to get the Instrumentation object for the current VM. That may be convenient for some but this is the all-powerful Instrumentation object that shouldn't be leaked to library or application code. Now combine this with the proposed defineClass and it means that any code on the class path could inject a class into java.lang or any run-time package without any agent voodoo or opt-in via the command line. That would be difficult genie to re-bottle if it were to get traction. You mentioned restricting JNI in the future. I'm not aware of a definite plan or time-frame. Project Panama is pioneering restricting access to native operations as a bug or mis-use with the linker API can easily crash the VM or breakage in other ways. Extending this to JNI would be a logical next step but I could imagine it taking a long time and many releases to get there. As regards this PR then I would be happy to work with you on a revised proposed that would limit it to agents specified with -javaagent. That would not preclude extending the capability, maybe in a more restricted form, to agents loaded into a running VM in the future. -Alan.
Re: RFR: 8265036: JFR: Remove use of -XX:StartFlightRecording= and -XX:FlightRecorderOptions= [v2]
On Mon, 19 Apr 2021 20:16:59 GMT, Chris Plummer wrote: > The changes look good. Have you considered doing a test run where the use of > `=` and `XX:+FlightRecorder` are disallowed just to make sure you caught them > all? It's a good idea, but it requires changes outside OpenJDK which I rather not do now. I'm sure somebody will reintroduce '=', so this is not so much about getting rid of them all, but to avoid them being propagated, when code is copy pasted for new tests etc. - PR: https://git.openjdk.java.net/jdk/pull/3561
Re: RFR: 8265036: JFR: Remove use of -XX:StartFlightRecording= and -XX:FlightRecorderOptions= [v2]
> Hi, > > Could I have a review of fix that removes the use of "=" together with > -XX:StartFlightRecording and -XX:FlightRecorderOptions. It's been possible to > use "-XX:StartFlightRecording:" and "-XX:FlightRecorderOption:" since JFR was > introduced into OpenJDK (JDK 11), so this is not a change of the > specification, just an update to make the use consistent in tests, comments, > documentation etc. > > I also removed the use of -XX:+FlightRecorder, which is not needed, and has > been deprecated since JDK 13. > > Testing: jdk/jdk/jfr, tier 1-4. > > Thanks > Erik Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Replace '=' in jfrOptionsSet.cpp - Changes: - all: https://git.openjdk.java.net/jdk/pull/3561/files - new: https://git.openjdk.java.net/jdk/pull/3561/files/4ce08939..138bac16 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3561&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3561&range=00-01 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/3561.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3561/head:pull/3561 PR: https://git.openjdk.java.net/jdk/pull/3561
Re: RFR: 8265237: String.join and StringJoiner can be improved further [v4]
> While JDK-8148937 improved StringJoiner class by replacing internal use of > getChars that copies out characters from String elements into a char[] array > with StringBuilder which is somehow more optimal, the improvement was > marginal in speed (0% ... 10%) and mainly for smaller strings, while GC was > reduced by about 50% in average per operation. > Initial attempt to tackle that issue was more involved, but was later > discarded because it was apparently using too much internal String details in > code that lives outside String and outside java.lang package. > But there is another way to package such "intimate" code - we can put it into > String itself and just call it from StringJoiner. > This PR is an attempt at doing just that. It introduces new package-private > method in `java.lang.String` which is then used from both pubic static > `String.join` methods as well as from `java.util.StringJoiner` (via > SharedSecrets). The improvements can be seen by running the following JMH > benchmark: > > https://gist.github.com/plevart/86ac7fc6d4541dbc08256cde544019ce > > The comparative results are here: > > https://jmh.morethan.io/?gist=7eb421cf7982456a2962269137f71c15 > > The jmh-result.json files are here: > > https://gist.github.com/plevart/7eb421cf7982456a2962269137f71c15 > > Improvement in speed ranges from 8% (for small strings) to 200% (for long > strings), while creation of garbage has been further reduced to an almost > garbage-free operation. > > So WDYT? Peter Levart has updated the pull request incrementally with one additional commit since the last revision: Fork JVM 3 times in JMH test to smooth out variance in a particular fork - Changes: - all: https://git.openjdk.java.net/jdk/pull/3501/files - new: https://git.openjdk.java.net/jdk/pull/3501/files/a2d5e819..84af9e6c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3501&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3501&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3501.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3501/head:pull/3501 PR: https://git.openjdk.java.net/jdk/pull/3501
