Re: RFR: 8336934: Clean up JavaLangReflectAccess
On Tue, 23 Jul 2024 04:10:38 GMT, Chen Liang wrote: > Removed redundant APIs in `JavaLangReflectAccess` and added general warning > against using `SharedSecrets`. src/java.base/share/classes/java/lang/reflect/Constructor.java line 168: > 166: } > 167: > 168: /** Creates a new root constructor with a custom accessor for > serialization hooks. */ Preserve "/**" comments for generated javadoc, a "//" comment would be more appropriate for a 1-liner. src/java.base/share/classes/jdk/internal/access/JavaLangReflectAccess.java line 53: > 51: // > 52: > 53: // Personal editor comments should not be included. src/java.base/share/classes/jdk/internal/access/JavaLangReflectAccess.java line 68: > 66: > 67: // > 68: Remove - PR Review Comment: https://git.openjdk.org/jdk/pull/20290#discussion_r1692174982 PR Review Comment: https://git.openjdk.org/jdk/pull/20290#discussion_r1692164266 PR Review Comment: https://git.openjdk.org/jdk/pull/20290#discussion_r1692165039
Re: RFR: 8336934: Clean up JavaLangReflectAccess
On Tue, 23 Jul 2024 04:10:38 GMT, Chen Liang wrote: > Removed redundant APIs in `JavaLangReflectAccess` and added general warning > against using `SharedSecrets`. The description should include a summary of the changes and the rationale. The Jira description is a bit better and could also be more complete. - PR Comment: https://git.openjdk.org/jdk/pull/20290#issuecomment-2251449428
Re: RFR: 8335791: Speed up j.u.Formatter & String.format [v4]
On Sat, 6 Jul 2024 00:15:04 GMT, Shaojin Wen wrote:
>> String.format is widely used, and improving its performance is very
>> meaningful. This PR can significantly improve the performance of
>> String.format. Sorry, there are many changes, which will take a lot of time.
>> I hope you can review it patiently.
>>
>>
>> Improved performance includes the following:
>>
>> ## 1. Write directly during the parse process to reduce object allocation.
>>
>> In the current Formatter implementation, some objects do not need to be
>> allocated, such as:
>>
>>
>> class Formatter {
>> public Formatter format(Locale l, String format, Object ... args) {
>> List fsa = parse(format);
>> // ...
>> }
>>
>> static List parse(String s) {
>> ArrayList al = new ArrayList<>();
>>
>> while (i < max) {
>> int n = s.indexOf('%', i);
>> if (n < 0) {
>> //
>> al.add(new FixedString(s, i, max));
>> }
>> }
>> }
>> }
>>
>> In the process of parsing, the content that is not a Specifier is directly
>> appended without going through FixedString. By directly printing the parsed
>> FormatString object, there is no need to construct a `List
>> fsa` to store it.
>>
>> ## 2. Fast path print
>> Use specialized FormatString implementations for single-character and
>> single-width specifiers to avoid calling the large FormatSpecifier#print
>> method.
>>
>> ## 3. String.format directly calls j.u.Formatter
>> String.format directly calls j.u.Formatter via SharedSecrets to improve
>> performance
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Implementing JavaUtilFormatterAccess using anonymous class
Re-designing the formatting implementation should wait for the re-emergence of
the templates project and implementation. It has similar performance goals and
it would be beneficial to have a single implementation of the lower levels to
avoid code duplication and take advantage of the optimizations.
-
PR Comment: https://git.openjdk.org/jdk/pull/20055#issuecomment-2243122522
Re: RFR: 8325109: Sort method modifiers in canonical order
On Thu, 1 Feb 2024 11:57:04 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on > [JDK-8324053](https://bugs.openjdk.org/browse/JDK-8324053). I have run the > bin/blessed-modifier-order.sh on the entire code base, and manually checked > the result. I have reverted all but these trivial and uncontroversial changes. > > Almost all of these are about moving `static` to its proper position; a few > do not involve `static` but instead puts `final` or `abstract` in the correct > place. > > I have deliberately stayed away from `default` in this PR, since they should > probably get a more thorough treatment, see > https://github.com/openjdk/jdk/pull/17468#pullrequestreview-1829238473. lgtm; all look good - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17670#pullrequestreview-1857031136
Re: RFR: 8324646: Avoid Class.forName in SecureRandom constructor [v2]
On Wed, 24 Jan 2024 17:57:38 GMT, Oli Gillespie wrote: >> src/java.base/share/classes/java/security/Provider.java line 1560: >> >>> 1558: final boolean supportsParameter; >>> 1559: final String constructorParameterClassName; >>> 1560: private volatile Class constructorParameterClass; >> >> Style: no need for `private` here, match what other fields are doing. > > I don't disagree in principle but it was like this before the revert, and is > still like this in 17. Is volatile really needed? And there is some performance penalty and in practice the value will be the same even if recomputed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465483019
Re: RFR: 8275338: Add JFR events for notable serialization situations [v14]
On Wed, 10 Jan 2024 18:56:45 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Small space optimization. Thanks for the updates. LGTM - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17129#pullrequestreview-1818884173
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Wed, 10 Jan 2024 17:41:41 GMT, Raffaello Giulietti wrote: >> The spec is silent about methods being 'native'; it would generally be >> impractical to implement native methods for these purposes, but a native >> method can implement the behavior. > > @RogerRiggs The checks are agnostic about a method being `native` or not, so > I'm not sure I get your point about `native` methods. Right, there's nothing to change; a method declared as native is not mis-declared. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1447785756
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 15:58:10 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java >> line 185: >> >>> 183: commitEvent(PRIV_METH_NON_STATIC, >>> 184: m + " must be non-static to be effective"); >>> 185: } >> >> Should there also be a check to see if this `private` method is >> (misdeclared) as a `native` method? > > I'm not sure that a `native` method is not considered effective by > serialization. I have to check. The spec is silent about methods being 'native'; it would generally be impractical to implement native methods for these purposes, but a native method can implement the behavior. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1447665831
Re: RFR: 8275338: Add JFR events for notable serialization situations [v13]
On Wed, 10 Jan 2024 15:43:46 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Changes according to reviewers feedback.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 129:
> 127: Object spf = objectFromStatic(f);
> 128: if (spf == null) {
> 129: commitEvent(cl, SERIAL_PERSISTENT_FIELDS_NAME + " must be
> non-null");
"must" -> "should".
The [serialization
spec](https://docs.oracle.com/en/java/javase/21/docs/specs/serialization/serial-arch.html#defining-serializable-fields-for-a-class)
describes the behavior if the field is null.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1447662278
Re: RFR: 8275338: Add JFR events for notable serialization situations [v12]
On Tue, 9 Jan 2024 10:41:31 GMT, Raffaello Giulietti
wrote:
>> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
>> line 53:
>>
>>> 51: private static final Class[] READ_OBJECT_NO_DATA_PARAM_TYPES =
>>> {};
>>> 52: private static final Class[] WRITE_REPLACE_PARAM_TYPES = {};
>>> 53: private static final Class[] READ_RESOLVE_PARAM_TYPES = {};
>>
>> These could share a single zero length Class array.
>
> Conceptually, these are independent types. There's no logical relationship
> between them. Sharing a zero length array would convey a false sense of
> logical sharing.
true, but its wasted space and IMHO ok as a local and private implementation
detail.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1447655018
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v11]
On Wed, 10 Jan 2024 14:58:37 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/java/math/BigInteger.java line 3998: >> >>> 3996: int i = ArraysSupport.mismatch(m1, m2, len1); >>> 3997: if (i != -1) >>> 3998: return Integer.compareUnsigned(m1[i], m2[i]) < 0 ? -1 : 1; >> >> Just an observation. The (Java and intrinsic) implementation of >> Integer.compareUnsigned already returns -1, 0, 1. >> Returning `Integer.compareUnsigned(m1[i], m2[i])` would yield the same >> result without the tertiary expression. > > Yes, that's what was proposed > [here](https://github.com/openjdk/jdk/pull/14630#discussion_r1242245875) some > time ago. > > But the spec of `compareUnsigned()` does _not_ guarantee a -1, 0, 1 result, > so there's a (minimal) risk when returning its value directly. (For some > reason, `BigInteger` specifies a -1, 0, 1 outcome.) In expressing intent, perhaps `Integer.signum(Integer.compareUnsigned(m1[i], [m2[i]))`. Though it may all be for nothing if C2 can optimize it away. - PR Review Comment: https://git.openjdk.org/jdk/pull/14630#discussion_r1447521230
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v11]
On Wed, 10 Jan 2024 11:27:53 GMT, Pavel Rappo wrote: >> Please review this PR to use modern APIs and language features to simplify >> equals, hashCode, and compareTo for BigInteger. If you have any performance >> concerns, please raise them. >> >> This PR is cherry-picked from a bigger, not-yet-published PR, to test the >> waters. That latter PR will be published soon. > > Pavel Rappo has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 20 commits: > > - Use Integer.compareUnsigned > - Update copyright years and headers > - Merge branch 'master' into 8310813 > - Merge branch 'master' into 8310813 > - Merge branch 'master' into 8310813 > - Merge branch 'master' into 8310813 > - Merge branch 'master' into 8310813 > - Fix bugs in Shared.createSingle > - Merge branch 'master' into 8310813 > - Group params coarser (suggested by @cl4es) > >- Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. > Every testXYZ method invokes M operations, where M is the maximum > number of elements in a group. Shorter groups are cyclically padded. >- Uses the org.openjdk.jmh.infra.Blackhole API and increases > benchmark time. >- Fixes a bug in Shared that precluded 0 from being in a pair. > - ... and 10 more: https://git.openjdk.org/jdk/compare/bc05893f...08e6adca src/java.base/share/classes/java/math/BigInteger.java line 3998: > 3996: int i = ArraysSupport.mismatch(m1, m2, len1); > 3997: if (i != -1) > 3998: return Integer.compareUnsigned(m1[i], m2[i]) < 0 ? -1 : 1; Just an observation. The (Java and intrinsic) implementation of Integer.compareUnsigned already returns -1, 0, 1. Returning `Integer.compareUnsigned(m1[i], m2[i])` would yield the same result without the tertiary expression. - PR Review Comment: https://git.openjdk.org/jdk/pull/14630#discussion_r1447488470
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v10]
On Tue, 2 Jan 2024 14:37:27 GMT, Pavel Rappo wrote: >> Please review this PR to use modern APIs and language features to simplify >> equals, hashCode, and compareTo for BigInteger. If you have any performance >> concerns, please raise them. >> >> This PR is cherry-picked from a bigger, not-yet-published PR, to test the >> waters. That latter PR will be published soon. > > Pavel Rappo 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 17 additional > commits since the last revision: > > - Merge branch 'master' into 8310813 > - Merge branch 'master' into 8310813 > - Merge branch 'master' into 8310813 > - Merge branch 'master' into 8310813 > - Fix bugs in Shared.createSingle > - Merge branch 'master' into 8310813 > - Group params coarser (suggested by @cl4es) > >- Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. > Every testXYZ method invokes M operations, where M is the maximum > number of elements in a group. Shorter groups are cyclically padded. >- Uses the org.openjdk.jmh.infra.Blackhole API and increases > benchmark time. >- Fixes a bug in Shared that precluded 0 from being in a pair. > - Use better overloads (suggested by @cl4es) > >- Uses simpler, more suitable overloads for the subrange > starting from 0 > - Improve benchmarks > - Merge branch 'master' into 8310813 > - ... and 7 more: https://git.openjdk.org/jdk/compare/c09491be...252b7378 LGTM - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14630#pullrequestreview-1811620453
Re: RFR: 8275338: Add JFR events for notable serialization situations [v12]
On Mon, 8 Jan 2024 13:48:06 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti 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 15 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8275338
> - Simplified event messages.
>Remove ckecker allocation.
> - Corrected @Label of event and of field.
> - Removed @module from test.
> - Merge branch 'master' into 8275338
> - Renamed an event field.
> - Minor changes.
> - Removed event kind.
>serialVersionUID must have type long.
>Test now base on keyword search in event message.
>Commented test classes about misdeclarations.
> - Changes according to reviewer's comments.
> - Better name for a label, corrected name of removed field.
> - ... and 5 more: https://git.openjdk.org/jdk/compare/8d57faa7...9ca1f36d
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 53:
> 51: private static final Class[] READ_OBJECT_NO_DATA_PARAM_TYPES = {};
> 52: private static final Class[] WRITE_REPLACE_PARAM_TYPES = {};
> 53: private static final Class[] READ_RESOLVE_PARAM_TYPES = {};
These could share a single zero length Class array.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 70:
> 68: privilegedCheckAccessibleMethod(cl, WRITE_REPLACE_NAME,
> 69: WRITE_REPLACE_PARAM_TYPES, Object.class);
> 70: privilegedCheckAccessibleMethod(cl, READ_RESOLVE_NAME,
Thinking ahead to when the security manager is removed, can the code that needs
private access to field values (SUID) be more narrowly accessed? Perhaps switch
to using a VarHandle and MethodHandles.Lookup. This may be a longer term issue
and beyond the scope of this PR.
In the naming of the `PrivilegedXXX` methods, I would drop the "privileged"
part of the name.
The methods do not change the privilege level and operate correctly if when the
caller has the privileges needed. And all of these methods are read-only so
there is no/little risk in their implementations and avoid refitting the
terminology later.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 87:
> 85: }
> 86: if (cl.isEnum()) {
> 87: commitEvent(cl, SUID_NAME + " in an enum class is not
> effective");
Is there a best practice that can be included in the message? "SUID should not
be declared"?
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 113:
> 111: } else if (cl.isEnum()) {
> 112: commitEvent(cl, SERIAL_PERSISTENT_FIELDS_NAME +
> 113: " in an enum class is not effective");
Is there best practice to include in the message? "SPFN should not be declared"?
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 33:
> 31: import org.junit.jupiter.params.provider.MethodSource;
> 32:
> 33: import java.io.*;
Explicit imports are preferred.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445102883
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445129715
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445107271
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445108891
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445137904
Re: RFR: 8275338: Add JFR events for notable serialization situations [v10]
On Thu, 21 Dec 2023 09:53:10 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Corrected @Label of event and of field.
src/java.base/share/classes/java/io/ObjectStreamClass.java line 466:
> 464:
> 465: if (SerializationMisdeclarationEvent.enabled() && serializable) {
> 466: new
> SerializationMisdeclarationChecker(cl).checkMisdeclarations();
Is there any benefit to avoiding the allocation and passing the class through
the methods as an extra arg?
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 149:
> 147: if (!(spf instanceof ObjectStreamField[])) {
> 148: commitEvent(SERIAL_PERSISTENT_FIELDS_NAME +
> 149: " must be an instance of ObjectStreamField[] to be
> effective");
Hmmm...
The terminology "to be effective" seems a bit indirect and may lead to some
head scratching.
In most cases it means it is ignored.
I might suggest to just state the condition that is required and drop the extra
phrase.
For example, "xxx should be an instance of ObjectStreamField[]".
It would result in shorter messages and if further explanation of the message
is needed it can more completely elaborate on the impact of the incorrect
declaration.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 165:
> 163: commitEvent("method " + m + " on an enum class is not
> effective");
> 164: } else if (cl.isRecord()) {
> 165: commitEvent("method " + m + " on an record class is not
> effective");
"an record" -> "a record"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434524764
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434512690
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434513714
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Wed, 20 Dec 2023 14:28:39 GMT, Roger Riggs wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Changes according to reviewer's comments. > > It would also be useful/interesting to include a test that checks every > Serializable class (by Invoking `ObjectStreamClass.lookup(clazz)`) in the > Java runtime and reports any jfr events. > Fixing them would be a separate task. The compiler warnings from last year > should have fixed most/many non-conforming classes. > @RogerRiggs Do you mean a permanent test in the codebase, or just a throwaway > run? Anyway, interesting suggestion. A separate PR to add permanent test would advise about the current state and prevent new cases. If there are cases that can't be fixed (because of some kind of backward compatibility issue), there might need to be an exclusion list. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1864639166
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Wed, 20 Dec 2023 15:01:02 GMT, Raffaello Giulietti
wrote:
>> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
>> line 113:
>>
>>> 111: if (longFromStatic(f) == null) {
>>> 112: commitEvent(SUID_CONVERTIBLE_TO_LONG,
>>> 113: SUID_NAME + " must be convertible to long via
>>> widening to be effective");
>>
>> The serialization spec only shows using long. If any recommendation is made
>> it should be to declare the field as a `long`
>
> There's a check on the type at L.104 which is about the "should"
> recommendation, since serialization does not care about the type of the field
> being `long`.
>
> This check is about the value at runtime, which is a "must" because
> serialization expects it to be convertible to long by widening.
The implementation should not show through. I don't remember if the particular
implementation was intentional or just a shortcut to get the value. But any
suggestion that implies a fix should be to recommend the best practice.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432834402
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 16:45:04 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Changes according to reviewer's comments. It would also be useful/interesting to include a test that checks every Serializable class (by Invoking `ObjectStreamClass.lookup(clazz)`) in the Java runtime and reports any jfr events. Fixing them would be a separate task. The compiler warnings from last year should have fixed most/many non-conforming classes. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1864569540
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 16:45:04 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Changes according to reviewer's comments.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 113:
> 111: if (longFromStatic(f) == null) {
> 112: commitEvent(SUID_CONVERTIBLE_TO_LONG,
> 113: SUID_NAME + " must be convertible to long via
> widening to be effective");
The serialization spec only shows using long. If any recommendation is made it
should be to declare the field as a `long`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432778556
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Wed, 20 Dec 2023 08:29:19 GMT, Daniel Fuchs wrote: >> You could define them with an Enum but use the ordinal as the value for JFR. > > Same remark here about finality as > https://github.com/openjdk/jdk/pull/17129#discussion_r1432400888. public > statics should be final. I'd also remove the error codes, the string messages will be pretty stable and the extra surface area of the constants is just maintenance overhead. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432772028
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Better name for a label, corrected name of removed field.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 108:
> 106: SUID_NAME + " should be declared of type long");
> 107: }
> 108: if (!isStatic(f)) {
The two calls to isStatic could be reordered closer together to be a single if
(isstatic()) { ... } else {... }.
src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java
line 126:
> 124: public static int ACC_METH_PARAM_TYPES= 404;
> 125: public static int ACC_METH_NON_ACCESSIBLE = 405;
> 126:
I'd rather see few (fewer, just one) kinds of events, with so many different
kinds of events there needs to be a convenient method to look for any kind of
serialization mis-declaration.
Perhaps a single event with flags for the different kinds of errors.
The event classes could be responsible for turning the flags back into useful
messages on display.
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 157:
> 155: }
> 156:
> 157: private static class A implements Serializable {
The test classes should document the good or badness of the class either in the
class/field/method names or in comments. What's wrong with this XXX.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431846034
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431850091
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431856800
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 17:41:57 GMT, Raffaello Giulietti wrote: >> Users (not OpenJDK developers) don't know what the error code means. I think >> it's better to not have them. This is how other events work. If you want to >> guard against changes, I would export the package to the test. > > What about fixed `String`s rather than `int`s for the kind of error? > Something like `"SUID_INEFFECTIVE_ON_ENUM"`, and so on? > It would be nice to be able to use enums, but AFAIK that's not supported in > JFR. You could define them with an Enum but use the ordinal as the value for JFR. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431864329
Re: RFR: 8315097: Rename createJavaProcessBuilder [v7]
On Wed, 25 Oct 2023 08:44:29 GMT, Leo Korinth wrote: >> This pull request renames `createJavaProcessBuilder` to >> `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to >> `createTestJavaProcessBuilder`. Both are implemented through a private >> `createJavaProcessBuilder`. It also updates the java doc. >> >> This is so that it should be harder to by mistake use >> `createLimitedTestJavaProcessBuilder` that is problematic because it will >> not forward JVM flags to the tested JVM. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > fix copyright year and indentation ok, to correct the javadoc in a subsequent PR. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1700061972
Re: RFR: 8315097: Rename createJavaProcessBuilder [v7]
On Wed, 25 Oct 2023 08:44:29 GMT, Leo Korinth wrote:
>> This pull request renames `createJavaProcessBuilder` to
>> `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to
>> `createTestJavaProcessBuilder`. Both are implemented through a private
>> `createJavaProcessBuilder`. It also updates the java doc.
>>
>> This is so that it should be harder to by mistake use
>> `createLimitedTestJavaProcessBuilder` that is problematic because it will
>> not forward JVM flags to the tested JVM.
>
> Leo Korinth has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix copyright year and indentation
Suggestions to complete the descriptions of the createXXXJavaProcessBuilder
methods.
test/lib/jdk/test/lib/process/ProcessTools.java line 505:
> 503: * @return The ProcessBuilder instance representing the java command.
> 504: */
> 505: public static ProcessBuilder
> createTestJavaProcessBuilder(List command) {
Include the same description of other properties that are included in creating
the ProcessBuilder...
``` * Unless the "test.noclasspath" property is "true"
* the classpath property "java.class.path" is appended to the command line
and
* the environment of the ProcessBuilder is modified to remove "CLASSPATH".
* If the property "test.thread.factory" is provided the command args are
* updated and appended to invoke ProcessTools main() and provide the
* name of the thread factory.
* The "-Dtest.thread.factory" is appended to the arguments with the thread
factory value.
* The remaining command args are scanned for unsupported options and
* are appended to the ProcessBuilder.
test/lib/jdk/test/lib/process/ProcessTools.java line 520:
> 518: * @return The ProcessBuilder instance representing the java command.
> 519: */
> 520: public static ProcessBuilder createTestJavaProcessBuilder(String...
> command) {
Include the same description of other properties that are included in creating
the ProcessBuilder...
* Unless the "test.noclasspath" property is "true"
* the classpath property "java.class.path" is appended to the command line
and
* the environment of the ProcessBuilder is modified to remove "CLASSPATH".
* If the property "test.thread.factory" is provided the command args are
* updated and appended to invoke ProcessTools main() and provide the
* name of the thread factory.
* The "-Dtest.thread.factory" is appended to the arguments with the thread
factory value.
* The remaining command args are scanned for unsupported options and
* are appended to the ProcessBuilder.
test/lib/jdk/test/lib/process/ProcessTools.java line 538:
> 536: * it in combination with @requires vm.flagless JTREG
> 537: * anotation as to not waste energy and test resources.
> 538: *
Consider adding this description of what this method does.
Suggestion:
* Unless the "test.noclasspath" property is "true"
* the classpath property "java.class.path" is appended to the command line
and
* the environment of the ProcessBuilder is modified to remove "CLASSPATH".
* If the property "test.thread.factory" is provided the command args are
* updated and appended to invoke ProcessTools main() and provide the
* name of the thread factory.
* The "-Dtest.thread.factory" is appended to the arguments with the thread
factory value.
* The remaining command args are scanned for unsupported options and
* are appended to the ProcessBuilder.
test/lib/jdk/test/lib/process/ProcessTools.java line 560:
> 558: * it in combination with @requires vm.flagless JTREG
> 559: * anotation as to not waste energy and test resources.
> 560: *
Suggestion:
* Unless the "test.noclasspath" property is "true"
* the classpath property "java.class.path" is appended to the command line
and
* the environment of the ProcessBuilder is modified to remove "CLASSPATH".
* If the property "test.thread.factory" is provided the command args are
* updated and appended to invoke ProcessTools main() and provide the
* name of the thread factory.
* The "-Dtest.thread.factory" is appended to the arguments with the thread
factory value.
* The remaining command args are scanned for unsupported options and
* are appended to the ProcessBuilder.
-
PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1698308785
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1372364800
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1372364171
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1372361862
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1372362333
Re: RFR: 8315097: Rename createJavaProcessBuilder [v6]
On Tue, 24 Oct 2023 07:49:30 GMT, Leo Korinth wrote:
>> This pull request renames `createJavaProcessBuilder` to
>> `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to
>> `createTestJavaProcessBuilder`. Both are implemented through a private
>> `createJavaProcessBuilder`. It also updates the java doc.
>>
>> This is so that it should be harder to by mistake use
>> `createLimitedTestJavaProcessBuilder` that is problematic because it will
>> not forward JVM flags to the tested JVM.
>
> Leo Korinth has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - static import
> - copyright
> - whitespace
> - whitespace
> - sed
> - fix test/lib/jdk/test/lib/process/ProcessTools.java
test/lib/jdk/test/lib/process/ProcessTools.java line 506:
> 504: */
> 505: public static ProcessBuilder
> createTestJavaProcessBuilder(List command) {
> 506: return
> createTestJavaProcessBuilder(command.toArray(String[]::new));
The javadoc shoul d describe all of the options being added to the
ProcessBuilder.
They were inadequated described previously and still are.
The other options (seem to be from the code), test.noclasspath,
java.class.path, and test.thread.factory.
The description of test.thread.factory and addTestThreadFactoryArgs method
seems inadequately described.
test/lib/jdk/test/lib/process/ProcessTools.java line 527:
> 525: * Create ProcessBuilder using the java launcher from the jdk to
> 526: * be tested.
> 527: *
As above, should described the limited options that are added to the
ProcessBuilder, the same as for `reateTestJavaProcessBuilder(...)`
test/lib/jdk/test/lib/process/ProcessTools.java line 549:
> 547: * Create ProcessBuilder using the java launcher from the jdk to
> 548: * be tested.
> 549: *
As above, should described the limited options that are added to the
ProcessBuilder, the same as for reateTestJavaProcessBuilder(...)
test/lib/jdk/test/lib/process/ProcessTools.java line 599:
> 597: */
> 598: public static OutputAnalyzer executeTestJvm(String... cmds) throws
> Exception {
> 599: ProcessBuilder pb = createTestJavaProcessBuilder(cmds);
This should also describe *all* of the options being set in the ProcessBuilder
before executing the process.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1370728371
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1370729609
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1370729925
PR Review Comment: https://git.openjdk.org/jdk/pull/15452#discussion_r1370730637
Re: RFR: 8318200: String::newStringNoRepl and String::getBytesNoRepl does not throw CharacterCodingException [v5]
On Wed, 18 Oct 2023 16:39:47 GMT, Alan Bateman wrote: > We have to be very careful with proposals like this as it means code outside > of java.lang having access to the underlying bytes. I think other > alternatives needs to be explored to avoid this and related concerns. Yes, adding another method in the "NoRepl" family needs to be done with care and be narrowly focused. JLA already exposes several methods that allow the contents of strings to be read from the underlying value or created from caller provided buffers. All to avoid extra allocations and copying. - PR Comment: https://git.openjdk.org/jdk/pull/16209#issuecomment-1769026087
Re: RFR: 8318200: String::newStringNoRepl and String::getBytesNoRepl does not throw CharacterCodingException [v5]
On Tue, 17 Oct 2023 14:22:10 GMT, Shaojin Wen wrote:
>> When calling String::newStringNoRepl and String::getBytesNoRepl, we need to
>> use try...catch to handle CharacterCodingException and throw
>> IllegalArgumentException instead of CharacterCodingException to make the
>> calling code cleaner.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix from @AlanBateman 's review
The new usages that are driving this change would be better served by a method
dedicated to creating latin1 strings from a caller provided byte array. The
JavaLangAccess shared secret interface is already overused, but it is cleaner,
more maintainable, and fit for purpose to add a method than to contort the
exception handling as proposed.
/**
* Return a String constructed using the byte array containing latin1
(ISO-8859-1) characters.
* The byte array must not be modified or otherwise referenced or used
after the String is created.
* If COMPACT_STRINGS is true the coder will be LATIN1 and the byte array
is the string value;
* otherwise the contents are inflated to create a UTF16 string.
*/
public String newStringLatin1NoRepl(byte[] src) {
if (COMPACT_STRINGS)
return new String(src, LATIN1);
return new String(StringLatin1.inflate(src, 0, src.length), UTF16);
}
Creating strings from other encodings that may or have encoding errors should
continue to use the current function and handle CCE as required.
-
PR Comment: https://git.openjdk.org/jdk/pull/16209#issuecomment-1768911569
Re: RFR: 8318200: String::newStringNoRepl and String::getBytesNoRepl does not throw CharacterCodingException [v5]
On Tue, 17 Oct 2023 14:22:10 GMT, Shaojin Wen wrote: >> When calling String::newStringNoRepl and String::getBytesNoRepl, we need to >> use try...catch to handle CharacterCodingException and throw >> IllegalArgumentException instead of CharacterCodingException to make the >> calling code cleaner. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > fix from @AlanBateman 's review Hacking around the exception types isn't a good fix. If the exception is thrown, it is because of a charset conversion error. IllegalArgumentException is for cases where the arguments are incorrect. - Changes requested by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16209#pullrequestreview-1683043411
Re: RFR: 8315999: Improve Date toString performance [v13]
On Wed, 13 Sep 2023 14:22:35 GMT, 温绍锦 wrote: >> improve date toString performance, includes: >> >> java.util.Date.toString >> java.util.Date.toGMTString >> java.time.Instant.toString >> java.time.LocalDate.toString >> java.time.LocalDateTime.toString >> java.time.LocalTime.toString > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > simplify LocalDate::getChars As a consideration to core-libs-dev readers, push commits when you are convinced are ready for review and you don't intend to make more changes. It will improve the signal to noise ratio. Thanks - PR Comment: https://git.openjdk.org/jdk/pull/15658#issuecomment-1718192179
Re: RFR: 8315999: Improve Date toString performance [v13]
On Wed, 13 Sep 2023 14:22:35 GMT, 温绍锦 wrote: >> improve date toString performance, includes: >> >> java.util.Date.toString >> java.util.Date.toGMTString >> java.time.Instant.toString >> java.time.LocalDate.toString >> java.time.LocalDateTime.toString >> java.time.LocalTime.toString > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > simplify LocalDate::getChars Based on the use cases cited, if your library needs specific performance improvements for specific formats, they should be done in that library. - PR Comment: https://git.openjdk.org/jdk/pull/15658#issuecomment-1718186604
Re: RFR: 8315968: Move java.util.Digits to jdk.internal.util and refactor to reduce duplication [v19]
On Tue, 12 Sep 2023 13:27:29 GMT, 温绍锦 wrote: >> java.util.DecimalDigits::DIGITS and java.lang.StringLatin1.PACKED_DIGITS are >> duplicates, We need to move >> java.util.Digits/OctalDigits/DecimalDigits/HexDigits to the >> jdk.internal.util package, and modify these classes to public class, so that >> classes in other packages can also access them. >> >> DecimalDigits::DIGITS provides a new digitPair static method, used to >> replace StringLatin1.PACKED_DIGITS access. >> >> In order to be consistent with the original StringLatin1.PACKED_DIGITS, >> OctalDigits::DIGITS and DecimalDigits::DIGITS are modified to little-endian. >> HexDigits::DIGITS will also be modified after PR #14745 is merged. >> >> These changes will be used by PR #15658 #1 > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > little-endian Thanks for the updates. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15651#pullrequestreview-1622614379
Re: RFR: 8315968: Move java.util.Digits to jdk.internal.util and refactor to reduce duplication [v19]
On Tue, 12 Sep 2023 14:13:06 GMT, 温绍锦 wrote: > The title has been updated, please help update the title of JIRA The description needs an update too. - PR Comment: https://git.openjdk.org/jdk/pull/15651#issuecomment-1715814638
Re: RFR: 8315999: Improve Date toString performance [v7]
On Tue, 12 Sep 2023 13:05:19 GMT, 温绍锦 wrote: >> improve date toString performance, includes: >> >> java.util.Date.toString >> java.util.Date.toGMTString >> java.time.Instant.toString >> java.time.LocalDate.toString >> java.time.LocalDateTime.toString >> java.time.LocalTime.toString > > 温绍锦 has updated the pull request incrementally with two additional commits > since the last revision: > > - improved DateTimeFormatter.format > - improved ISO_INSTANT format You haven't make the case for these changes, please describe the use cases when performance is a significant constraint on application performance. The changes largely just add more code to maintain without otherwise adding sufficient value. - PR Comment: https://git.openjdk.org/jdk/pull/15658#issuecomment-1715774423
Re: RFR: 8315968: Consolidate java.util.Digits and StringLatin1::PACKED_DIGITS [v19]
On Tue, 12 Sep 2023 13:27:29 GMT, 温绍锦 wrote: >> Some codes in core libs are duplicated, including: >> java.util.DecimalDigits::DIGITS -> java.lang.StringLatin1.PACKED_DIGITS >> java.util.DecimalDigits::size -> java.lang.Long.stringSize >> >> We can reduce duplication through JavaLangAccess, which is also needed in >> other places, such as: >> https://github.com/openjdk/jdk/pull/1 > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > little-endian Please update this PR title and description to indicate this refactoring to move to jdk.internal.util. I can update the Jira title and description to match after that. - PR Comment: https://git.openjdk.org/jdk/pull/15651#issuecomment-1715764494
Re: RFR: 8315968: Consolidate java.util.Digits and StringLatin1::PACKED_DIGITS [v7]
On Mon, 11 Sep 2023 16:36:55 GMT, 温绍锦 wrote:
>> src/java.base/share/classes/jdk/internal/util/DecimalDigits.java line 143:
>>
>>> 141: * code after loop unrolling.
>>> 142: */
>>> 143: public static int stringSize(int x) {
>>
>> I suggest splitting the moves of `stringSize` `getChars` into a new PR
>> dependent on this one; your future date and time optimizations can depend on
>> that one, which exposes stringSize.
>>
>> Having the DecimalDigits package move and `stringSize` `getChars` moves
>> together complicates the file changes, and it's hard to detect if there's
>> any accidental typo/malicious code in the new additions.
>
> If this PR is split into two PRs, the other two PRs I submitted #15658 #1
> cannot be based on this PR.
I agree, do the package move separate from the refactoring.
The other PRs can wait a bit or be committed as is and take part in the
refactoring later.
One step at a time please.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321957533
Re: RFR: 8315968: Consolidate java.util.Digits and StringLatin1::PACKED_DIGITS [v10]
On Mon, 11 Sep 2023 15:57:22 GMT, 温绍锦 wrote: >> Some codes in core libs are duplicated, including: >> java.util.DecimalDigits::DIGITS -> java.lang.StringLatin1.PACKED_DIGITS >> java.util.DecimalDigits::size -> java.lang.Long.stringSize >> >> We can reduce duplication through JavaLangAccess, which is also needed in >> other places, such as: >> https://github.com/openjdk/jdk/pull/1 > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > revert code format Changes requested by rriggs (Reviewer). src/java.base/share/classes/jdk/internal/util/DecimalDigits.java line 1: > 1: /* Can git be convinced to show this as a rename instead of a delete and add? The history will be cleaner. src/java.base/share/classes/jdk/internal/util/OctalDigits.java line 41: > 39: * Singleton instance of OctalDigits. > 40: */ > 41: public static final Digits INSTANCE = new OctalDigits(); The constructor should be after the field definitions. Don't let some tool reformat the code and move things around. - PR Review: https://git.openjdk.org/jdk/pull/15651#pullrequestreview-1620644060 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321958613 PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1321960820
Re: RFR: 8315968: Consolidate java.util.Digits and StringLatin1::PACKED_DIGITS [v2]
On Sun, 10 Sep 2023 17:59:10 GMT, 温绍锦 wrote:
>> Some codes in core libs are duplicated, including:
>> java.util.DecimalDigits::DIGITS -> java.lang.StringLatin1.PACKED_DIGITS
>> java.util.DecimalDigits::size -> java.lang.Long.stringSize
>>
>> We can reduce duplication through JavaLangAccess, which is also needed in
>> other places, such as:
>> https://github.com/openjdk/jdk/pull/1
>
> 温绍锦 has updated the pull request incrementally with one additional commit
> since the last revision:
>
> move java.util.DecimalDigits to jdk.internal.util.DecimalDigits
This would cleaner and easier to review, if you moved the classes together and
did not refactor the functions. Only include the dependencies. Update the
functions in a separate PR. (We can retitle the issue to match).
src/java.base/share/classes/java/lang/Long.java line 45:
> 43: import static java.lang.String.UTF16;
> 44:
> 45: import static jdk.internal.util.DecimalDigits.stringSize;
Please don't use static import for methods. It makes it much harder to find
where they come from.
src/java.base/share/classes/java/util/Digits.java line 36:
> 34: * @since 21
> 35: */
> 36: sealed interface Digits permits HexDigits, OctalDigits {
Don't break up the trio, move all three classes and the interface to
jdk.internal.util.
I don't see the value in the INSTANCE values but keep it intact.
-
Changes requested by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15651#pullrequestreview-1618893492
PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1320839379
PR Review Comment: https://git.openjdk.org/jdk/pull/15651#discussion_r1320839614
Re: RFR: 8315968: Consolidate java.util.Digits and StringLatin1::PACKED_DIGITS
On Sun, 10 Sep 2023 16:15:01 GMT, 温绍锦 wrote: > Some codes in core libs are duplicated, including: > java.util.DecimalDigits::DIGITS -> java.lang.StringLatin1.PACKED_DIGITS > java.util.DecimalDigits::size -> java.lang.Long.stringSize > > We can reduce duplication through JavaLangAccess, which is also needed in > other places, such as: > https://github.com/openjdk/jdk/pull/1 Instead of packing more stuff into SharedSecrets, how about moving some common utilities into package jdk.internal.util or a new package jdk.internal.string. For example, Digits, HexDigits, DecimalDigits, OctalDigits. That would make access better without the overhead shared secrets. - PR Comment: https://git.openjdk.org/jdk/pull/15651#issuecomment-1712866040
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Mon, 4 Sep 2023 11:01:23 GMT, Leo Korinth wrote: > What do you prefer? Do you have a better alternative? Do someone still think > the current code is good? I think what we have today is inferior to all these > improvements, and I would like to make it harder to develop bad test ca The current API (name) is fine and fit for purpose; it does not promise or hide extra functionality under a simple name. There needs to be an explicit intention in the test(s) to support after the fact that arbitrary flags can be added. @AlanBateman's proposal for naming [above](https://github.com/openjdk/jdk/pull/15452#issuecomment-1700459277) (or similar) would capture more clearly that test options are propagated to the child process. Every test writer should be aware that additional command line options may be mixed in. There are many cases in which the ProcessTools APIs are not used to create child processes and do not need to be used in writing tests. They provide some convenience but also add a dependency and another API layer to work through in the case of failures. As far as I'm aware, there is no general guidance or design pattern outside of hotspot tests to propagate flags or use ProcessTools. Adding that as a requirement will need a different level of communication and change. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1707072375
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > fix static import In the context of the goal is to declaratively identify tests that do or do not benefit from additional test flags renaming the `createjavaProcessBuilder` method does not further that goal. The method name and javadoc of `createjavaProcessBuilder` do not imply that the test options are consulted or used; it only says it creates a ProcessBuilder, and does not promise or document more than that. The javadoc should probably describe the use of the the three properties that modify the way that the java is launched. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1699362435
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
On Tue, 29 Aug 2023 14:06:01 GMT, Roger Riggs wrote: >> Leo Korinth has updated the pull request incrementally with one additional >> commit since the last revision: >> >> copyright > > I don't think this is the best change across so many files. > It gives a very ugly name to a common test function and affects a very large > number of tests. > @RogerRiggs If it is only the name you want changed, maybe you can offer a > better name? @lkorinth Sorry for the too short comment; I wanted to make sure it wasn't integrated before I could look at it more closely. Neither the bug report or the PR describe the problem and its ramifications, only the solution. Can you elaborate on the conditions that lead to this. (and include them in the bug report). - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1697803842
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
On Tue, 29 Aug 2023 09:12:51 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > copyright I don't think this is the best change across so many files. It gives a very ugly name to a common test function and affects a very large number of tests. - Changes requested by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1600512718
Re: RFR: 8311170: Simplify and modernize equals and hashCode in security area [v14]
On Tue, 8 Aug 2023 16:17:44 GMT, Pavel Rappo wrote:
>> Please review this PR to use modern APIs and language features to simplify
>> `equals` and `hashCode` in security area.
>>
>> I understand that security area is sensitive and a non-expert, such as
>> myself, should tread carefully; so below are my notes to assist the review.
>>
>> * Unlike `hashCode`, non-secure `equals` implementations are typically
>> short-circuit. But because of "timing attacks", we seem to have specialized
>> implementations, such as `java.security.MessageDigest.isEqual(byte[],
>> byte[])` and a more general `sun.security.util.ByteArrays.isEqual(byte[],
>> int, int, byte[], int, int)`. So while reviewing this PR, take an
>> opportunity to audit the affected `equals` implementations: perhaps some of
>> them need to become secure, not modern. I have no domain knowledge to tell
>> those cases apart, I only note that those cases exist.
>>
>> * This PR sacrifices compatibility for pragmatism: it changes some
>> `hashCode` implementations to produce different values than before to allow
>> more utilization of methods from `Objects` and `Arrays`. To my mind, those
>> changes are **benign**. If you disagree, I'd be happy to discuss that and/or
>> retract the concerning part of the change.
>>
>> * BitArray could be a topic of its own, but I'll do my best to be concise.
>>
>> * Truth to be told, BitArray's `equals` and `hashCode` are not used
>> anywhere in source, and `equals` is only used in one test. For that reason,
>> I refrained from reimplementing internals of `BitArray` using more general
>> `java.util.BitSet`: too much effort and risk for almost nothing.
>> * Speaking of `BitSet`-powered `BitArray`. Such an implementation is not
>> for the faint of heart: there's too much impedance mismatch between data
>> structures that those classes use to store bits. That said, for the sake of
>> testing that it is possible and that I understand the `BitArray` correctly,
>> I actually implemented it using `BitSet`. While that implementation is
>> **NOT** part of this PR, you can have a look at it
>> [here](https://cr.openjdk.org/~prappo/8311170/BitArray.java).
>>
>> * One suggestion to consider is to change this somewhat arcane piece in
>> java.security.UnresolvedPermission.equals:
>>
>> // check certs
>> if (this.certs == null && that.certs != null ||
>> this.certs != null && that.certs == null ||
>> this.certs != null &&
>> this.certs.length != that.certs.length) {
>> return false;
>> }
>>
>> int i,j;
>> boolea...
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Feedback (part 3)
Marked as reviewed by rriggs (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/14738#pullrequestreview-1567834164
Re: RFR: 8311170: Simplify and modernize equals and hashCode in security area [v9]
On Thu, 13 Jul 2023 22:57:49 GMT, Pavel Rappo wrote:
>> Please review this PR to use modern APIs and language features to simplify
>> `equals` and `hashCode` in security area.
>>
>> I understand that security area is sensitive and a non-expert, such as
>> myself, should tread carefully; so below are my notes to assist the review.
>>
>> * Unlike `hashCode`, non-secure `equals` implementations are typically
>> short-circuit. But because of "timing attacks", we seem to have specialized
>> implementations, such as `java.security.MessageDigest.isEqual(byte[],
>> byte[])` and a more general `sun.security.util.ByteArrays.isEqual(byte[],
>> int, int, byte[], int, int)`. So while reviewing this PR, take an
>> opportunity to audit the affected `equals` implementations: perhaps some of
>> them need to become secure, not modern. I have no domain knowledge to tell
>> those cases apart, I only note that those cases exist.
>>
>> * This PR sacrifices compatibility for pragmatism: it changes some
>> `hashCode` implementations to produce different values than before to allow
>> more utilization of methods from `Objects` and `Arrays`. To my mind, those
>> changes are **benign**. If you disagree, I'd be happy to discuss that and/or
>> retract the concerning part of the change.
>>
>> * BitArray could be a topic of its own, but I'll do my best to be concise.
>>
>> * Truth to be told, BitArray's `equals` and `hashCode` are not used
>> anywhere in source, and `equals` is only used in one test. For that reason,
>> I refrained from reimplementing internals of `BitArray` using more general
>> `java.util.BitSet`: too much effort and risk for almost nothing.
>> * Speaking of `BitSet`-powered `BitArray`. Such an implementation is not
>> for the faint of heart: there's too much impedance mismatch between data
>> structures that those classes use to store bits. That said, for the sake of
>> testing that it is possible and that I understand the `BitArray` correctly,
>> I actually implemented it using `BitSet`. While that implementation is
>> **NOT** part of this PR, you can have a look at it
>> [here](https://cr.openjdk.org/~prappo/8311170/BitArray.java).
>>
>> * One suggestion to consider is to change this somewhat arcane piece in
>> java.security.UnresolvedPermission.equals:
>>
>> // check certs
>> if (this.certs == null && that.certs != null ||
>> this.certs != null && that.certs == null ||
>> this.certs != null &&
>> this.certs.length != that.certs.length) {
>> return false;
>> }
>>
>> int i,j;
>> boolea...
>
> Pavel Rappo has updated the pull request incrementally with five additional
> commits since the last revision:
>
> - Feedback: avoid intermediate assignments
> - More previously missed cases
> - Fix: log hashCode as an unsigned long
>
>If they don't match, this test fails:
>test/jdk/jdk/security/logging/TestX509ValidationLog.java
> - Fix: match hashCode implementations
>
>hashCode in the included classes must match that of
>javax.crypto.spec.SecretKeySpec.hashCode.
>
>If they don't match, this test fails:
>test/jdk/javax/crypto/KeyGenerator/CompareKeys.java
> - Fix: revert short-circuiting when destroyed
>
>That change caused this test to fail:
>test/jdk/javax/security/auth/kerberos/KerberosHashEqualsTest.java
Thanks for the thorough checking and testing.
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14738#pullrequestreview-1530476884
Re: RFR: 8311170: Simplify and modernize equals and hashCode in security area [v8]
On Thu, 13 Jul 2023 08:51:23 GMT, Pavel Rappo wrote:
>> Please review this PR to use modern APIs and language features to simplify
>> `equals` and `hashCode` in security area.
>>
>> I understand that security area is sensitive and a non-expert, such as
>> myself, should tread carefully; so below are my notes to assist the review.
>>
>> * Unlike `hashCode`, non-secure `equals` implementations are typically
>> short-circuit. But because of "timing attacks", we seem to have specialized
>> implementations, such as `java.security.MessageDigest.isEqual(byte[],
>> byte[])` and a more general `sun.security.util.ByteArrays.isEqual(byte[],
>> int, int, byte[], int, int)`. So while reviewing this PR, take an
>> opportunity to audit the affected `equals` implementations: perhaps some of
>> them need to become secure, not modern. I have no domain knowledge to tell
>> those cases apart, I only note that those cases exist.
>>
>> * This PR sacrifices compatibility for pragmatism: it changes some
>> `hashCode` implementations to produce different values than before to allow
>> more utilization of methods from `Objects` and `Arrays`. To my mind, those
>> changes are **benign**. If you disagree, I'd be happy to discuss that and/or
>> retract the concerning part of the change.
>>
>> * BitArray could be a topic of its own, but I'll do my best to be concise.
>>
>> * Truth to be told, BitArray's `equals` and `hashCode` are not used
>> anywhere in source, and `equals` is only used in one test. For that reason,
>> I refrained from reimplementing internals of `BitArray` using more general
>> `java.util.BitSet`: too much effort and risk for almost nothing.
>> * Speaking of `BitSet`-powered `BitArray`. Such an implementation is not
>> for the faint of heart: there's too much impedance mismatch between data
>> structures that those classes use to store bits. That said, for the sake of
>> testing that it is possible and that I understand the `BitArray` correctly,
>> I actually implemented it using `BitSet`. While that implementation is
>> **NOT** part of this PR, you can have a look at it
>> [here](https://cr.openjdk.org/~prappo/8311170/BitArray.java).
>>
>> * One suggestion to consider is to change this somewhat arcane piece in
>> java.security.UnresolvedPermission.equals:
>>
>> // check certs
>> if (this.certs == null && that.certs != null ||
>> this.certs != null && that.certs == null ||
>> this.certs != null &&
>> this.certs.length != that.certs.length) {
>> return false;
>> }
>>
>> int i,j;
>> boolea...
>
> Pavel Rappo 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 12 additional
> commits since the last revision:
>
> - Much more cases: apologies for extra review work
> - Merge branch 'master' into 8311170
> - Merge branch 'master' into 8311170
> - Reflow previously missed doc comment
> - Reflow doc comment as suggested
> - Revert for readability
> - Merge branch 'master' into 8311170
> - Be consistent with the rest of the change
> - Fix reported bugs
> - Add even more cases and tidy up
> - ... and 2 more: https://git.openjdk.org/jdk/compare/bb8f1401...457b9c56
Look good, lots of nice cleanup.
src/java.base/share/classes/javax/crypto/CryptoPolicyParser.java line 627:
> 625: retval ^= maxKeySize;
> 626: retval ^= (checkParam ? 100 : 0);
> 627: retval ^= Objects.hashCode(algParamSpec);
After this change, someone will wonder why the code does all the intermediate
assignments.
return Objects.hashCode(alg)
^ Objects.hashCode(exemptionMechanism)
^ (checkParam ? 100 : 0)
^ Objects.hashCode(algParamSpec);
}
Here and elsewhere...
src/java.base/share/classes/sun/security/x509/DistributionPoint.java line 351:
> 349: hash += Objects.hashCode(relativeName);
> 350: hash += Objects.hash(crlIssuer);
> 351: hash += Arrays.hashCode(reasonFlags);
Another case where the intermediate assignments look unnecessary.
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14738#pullrequestreview-1528668165
PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1262696059
PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1262705536
Re: RFR: 8311170: Simplify and modernize equals and hashCode in security area [v6]
On Thu, 6 Jul 2023 19:10:14 GMT, Pavel Rappo wrote:
>> Please review this PR to use modern APIs and language features to simplify
>> `equals` and `hashCode` in security area.
>>
>> I understand that security area is sensitive and a non-expert, such as
>> myself, should tread carefully; so below are my notes to assist the review.
>>
>> * Unlike `hashCode`, non-secure `equals` implementations are typically
>> short-circuit. But because of "timing attacks", we seem to have specialized
>> implementations, such as `java.security.MessageDigest.isEqual(byte[],
>> byte[])` and a more general `sun.security.util.ByteArrays.isEqual(byte[],
>> int, int, byte[], int, int)`. So while reviewing this PR, take an
>> opportunity to audit the affected `equals` implementations: perhaps some of
>> them need to become secure, not modern. I have no domain knowledge to tell
>> those cases apart, I only note that those cases exist.
>>
>> * This PR sacrifices compatibility for pragmatism: it changes some
>> `hashCode` implementations to produce different values than before to allow
>> more utilization of methods from `Objects` and `Arrays`. To my mind, those
>> changes are **benign**. If you disagree, I'd be happy to discuss that and/or
>> retract the concerning part of the change.
>>
>> * BitArray could be a topic of its own, but I'll do my best to be concise.
>>
>> * Truth to be told, BitArray's `equals` and `hashCode` are not used
>> anywhere in source, and `equals` is only used in one test. For that reason,
>> I refrained from reimplementing internals of `BitArray` using more general
>> `java.util.BitSet`: too much effort and risk for almost nothing.
>> * Speaking of `BitSet`-powered `BitArray`. Such an implementation is not
>> for the faint of heart: there's too much impedance mismatch between data
>> structures that those classes use to store bits. That said, for the sake of
>> testing that it is possible and that I understand the `BitArray` correctly,
>> I actually implemented it using `BitSet`. While that implementation is
>> **NOT** part of this PR, you can have a look at it
>> [here](https://cr.openjdk.org/~prappo/8311170/BitArray.java).
>>
>> * One suggestion to consider is to change this somewhat arcane piece in
>> java.security.UnresolvedPermission.equals:
>>
>> // check certs
>> if (this.certs == null && that.certs != null ||
>> this.certs != null && that.certs == null ||
>> this.certs != null &&
>> this.certs.length != that.certs.length) {
>> return false;
>> }
>>
>> int i,j;
>> boolea...
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflow doc comment as suggested
Marked as reviewed by rriggs (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/14738#pullrequestreview-1519605132
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Tue, 27 Jun 2023 15:06:45 GMT, Sean Coffey wrote: >> New functionality in the -XshowSettings menu to display relevant information >> about JDK security configuration > > 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 15 additional > commits since the last revision: > > - Avoid sharing INDENT variables > - Merge branch 'master' into 8281658-showsettings-security > - Don't allow bad subcommand values for security component > - restore more informative help message > - Split long properties for ; also > - Pass PrintStream to security helper > - Refactor out security code to helper class > - Print aliases. Order Provider type/service output. > - Incorporate review comments from Roger and tweak some code > - Merge branch 'master' into 8281658-showsettings-security > - ... and 5 more: https://git.openjdk.org/jdk/compare/da750e20...7e6f5090 Marked as reviewed by rriggs (Reviewer). src/java.base/share/classes/sun/launcher/LauncherHelper.java line 115: > 113: private static StringBuilder outBuf = new StringBuilder(); > 114: > 115: private static final String INDENT = " ".repeat(4); Revert this, its just making a simple constant into a runtime expression. - PR Review: https://git.openjdk.org/jdk/pull/14394#pullrequestreview-1517119059 PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1254799500
Re: RFR: 8311170: Simplify and modernize equals and hashCode in security area [v4]
On Wed, 5 Jul 2023 20:52:56 GMT, Pavel Rappo wrote: >> Are you sure? I just checked lines 91-92 and I'd say the change looks >> correct. > >> The original `<=` was correct, the number of bits in the input array must be >> less than the requested length of the BitArray. The constructors also >> describe the length using `<=`; they all should be consistent. > > Hm... My reading is that those "i.e." parts state preconditions for the > constructors to return successfully, not preconditions for them to throw an > exception. I stand corrected. - PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253655028
Re: RFR: 8311170: Simplify and modernize equals and hashCode in security area [v4]
On Wed, 5 Jul 2023 14:52:22 GMT, Pavel Rappo wrote:
>> Please review this PR to use modern APIs and language features to simplify
>> `equals` and `hashCode` in security area.
>>
>> I understand that security area is sensitive and a non-expert, such as
>> myself, should tread carefully; so below are my notes to assist the review.
>>
>> * Unlike `hashCode`, non-secure `equals` implementations are typically
>> short-circuit. But because of "timing attacks", we seem to have specialized
>> implementations, such as `java.security.MessageDigest.isEqual(byte[],
>> byte[])` and a more general `sun.security.util.ByteArrays.isEqual(byte[],
>> int, int, byte[], int, int)`. So while reviewing this PR, take an
>> opportunity to audit the affected `equals` implementations: perhaps some of
>> them need to become secure, not modern. I have no domain knowledge to tell
>> those cases apart, I only note that those cases exist.
>>
>> * This PR sacrifices compatibility for pragmatism: it changes some
>> `hashCode` implementations to produce different values than before to allow
>> more utilization of methods from `Objects` and `Arrays`. To my mind, those
>> changes are **benign**. If you disagree, I'd be happy to discuss that and/or
>> retract the concerning part of the change.
>>
>> * BitArray could be a topic of its own, but I'll do my best to be concise.
>>
>> * Truth to be told, BitArray's `equals` and `hashCode` are not used
>> anywhere in source, and `equals` is only used in one test. For that reason,
>> I refrained from reimplementing internals of `BitArray` using more general
>> `java.util.BitSet`: too much effort and risk for almost nothing.
>> * Speaking of `BitSet`-powered `BitArray`. Such an implementation is not
>> for the faint of heart: there's too much impedance mismatch between data
>> structures that those classes use to store bits. That said, for the sake of
>> testing that it is possible and that I understand the `BitArray` correctly,
>> I actually implemented it using `BitSet`. While that implementation is
>> **NOT** part of this PR, you can have a look at it
>> [here](https://cr.openjdk.org/~prappo/8311170/BitArray.java).
>>
>> * One suggestion to consider is to change this somewhat arcane piece in
>> java.security.UnresolvedPermission.equals:
>>
>> // check certs
>> if (this.certs == null && that.certs != null ||
>> this.certs != null && that.certs == null ||
>> this.certs != null &&
>> this.certs.length != that.certs.length) {
>> return false;
>> }
>>
>> int i,j;
>> boolea...
>
> Pavel Rappo 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 branch 'master' into 8311170
> - Be consistent with the rest of the change
> - Fix reported bugs
> - Add even more cases and tidy up
> - More cases
> - Initial commit
src/java.base/share/classes/java/security/cert/X509CRLEntry.java line 110:
> 108: public int hashCode() {
> 109: try {
> 110: return Arrays.hashCode(this.getEncoded());
What data is at `entryData[0]` that will now be included in the hashCode when
it was skipped before?
src/java.base/share/classes/javax/security/cert/Certificate.java line 107:
> 105: try {
> 106: return Arrays.hashCode(this.getEncoded());
> 107: } catch (CertificateException e) {
Ditto, what value at `certData[0]` is now included in the hash?
src/java.base/share/classes/sun/security/util/BitArray.java line 72:
> 70: * specified byte array. The most significant bit of {@code a[0]} gets
> 71: * index zero in the BitArray. The array must be large enough to
> specify
> 72: * a value for every bit of the BitArray, i.e. {@code 8*a.length >=
> length}.
The original `<=` was correct, the number of bits in the input array must be
less than the requested length of the BitArray. The constructors also describe
the length using `<=`; they all should be consistent.
src/java.base/share/classes/sun/security/x509/X500Name.java line 422:
> 420: // quick check that number of RDNs and AVAs match before
> canonicalizing
> 421: if (!Arrays.equals(this.names, other.names,
> 422: Comparator.comparingInt(n -> n.assertion.length)))
I'd keep the original comparison of the lengths; its a lot less magical than
`Comparator.comparingInt(n -> n.assertion.length))`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253446806
PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253456172
PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253462853
PR Review Comment:
Re: RFR: 8310901: Convert String::newStringNoRepl with Latin-1 to String::newStringLatin1NoRepl [v4]
On Tue, 27 Jun 2023 07:45:34 GMT, Glavo wrote: >> Added a new method `newStringLatin1NoRepl` to the `JavaLangAccess`. >> >> Reasons: >> >> * Most use cases of `newStringNoRepl` use `ISO_8859_1` as the charset, >> creating a new shortcut can make writing shorter; >> * Since all possible values of `byte` are legal Latin-1 characters, >> `newStringLatin1NoRepl` **will not throw `CharacterCodingException`**, so >> users can make the compiler happy without using useless try-catch statements. > > Glavo 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 branch 'openjdk:master' into latin1-no-repl > - Merge branch 'openjdk:master' into latin1-no-repl > - update javadoc > - clean newStringNoRepl1 > - clean newStringNoRepl1 > - Rename jla to JLA > - Create new method JavaLangAccess::newStringLatin1NoRepl The goal of removing the try/catch from HexFormat can be solved locally by creating a private method in HexFormat that swallows the exception. There is no need to add a method to Java Lang Access or change 3 other files. The method `newStringUTF8NoRepl` is different, it can/does throw runtime exceptions but there is no need for try/catch in that case, no change is needed there. - PR Comment: https://git.openjdk.org/jdk/pull/14655#issuecomment-1610209289
Re: RFR: 8310901: Convert String::newStringNoRepl with Latin-1 to String::newStringLatin1NoRepl [v4]
On Tue, 27 Jun 2023 07:45:34 GMT, Glavo wrote: >> Added a new method `newStringLatin1NoRepl` to the `JavaLangAccess`. >> >> Reasons: >> >> * Most use cases of `newStringNoRepl` use `ISO_8859_1` as the charset, >> creating a new shortcut can make writing shorter; >> * Since all possible values of `byte` are legal Latin-1 characters, >> `newStringLatin1NoRepl` **will not throw `CharacterCodingException`**, so >> users can make the compiler happy without using useless try-catch statements. > > Glavo 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 branch 'openjdk:master' into latin1-no-repl > - Merge branch 'openjdk:master' into latin1-no-repl > - update javadoc > - clean newStringNoRepl1 > - clean newStringNoRepl1 > - Rename jla to JLA > - Create new method JavaLangAccess::newStringLatin1NoRepl I don't think this is a productive change. It takes a local inconvenience try/catch and spreads the impact over multiple files and packages as well as bulking up JLA. - PR Comment: https://git.openjdk.org/jdk/pull/14655#issuecomment-1609977623
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v5]
On Mon, 19 Jun 2023 16:11:12 GMT, Sean Coffey wrote:
>> src/java.base/share/classes/sun/launcher/SecuritySettings.java line 66:
>>
>>> 64: ostream.println("Unrecognized security subcommand. See
>>> \"java -X\" for help");
>>> 65: ostream.println("Printing all security settings");
>>> 66: printAllSecurityConfig();
>>
>> The error message is going to get lost in the volume of settings.
>> Allowing bad command input reinforces learning the wrong suboption; though
>> it may duplicate the help, I'd print the allowed options.
>
> @RogerRiggs - do you mean to print nothing in the "bad command input"
> scenario ? The current -XshowSettings launch behaviour prints all data if a
> bad value is passed to it. I was mimicking this for security subcommands.
>
> Are you suggesting to print the relevant part of the "java -X" help menu here
> or is restoring the message with value subcommands sufficient ?
Restoring the message with value subcommands is what I would prefer.
I do disagree with the original choice of printing everything if the command is
wrong and would not propagate that behavior to the new command.
Separately, I would consider changing the existing behavior but I suspect that
would fail to achieve consensus based on compatibility concerns.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1234344945
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v5]
On Fri, 16 Jun 2023 12:14:49 GMT, Sean Coffey wrote:
>> New functionality in the -XshowSettings menu to display relevant information
>> about JDK security configuration
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Pass PrintStream to security helper
src/java.base/share/classes/sun/launcher/SecuritySettings.java line 66:
> 64: ostream.println("Unrecognized security subcommand. See
> \"java -X\" for help");
> 65: ostream.println("Printing all security settings");
> 66: printAllSecurityConfig();
The error message is going to get lost in the volume of settings.
Allowing bad command input reinforces learning the wrong suboption; though it
may duplicate the help, I'd print the allowed options.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1232388914
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v2]
On Tue, 13 Jun 2023 13:57:13 GMT, Sean Coffey wrote:
>> New functionality in the -XshowSettings menu to display relevant information
>> about JDK security configuration
>
> 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 six additional
> commits since the last revision:
>
> - Merge branch 'master' into 8281658-showsettings-security
> - Incorporate feedback to date
> - minor edits, copyright, descriptions etc
> - Merge branch 'master' into 8281658-showsettings-security
> - Merge branch 'master' into 8281658-showsettings-security
> - 8281658
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 189:
> 187: } else {
> 188: printSecuritySettings("all");
> 189: }
Perhaps a slight preference for:
var opt = opts.length > 2 ? opts[2].trim() : "all";
printSecuritySettings(opt);
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 342:
> 340:
> 341: private static void printSecuritySettings(String arg) {
> 342: switch (arg.toLowerCase(Locale.ROOT)) {
Drop the conversion to lower case: none of the other categories allow
"sloppy-case" inputs.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 422:
> 420: ostream.println(TWOINDENT + "Provider name: " + p.getName());
> 421: if (verbose) {
> 422: ostream.println(TWOINDENT + PROV_INFO_STRING +
> wrappedString(p.getInfo(), 80));
Wrapping the args at 80 might still produce line that is quite long.
Perhaps wrap the string after it was concatenated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1228227964
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1228187867
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1228270167
Re: RFR: 8281658: Add a security category to the java -XshowSettings option
On Fri, 9 Jun 2023 13:54:14 GMT, Sean Coffey wrote:
> New functionality in the -XshowSettings menu to display relevant information
> about JDK security configuration
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 65:
> 63: import java.text.MessageFormat;
> 64: import java.text.Normalizer;
> 65: import java.util.*;
Wild card imports are discouraged. (Your IDE may have done this behind your
back)
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 328:
> 326:
> 327: private static void printSecuritySettings(String arg) {
> 328: if (arg.toLowerCase(Locale.ROOT).equals("properties")) {
How about a string switch with "all" and default: being the catchall. It would
be easier to add others and avoid multiple calls to `toLowerCase`.
It might be a good idea for unrecognized settings to print a 1-liner reminder
of the valid values, including `all`
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 354:
> 352: }
> 353: }
> 354: ostream.print("\n");
or just `ostream.println();`
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 363:
> 361:
> SSLContext.getDefault().getSocketFactory().createSocket();
> 362: } catch (IOException | NoSuchAlgorithmException e) {
> 363: throw new RuntimeException(e);
That's pretty rude of the runtime; perhaps `throw InternalError("Unable to
create secure socket")`.
It would give some clue that doesn't look like a coding bug.
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 376:
> 374: System.out.println(THREEINDENT + s);
> 375: }
> 376: ostream.print("\n");
`ostream.println();`
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 393:
> 391: }
> 392:
> 393: // return a string split across multiple lines which aims to limit
> max width
This utility method might be usable in more places if the indentation was an
argument instead of hard coded.
For example, in `printSecurityProperties`. It might need to be able to split
in more places.
src/java.base/share/classes/sun/launcher/resources/launcher.properties line 171:
> 169: \-XshowSettings:vm\n\
> 170: \ show all vm related settings and continue\n\
> 171: \-XshowSettings:security\n\
`all` should probably be documented.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1224440655
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r122089
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1224451282
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1224456125
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1224457654
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1224460749
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1224467126
Re: RFR: 8308016: Use snippets in java.io package [v8]
On Thu, 18 May 2023 19:14:02 GMT, Brian Burkhalter wrote:
>> Replace `{@code ...}` patterns and the like with `{@snippet
>> lang=java : ...}`.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8308016: Address reviewer comments since previous commit
Thanks for the updates.
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13957#pullrequestreview-1437322626
Re: RFR: 8308016: Use snippets in java.io package [v7]
On Wed, 17 May 2023 20:51:29 GMT, Brian Burkhalter wrote:
>> Replace `{@code ...}` patterns and the like with `{@snippet
>> lang=java : ...}`.
>
> Brian Burkhalter 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
> - 8308016: Reinstate @snippet for RandomAccessFile::readLong
> - 8308016: Reduce linking in File::toPath snippet
> - 8308016: Fix link in snippet of File::toPath
> - 8308016: Address reviewer comments since previous commit
> - 8308016: Remove ellipses ("...") from snippets
> - 8308016: Use snippets in java.io package
src/java.base/share/classes/java/io/ByteArrayOutputStream.java line 284:
> 282: * array such that:
> 283: * {@snippet lang=java :
> 284: * c == (char)(((hibyte & 0xff) << 8) | (b & 0xff))
I'd skip the extra indentation for just the single line example.
src/java.base/share/classes/java/io/CharArrayWriter.java line 189:
> 187: *
> 188: * {@snippet lang=java :
> 189: * out.write(csq.subSequence(start, end).toString())
a trailing ";' would be useful for copy/paste; butr that's not the existing
style. Your call.
src/java.base/share/classes/java/io/Console.java line 125:
> 123: * Scanner sc = new Scanner(con.reader());
> 124: * code: // @replace substring="code:" replacement="..."
> 125: * }
4 space indentation would be enough.
src/java.base/share/classes/java/io/RandomAccessFile.java line 762:
> 760: * then the result is:
> 761: * {@snippet lang=java :
> 762: * (byte)(b)
4 space indent is sufficient in this file
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13957#discussion_r1198146819
PR Review Comment: https://git.openjdk.org/jdk/pull/13957#discussion_r1198147802
PR Review Comment: https://git.openjdk.org/jdk/pull/13957#discussion_r1198151689
PR Review Comment: https://git.openjdk.org/jdk/pull/13957#discussion_r1198160636
Re: RFR: 8308016: Use snippets in java.io package [v2]
On Sun, 14 May 2023 05:50:20 GMT, Tagir F. Valeev wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8308016: Remove ellipses ("...") from snippets
>
> src/java.base/share/classes/java/io/RandomAccessFile.java line 904:
>
>> 902: * {@code b7}, and {@code b8,} where:
>> 903: * {@snippet lang=java :
>> 904: * 0 <= b1, b2, b3, b4, b5, b6, b7, b8 <= 255,
>
> Same: this is not Java language syntax. Code or pre tags are fine here, they
> are not deprecated.
I would keep the snippet markup but change the language to "text"; removing the
expectation of language support.
The benefits of a consistent look are still desirable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13957#discussion_r1196733214
Integrated: 8304911: Use OperatingSystem enum in some modules
On Tue, 4 Apr 2023 19:22:48 GMT, Roger Riggs wrote: > With the addition of `jdk.internal.util.OperatingSystem` references to the > system property `os.name` can be replaced. > This PR exports jdk.internal.util to: > - java.prefs, > - java.security.jgss, > - java.smartcardio, > - jdk.charsets, > - jdk.net, > - jdk.zipfs This pull request has now been integrated. Changeset: ba90dc77 Author:Roger Riggs URL: https://git.openjdk.org/jdk/commit/ba90dc77958c399e4e1fc3c4999dd76680480c7b Stats: 120 lines in 12 files changed: 20 ins; 50 del; 50 mod 8304911: Use OperatingSystem enum in some modules Reviewed-by: naoto, lancea, iris, jpai - PR: https://git.openjdk.org/jdk/pull/13335
Re: RFR: 8304911: Use OperatingSystem enum in some modules [v4]
> With the addition of `jdk.internal.util.OperatingSystem` references to the > system property `os.name` can be replaced. > This PR exports jdk.internal.util to: > - java.prefs, > - java.security.jgss, > - java.smartcardio, > - jdk.charsets, > - jdk.net, > - jdk.zipfs Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Remove errant space - Changes: - all: https://git.openjdk.org/jdk/pull/13335/files - new: https://git.openjdk.org/jdk/pull/13335/files/5546b824..b9267942 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13335=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13335=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13335.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13335/head:pull/13335 PR: https://git.openjdk.org/jdk/pull/13335
Re: RFR: 8304911: Use OperatingSystem enum in some modules [v3]
> With the addition of `jdk.internal.util.OperatingSystem` references to the > system property `os.name` can be replaced. > This PR exports jdk.internal.util to: > - java.prefs, > - java.security.jgss, > - java.smartcardio, > - jdk.charsets, > - jdk.net, > - jdk.zipfs Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Backout unneeded changes to module-info and default.policy. Fixup formatting. - Changes: - all: https://git.openjdk.org/jdk/pull/13335/files - new: https://git.openjdk.org/jdk/pull/13335/files/aadf5bbd..5546b824 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13335=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13335=01-02 Stats: 5 lines in 3 files changed: 1 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13335.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13335/head:pull/13335 PR: https://git.openjdk.org/jdk/pull/13335
Re: RFR: 8304911: Use OperatingSystem enum in some modules [v2]
> With the addition of `jdk.internal.util.OperatingSystem` references to the > system property `os.name` can be replaced. > This PR exports jdk.internal.util to: > - java.prefs, > - java.security.jgss, > - java.smartcardio, > - jdk.charsets, > - jdk.net, > - jdk.zipfs Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Revert chanes to ZipFileSystem to defer refactoring. Reformatted expression to match switch expression formatting. - Changes: - all: https://git.openjdk.org/jdk/pull/13335/files - new: https://git.openjdk.org/jdk/pull/13335/files/72cff1ed..aadf5bbd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13335=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13335=00-01 Stats: 11 lines in 2 files changed: 3 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/13335.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13335/head:pull/13335 PR: https://git.openjdk.org/jdk/pull/13335
Re: RFR: 8304911: Use OperatingSystem enum in some modules
On Wed, 5 Apr 2023 08:37:27 GMT, Alan Bateman wrote:
>> With the addition of `jdk.internal.util.OperatingSystem` references to the
>> system property `os.name` can be replaced.
>> This PR exports jdk.internal.util to:
>> - java.prefs,
>> - java.security.jgss,
>> - java.smartcardio,
>> - jdk.charsets,
>> - jdk.net,
>> - jdk.zipfs
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 2882:
>
>> 2880: elenNTFS = 36; // total 36 bytes
>> 2881: } else { // Extended Timestamp
>> otherwise
>> 2882: elenEXTT = 9;// only mtime in cen
>
> It might be better to drop ZipFileSystem from this patch and instead create a
> bug to re-examine this code. I would have expected it use NFTS when the
> timestamp is beyond the range of an extended timestamp. It shouldn't be
> looking at the current OS name.
Created [JDK-8305662](https://bugs.openjdk.org/browse/JDK-8305662) to track
refactoring.
Will revert.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13335#discussion_r1158587949
Re: RFR: 8304911: Use OperatingSystem enum in some modules
On Wed, 5 Apr 2023 08:39:35 GMT, Alan Bateman wrote:
>> With the addition of `jdk.internal.util.OperatingSystem` references to the
>> system property `os.name` can be replaced.
>> This PR exports jdk.internal.util to:
>> - java.prefs,
>> - java.security.jgss,
>> - java.smartcardio,
>> - jdk.charsets,
>> - jdk.net,
>> - jdk.zipfs
>
> src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java line 406:
>
>> 404: case MACOS ->
>> newInstance("jdk.net.MacOSXSocketOptions");
>> 405: case WINDOWS ->
>> newInstance("jdk.net.WindowsSocketOptions");
>> 406: default -> new PlatformSocketOptions();
>
> For another issue, but I assume this could be refactored to not need
> PlatformSocketOptions, in which case the mapping of OS name to implementation
> class would go away.
Created an issue to track the possible refactoring, will keep this change until
then.
https://bugs.openjdk.org/browse/JDK-8305661
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13335#discussion_r1158574021
RFR: 8304911: Use OperatingSystem enum in some modules
With the addition of `jdk.internal.util.OperatingSystem` references to the system property `os.name` can be replaced. This PR exports jdk.internal.util to: - java.prefs, - java.security.jgss, - java.smartcardio, - jdk.charsets, - jdk.net, - jdk.zipfs - Commit messages: - In jdk.prefs module, Replace os.name with OperatingSystem enum - Use OperatingSystem methods instead of system property os.name Changes: https://git.openjdk.org/jdk/pull/13335/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13335=00 Issue: https://bugs.openjdk.org/browse/JDK-8304911 Stats: 131 lines in 13 files changed: 24 ins; 54 del; 53 mod Patch: https://git.openjdk.org/jdk/pull/13335.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13335/head:pull/13335 PR: https://git.openjdk.org/jdk/pull/13335
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
On Mon, 6 Mar 2023 20:22:48 GMT, Pavel Rappo wrote: >> Please review this superficial documentation cleanup that was triggered by >> unrelated analysis of doc comments in JDK API. >> >> The only effect that this multi-area PR has on the JDK API Documentation >> (i.e. the observable effect on the generated HTML pages) can be summarized >> as follows: >> >> >> diff -ur >> build/macosx-aarch64/images/docs-before/api/serialized-form.html >> build/macosx-aarch64/images/docs-after/api/serialized-form.html >> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html >> 2023-03-02 11:47:44 >> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html >> 2023-03-02 11:48:45 >> @@ -17084,7 +17084,7 @@ >> throws > href="java.base/java/io/IOException.html" title="class in >> java.io">IOException, >> ClassNotFoundException >> readObject is called to restore the >> state of the >> - (@code BasicPermission} from a stream. >> + BasicPermission from a stream. >> >> Parameters: >> s - the ObjectInputStream from which data >> is read >> >> Notes >> - >> >> * I'm not an expert in any of the affected areas, except for jdk.javadoc, >> and I was merely after misused tags. Because of that, I would appreciate >> reviews from experts in other areas. >> * I discovered many more issues than I included in this PR. The excluded >> issues seem to occur in infrequently updated third-party code (e.g. >> javax.xml), which I assume we shouldn't touch unless necessary. >> * I will update copyright years after (and if) the fix had been approved, as >> required. > > Pavel Rappo 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 two additional > commits since the last revision: > > - Merge branch 'master' into 8303480 > - Initial commit Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8298873: Update IllegalRecordVersion.java for changes to TLS implementation [v4]
On Tue, 17 Jan 2023 17:12:05 GMT, Matthew Donovan wrote: >> - Updated ProtocolVersion.isNegotiable() to check a bounded range of version >> numbers. >> - Removed IllegalRecordVersion.java from ProblemList.txt >> >> Tested with jdk_security and jdk_security3 test groups. > > Matthew Donovan has updated the pull request incrementally with one > additional commit since the last revision: > > removed extra whitespace The copyright line has the incorrect syntax. Please followup with a fix. Missing comma, after 2023. - PR: https://git.openjdk.org/jdk/pull/11929
Re: RFR: 8300647: Miscellaneous hashCode improvements in java.base [v2]
On Thu, 19 Jan 2023 13:46:26 GMT, Claes Redestad wrote: >> Went through the jdk and found a few more places where >> `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc >> methods could be replaced with a plain call to `java.util.Arrays` >> equivalents. This patch addresses that. >> >> After this, #12068, and #12077 I think we're reaching the limit of sensible >> direct use of `AS::vectorizedHashCode`. I've found a few places outside of >> java.base where `vectorizedHashCode` might be applicable, but none that seem >> important enough to warrant an export of this method outside of java.base. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert ZipFileSystem changes since it's additionally built as a library > dependency and can't rely on jdk.internal Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12093
Re: RFR: JDK-8300133: Use generalized see and link tags in core libs [v2]
On Mon, 16 Jan 2023 15:06:25 GMT, Daniel Fuchs wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typo found in code review.
>
> src/java.base/share/classes/java/lang/CharSequence.java line 76:
>
>> 74: *
>> 75: * If the {@code char} value specified by the index is a
>> 76: * {@linkplain Character##unicode surrogate}, the surrogate value
>
> I didn't know about the `##` trick. Is that a new feature to target an HTML
> anchor?
The CSR has a concise description:
[JDK-8294195](https://bugs.openjdk.org/browse/JDK-8294195) Generalize see and
link tags for user-defined anchors
-
PR: https://git.openjdk.org/jdk/pull/12000
Re: RFR: JDK-8298170 : Introduce a macro for exception check, free and return
On Fri, 9 Dec 2022 12:23:04 GMT, Matthias Baesken wrote: > Hi Roger , the new proposed version JNU_CHECK_EXCEPTION_DO is now almost as > lengthy as the original coding, Is it really worth it introducing a macro > when it gets so lengthy ? Its easier to understand the flow and cleanup being done if its visible in the source. I (or new readers) don't have to go unwrap the macro to know what's going to happen. - PR: https://git.openjdk.org/jdk/pull/11539
Re: RFR: JDK-8298170 : Introduce a macro for exception check, free and return
On Tue, 6 Dec 2022 15:20:26 GMT, Matthias Baesken wrote:
> We have a number of places in the codebase where a macro could help when we
> check an exception and afterwrads free something and return.
Good idea, though perhaps the return (and value if any) could be explicit in
the macro invocation, instead of implicit (plus arg). A single macro would
suffice, instead of multiples.
Usage Example:
JNU_CHECK_EXCEPTION_DO(env, {
free(tab);
return;
})
Or put the invocation and return all on one line.
-
PR: https://git.openjdk.org/jdk/pull/11539
Re: RFR: JDK-8298170 : Introduce a macro for exception check, free and return
On Tue, 6 Dec 2022 15:20:26 GMT, Matthias Baesken wrote: > We have a number of places in the codebase where a macro could help when we > check an exception and afterwrads free something and return. The existing (and new) macro naming doesn't make clear that it always returns from the function. The presence of "RETURN" in the name only means there is a return value. The existing code more obviously handles memory deallocation. - PR: https://git.openjdk.org/jdk/pull/11539
Re: RFR: 8297519: Improve expressions and modernise code [v3]
On Wed, 30 Nov 2022 08:03:50 GMT, Per Minborg wrote: >> During the work of another PR (https://github.com/openjdk/jdk/pull/11260), >> several improvement areas were identified. These are now adressed in this >> separate PR proposing the use of more modern Java constructs as well as >> simplifying a large number of logical expressions that were previously >> non-normative. >> >> >> This branch has been tested and passed tier1-4 tests. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Remove switch and use pattern matching A suggestion on the bug title, include "PKCS" or something to identity what code is being modernized. - PR: https://git.openjdk.org/jdk/pull/11348
Re: RFR: 8297778: Modernize and improve module jdk.sctp
On Tue, 29 Nov 2022 16:46:43 GMT, Per Minborg wrote:
> This PR proposes a variety of modernisations to the `jdk.sctp` module.
>
> During the fix of https://bugs.openjdk.org/browse/JDK-8296024, several
> improvement areas were identified including:
>
> * Replacing duplicate code segments
> * Making certain fields final
> * Using enhanced switch
> * Using records
> * Fixing typos
> * Marking fields participating in serialisation with `@Serial`
> * Modernizing toString() implementations
> * Using pattern matching
> * Using diamond operators
src/jdk.sctp/aix/classes/sun/nio/ch/sctp/SctpChannelImpl.java line 39:
> 37: import com.sun.nio.sctp.SctpSocketOption;
> 38:
> 39: import static sun.nio.ch.sctp.Util.sctpNotSupported;
Not really a fan of static imports like this; it looks like a local method in
the code but is not.
In this case, the code would be more readable as `Util.sctpNotSupported()`.
This style of creating exceptions to throw is unusual in OpenJDK.
src/jdk.sctp/aix/classes/sun/nio/ch/sctp/Util.java line 1:
> 1: package sun.nio.ch.sctp;
Can this be shared in **share**/classes/sun/nio/sctp/Util.java?
src/jdk.sctp/macosx/classes/sun/nio/ch/sctp/SctpChannelImpl.java line 44:
> 42: * Unimplemented.
> 43: */
> 44: public class SctpChannelImpl extends SctpChannel {
Going a bit beyond just updating syntax; but that's a different PR...
There could be more sharing of the unsupported implementation if SctpChannel
was not abstract and its method bodies threw the exception. There would less
duplication for unsupported platforms.
src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 722:
> 720:
> 721: /**
> 722: * @throws IllegalArgumentException If the given association is not
> controlled by this channel
There should be a 1st sentence describing the function; its odd to see only an
@throws.
-
PR: https://git.openjdk.org/jdk/pull/11418
Re: RFR: 8297271: AccessFlags should be specific to class file version
On Mon, 28 Nov 2022 22:56:27 GMT, Roger Riggs wrote: > The accessFlags() methods added (in JDK 20, the current release) to > java.lang.Class, java.lang.reflect.Executable, and java.lang.reflect.Field > assume the access flags are from the current/most recent class file format > version. For current and past class file format versions there are few > significant variations but future changes are anticipated that change the > meaning of some access flag mask bits. > > The accessFlags() methods are clarified to return the access flags that are > applicable to the class file format version of the class. The existing > AccessFlag.Locations API already contains information about the locations > that are applicable to a class file format version. That information should > be used to construct the set of AccessFlags returned. A method is added to > AccessFlag that returns the applicable flags for a particular mask, Location, > and class file format version: Yes, the location information via `AccessFlags.locations(ClassFileFormatVersion)` provides the needed information. And the tests verify that information. My objective was to make the AccessFlags returned for a class reflect the class file format version number of the underlying class. Currently, it is implicitly the current class file format version supported by the VM, not necessarily the version of the class loaded. Suggestions welcome for how that is exposed in the APIs and implemented. - PR: https://git.openjdk.org/jdk/pull/11399
RFR: 8297271: AccessFlags should be specific to class file version
The accessFlags() methods added (in JDK 20, the current release) to java.lang.Class, java.lang.reflect.Executable, and java.lang.reflect.Field assume the access flags are from the current/most recent class file format version. For current and past class file format versions there are few significant variations but future changes are anticipated that change the meaning of some access flag mask bits. The accessFlags() methods are clarified to return the access flags that are applicable to the class file format version of the class. The existing AccessFlag.Locations API already contains information about the locations that are applicable to a class file format version. That information should be used to construct the set of AccessFlags returned. A method is added to AccessFlag that returns the applicable flags for a particular mask, Location, and class file format version: - Commit messages: - 8297271: AccessFlags should be specific to class file version Changes: https://git.openjdk.org/jdk/pull/11399/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11399=00 Issue: https://bugs.openjdk.org/browse/JDK-8297271 Stats: 206 lines in 9 files changed: 198 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/11399.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11399/head:pull/11399 PR: https://git.openjdk.org/jdk/pull/11399
Re: RFR: 8297515: serialVersionUID fields are not annotated with @Serial
On Thu, 24 Nov 2022 08:19:17 GMT, Per Minborg wrote: > This PR proposes adding `@Serial` annotations to certain fields participating > in serialisation. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11347
Re: RFR: 8297276: Remove thread text from Subject.current
On Tue, 22 Nov 2022 16:26:30 GMT, Weijun Wang wrote: > With the introduction of Virtual Threads, the current subject is no longer > guaranteed to be inherited in a new thread. Remove this requirement until we > find another way to implement `Subject::current`. LGTM, dropping the inaccurate text. A release note is a good idea. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11292
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v5]
On Fri, 14 Oct 2022 17:27:34 GMT, Aleksei Efimov wrote:
>> src/java.base/share/conf/security/java.security line 1388:
>>
>>> 1386: # are unused.
>>> 1387: #
>>> 1388: # Each class name pattern is matched against the factory class name
>>> to allow or disallow its
>>
>> It appears that for those protocols for which there is no specific filter, a
>> factory class will be accepted only if the global filter returns ALLOWED -
>> which contradicts what is written here (where it says that the class is
>> allowed if it's not REJECTED). Is this something that has changed with this
>> fix - or was the documentation wrong before?
>
> Very good catch Daniel! It is with this fix and I believe the code needs to
> be change to match what is written for the global filter here:
> What we've had before in `checkInput`:
>
> private static boolean checkInput(FactoryInfo factoryInfo) {
> Status result = GLOBAL.checkInput(factoryInfo);
> return result != Status.REJECTED;
>
> What we have now:
>
> if (filter == GLOBAL_FILTER) {
> return globalResult == Status.ALLOWED;
> }
>
>
> I think it needs to be changed to (to match the description for global
> filter):
>
> if (filter == GLOBAL_FILTER) {
> return globalResult != Status.REJECTED;
> }
In the general composition of filters, it is preferable that UNDECIDED is
treated as REJECTED.
That keeps unintentional holes in a filter from being permissive.
-
PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v3]
On Thu, 13 Oct 2022 12:34:47 GMT, Jaikiran Pai wrote: >> Aleksei Efimov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Change checkInput to be the global filter centric > > src/java.base/share/conf/security/java.security line 1408: > >> 1406: # references bound to LDAP contexts. The factory class named by the >> reference instance will >> 1407: # be matched against this filter. The filter property supports >> pattern-based filter syntax >> 1408: # with the same format as jdk.serialFilter. > > Hello Aleksei, as far as I can see the `jdk.serialFilter` allows for > additional details like `limits` (example: `maxdepth=value`). Should we note > here that this JNDI specific property will ignore those configurations? Yes, worth noting, but I would say they are "unused", not ignored. - PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v3]
On Mon, 10 Oct 2022 14:28:07 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> This change introduces new system and security properties for specifying >> factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider >> implementations. >> >> These new properties allow more granular control over the set of object >> factories allowed to reconstruct Java objects from LDAP and RMI contexts. >> >> The new factory filters are supplementing the existing >> `jdk.jndi.object.factoriesFilter` global factories filter to determine if a >> specific object factory is permitted to instantiate objects for the given >> protocol. >> >> Links: >> >> - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) >> - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) >> >> ### List of code changes >> >> - Implementation for two new system and security properties have been added >> to the `com.sun.naming.internal.ObjectFactoriesFilter` class >> - `java.security` and `module-info.java` files have been updated with a >> documentation for the new properties >> - To keep API of `javax.naming.spi.NamingManager` and >> `javax.naming.spi.DirectoryManager` classes unmodified a new internal >> `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All >> `getObjectInstance` calls have been updated to use the new helper class. >> >> NamingManagerHelper changes >> Changes performed to construct the `NamingManagerHelper` class: >> - `DirectoryManager.getObjectInstance` -> >> `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also >> moved to the `NamingManagerHelper` class >> - `NamingManager.getObjectInstance` -> >> `NamingManagerHelper.getObjectInstance`. Methods responsible for >> setting/getting object factory builder were moved to the >> `NamingManagerHelper` class too. >> >> >> ### Test changes >> New tests have been added for checking that new factory filters can be used >> to restrict reconstruction of Java objects from LDAP and RMI contexts: >> - LDAP protocol specific test: >> test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java >> - RMI protocol specific test: >> test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java >> Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated >> to allow test-specific factories filter used to reconstruct objects from the >> test LDAP server. >> >> ### Testing >> tier1-tier3 and JNDI regression/JCK tests not showing any failures related >> to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Change checkInput to be the global filter centric LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation
On Wed, 5 Oct 2022 15:23:43 GMT, Aleksei Efimov wrote:
> ### Summary of the change
> This change introduces new system and security properties for specifying
> factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider
> implementations.
>
> These new properties allow more granular control over the set of object
> factories allowed to reconstruct Java objects from LDAP and RMI contexts.
>
> The new factory filters are supplementing the existing
> `jdk.jndi.object.factoriesFilter` global factories filter to determine if a
> specific object factory is permitted to instantiate objects for the given
> protocol.
>
> Links:
>
> - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556)
> - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368)
>
> ### List of code changes
>
> - Implementation for two new system and security properties have been added
> to the `com.sun.naming.internal.ObjectFactoriesFilter` class
> - `java.security` and `module-info.java` files have been updated with a
> documentation for the new properties
> - To keep API of `javax.naming.spi.NamingManager` and
> `javax.naming.spi.DirectoryManager` classes unmodified a new internal
> `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All
> `getObjectInstance` calls have been updated to use the new helper class.
>
> NamingManagerHelper changes
> Changes performed to construct the `NamingManagerHelper` class:
> - `DirectoryManager.getObjectInstance` ->
> `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also moved
> to the `NamingManagerHelper` class
> - `NamingManager.getObjectInstance` ->
> `NamingManagerHelper.getObjectInstance`. Methods responsible for
> setting/getting object factory builder were moved to the
> `NamingManagerHelper` class too.
>
>
> ### Test changes
> New tests have been added for checking that new factory filters can be used
> to restrict reconstruction of Java objects from LDAP and RMI contexts:
> - LDAP protocol specific test:
> test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java
> - RMI protocol specific test:
> test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java
> Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated
> to allow test-specific factories filter used to reconstruct objects from the
> test LDAP server.
>
> ### Testing
> tier1-tier3 and JNDI regression/JCK tests not showing any failures related to
> this change.
> No failures observed for the modified regression tests.
Nice work, but a few comments.
src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java
line 91:
> 89: }
> 90:
> 91: private static boolean checkInput(String scheme, FactoryInfo
> factoryInfo) {
This construct in which the supplied lambda fills in the serialClass is pretty
obscure.
Perhaps the variable name can be "serialClass" to match the only non-default
method in ObjectInputFilter would give a better hint.
src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java
line 92:
> 90:
> 91: private static boolean checkInput(String scheme, FactoryInfo
> factoryInfo) {
> 92: Status result = switch(scheme) {
The handling of the selection of the filter could be more direct.
You can change the argument to checkInput to be ObjectInputFilter and pass the
desired filter; LDAP_FILTER, RMI_FITLER, or GLOBAL_FILTER.
And make the check of the GLOBAL_FILTER conditional on it not having already
been evaluated.
Then it will be the case that there must always be a specific filter.
The callers are all local to the class and would change to pass the desired
filter.
src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java
line 173:
> 171:DEFAULT_GLOBAL_SP_VALUE));
> 172:
> 173: // A system-wide LDAP specific object factories filter constructed
> from the system
Where does the IllegalArgumentException from ObjectInputFilter.create get
handled or reported?
If the property value is illformed, the error should be enough to diagnose and
correct the property.
test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java line 46:
> 44: * @build LDAPServer LDAPTestUtils TestFactory
> 45: *
> 46: * @run main/othervm LdapFactoriesFilterTest false
Are any of these filter property cases, malformed and would produce an error
from ObjectInputFilter.create?
-
PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: JDK-8291974 PrivateCredentialPermission should not use local variable to enable debugging [v2]
On Mon, 19 Sep 2022 17:54:57 GMT, Mark Powers wrote: >> https://bugs.openjdk.org/browse/JDK-8291974 > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > added test As mentioned earlier in the comments... This is a completely compatible change. Removing a field is well specified as a compatible change. >From old version to new version the value in the stream is discarded. >From a new version to the old version, the value in the stream is missing so >the field is initialized to its default value (false for boolean). There's no reason to change the serialVersionUID. Changing it would be an incompatible change and cause all kinds of other problems. In the current case, since the value of the field is false, it will be hard to observe any change in behavior. - PR: https://git.openjdk.org/jdk/pull/10206
Re: RFR: 8288568: Reduce runtime of java.security microbenchmarks [v2]
On Fri, 17 Jun 2022 12:24:50 GMT, Claes Redestad wrote: >> - Reduce forks, iteration, runtime to reduce runtime while maintaining high >> data quality on typical benchmarking hosts. >> >> Reduces runtime from estimated 10+ hours to 54 minutes. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Copyrights, apply consistent settings to PermissionsImplies LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/9189
Re: RFR: 8289908: Skip bounds check for cases when String is constructed from entirely used byte[] [v4]
On Wed, 13 Jul 2022 15:11:25 GMT, Сергей Цыпанов wrote: >> We can skip bounds check and null check for Charset in case we use the array >> entirely and the Charset is either default one or proven to be non-null. >> >> Benchmark results: >> >> before >> >> Benchmark Mode Cnt Score >> Error Units >> StringConstructor.newStringFromArray avgt 50 4,815 >> ± 0,154 ns/op >> StringConstructor.newStringFromArrayWithCharsetavgt 50 4,462 >> ± 0,068 ns/op >> StringConstructor.newStringFromArrayWithCharsetNameavgt 50 8,653 >> ± 0,040 ns/op >> StringConstructor.newStringFromRangedArray avgt 50 5,090 >> ± 0,066 ns/op >> StringConstructor.newStringFromRangedArrayWithCharset avgt 50 4,550 >> ± 0,041 ns/op >> StringConstructor.newStringFromRangedArrayWithCharsetName avgt 50 8,080 >> ± 0,055 ns/op >> >> after >> >> Benchmark Mode Cnt Score >> Error Units >> StringConstructor.newStringFromArray avgt 50 4,595 >> ± 0,053 ns/op >> StringConstructor.newStringFromArrayWithCharsetavgt 50 4,038 >> ± 0,062 ns/op >> StringConstructor.newStringFromArrayWithCharsetNameavgt 50 8,035 >> ± 0,031 ns/op >> StringConstructor.newStringFromRangedArray avgt 50 4,084 >> ± 0,007 ns/op >> StringConstructor.newStringFromRangedArrayWithCharset avgt 50 4,014 >> ± 0,008 ns/op >> StringConstructor.newStringFromRangedArrayWithCharsetName avgt 50 7,466 >> ± 0,071 ns/op > > Сергей Цыпанов 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: > > - 8289908: Rework constructor > - Merge branch 'master' into 8289908 > - 8289908: Fixed tests > - Merge branch 'master' into 8289908 > - 8289908: Make constructor private and use trailing Void instead of int > - 8289908: Skip bounds check for cases when String is constructed from > entirely used byte[] Looks good, thanks for the updates. src/java.base/share/classes/java/lang/String.java line 528: > 526: * > 527: * Important: parameter order of this method is deliberately changed > in order to > 528: * disambiguate it against other similar methods ot this class. typo? "ot this". - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/9407
Re: RFR: 8289908: Skip bounds check for cases when String is constructed from entirely used byte[]
On Fri, 8 Jul 2022 07:37:36 GMT, Сергей Цыпанов wrote:
>> src/java.base/share/classes/java/lang/String.java line 1429:
>>
>>> 1427: */
>>> 1428: public String(byte[] bytes, int offset, int length) {
>>> 1429: this(bytes, offset, length, Charset.defaultCharset(),
>>> checkBoundsOffCount(offset, length, bytes.length));
>>
>> Can you avoid the extra constructor by applying `checkBoundOffCount` to the
>> offset argument; it returns the offset.
>>
>> this(bytes, checkBoundsOffCount(offset, length, bytes.length), length,
>> Charset.defaultCharset());
>>
>> or call `Preconditions.checkFromIndexSize` directly.
>
> But if I roll back the added constructor I'll go through existing public one
> `public String(byte[] bytes, int offset, int length, Charset charset)` doing
> bounds check twice, won't I?
The new constructor looks very odd, especially when it does not have an
explanation and doesn't describe the required preconditions for calling it. Is
there a better way than adding a non-functional argument?
The "unused" name is going to draw a warning from IntelliJ and some
enterprising developer is going to try to remove it, not understanding why its
there. And there is a bit of overhead pushing the extra arg.
The constructor should be private, it has a very narrow scope of use given the
lack of checking its args.
It would be nice if the Hotspot compiler would recognize the llmits on the
range and optimize away the checks;
it would have broader applicability then this point fix.
I would be interesting to ask the compiler folks if that's a future
optimization.
These source changes make it harder to understand what's happening where;
though that is sometimes work it for a noticeable performance improvement.
-
PR: https://git.openjdk.org/jdk/pull/9407
Re: RFR: 8289908: Skip bounds check for cases when String is constructed from entirely used byte[]
On Thu, 7 Jul 2022 10:21:06 GMT, Сергей Цыпанов wrote:
> We can skip bounds check and null check for Charset in case we use the array
> entirely and the Charset is either default one or proven to be non-null.
>
> Benchmark results:
>
> before
>
> Benchmark Mode Cnt Score
> Error Units
> StringConstructor.newStringFromArray avgt 50 4,815 ±
> 0,154 ns/op
> StringConstructor.newStringFromArrayWithCharsetavgt 50 4,462 ±
> 0,068 ns/op
> StringConstructor.newStringFromArrayWithCharsetNameavgt 50 8,653 ±
> 0,040 ns/op
> StringConstructor.newStringFromRangedArray avgt 50 5,090 ±
> 0,066 ns/op
> StringConstructor.newStringFromRangedArrayWithCharset avgt 50 4,550 ±
> 0,041 ns/op
> StringConstructor.newStringFromRangedArrayWithCharsetName avgt 50 8,080 ±
> 0,055 ns/op
>
> after
>
> Benchmark Mode Cnt Score
> Error Units
> StringConstructor.newStringFromArray avgt 50 4,595 ±
> 0,053 ns/op
> StringConstructor.newStringFromArrayWithCharsetavgt 50 4,038 ±
> 0,062 ns/op
> StringConstructor.newStringFromArrayWithCharsetNameavgt 50 8,035 ±
> 0,031 ns/op
> StringConstructor.newStringFromRangedArray avgt 50 4,084 ±
> 0,007 ns/op
> StringConstructor.newStringFromRangedArrayWithCharset avgt 50 4,014 ±
> 0,008 ns/op
> StringConstructor.newStringFromRangedArrayWithCharsetName avgt 50 7,466 ±
> 0,071 ns/op
src/java.base/share/classes/java/lang/String.java line 1429:
> 1427: */
> 1428: public String(byte[] bytes, int offset, int length) {
> 1429: this(bytes, offset, length, Charset.defaultCharset(),
> checkBoundsOffCount(offset, length, bytes.length));
Can you avoid the extra constructor by applying `checkBoundOffCount` to the
offset argument; it returns the offset.
this(bytes, checkBoundsOffCount(offset, length, bytes.length), length,
Charset.defaultCharset());
or call `Preconditions.checkFromIndexSize` directly.
-
PR: https://git.openjdk.org/jdk/pull/9407
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v11]
On Fri, 1 Jul 2022 08:31:28 GMT, Xue-Lei Andrew Fan wrote: > Could someone in Oracle help me run Mach 5 testing? The CI Passed for Tiers 1-3. - PR: https://git.openjdk.org/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v10]
On Fri, 1 Jul 2022 08:40:06 GMT, Daniel Fuchs wrote: >> @dfuch Taking a reference as parameter could simplify the use of ForceGC. >> I though about this idea as well, when I had to check lambada expressions in >> each call. I would like to do it in the future so that this PR could focus >> on less things. > > Thanks @XueleiFan, that's fine by me! When using Reference/ReferenceQueue there is no Cleaner, only normal reference processing (via GC). - PR: https://git.openjdk.org/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v10]
On Thu, 30 Jun 2022 18:39:48 GMT, Xue-Lei Andrew Fan wrote:
>> test/lib/jdk/test/lib/util/ForceGC.java line 58:
>>
>>> 56: Reference.reachabilityFence(ref);
>>> 57:
>>> 58: for (int retries = (int)(timeout / 200); retries >= 0;
>>> retries--) {
>>
>> The logic around the timeout might be clearer if it was only based on the
>> number of retries,
>> and can be scaled by the TIMEOUT_FACTOR too.
>
> I just curious if the factor could be set to some unusual values like "1.25".
> Scaling on timeout, and then calculating the retires could be more accuracy
> for such circumstances, although it may be not necessary. I moved the
> retries calculation close to the for-loop. Hope it is better for readers.
Yes, `Utils.adjustTimeout(long x)` uses floating point and then converts back.
-
PR: https://git.openjdk.org/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v11]
On Thu, 30 Jun 2022 18:44:30 GMT, Xue-Lei Andrew Fan wrote: >> This is a follow up update per comments in [JDK-8287384 >> PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in >> open part looks good to me. Please help to run Mach5 just case the closed >> test cases are impacted. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > update per feedback LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v10]
On Thu, 30 Jun 2022 18:24:14 GMT, Xue-Lei Andrew Fan wrote:
>> test/lib/jdk/test/lib/util/ForceGC.java line 70:
>>
>>> 68: // But it is fine. For most cases, the 1st GC is
>>> sufficient
>>> 69: // to trigger and complete the cleanup.
>>> 70: queue.remove(200L);
>>
>> If `remove()` returns a non-null value, then it is safe to break out of the
>> loop.
>> The GC has cleared the ref. And the final `getAsBoolean()` below will return
>> the condition.
>
> I'm not sure if all unused object will be collected in one GC call always.
> The gc() specification says "When control returns from the method call, the
> Java Virtual Machine has made a best effort to reclaim space from all unused
> objects. ... There is also no guarantee that this effort will determine the
> change of reachability in any particular number of objects, or that any
> particular number of {@link java.lang.ref.Reference Reference} objects will
> be cleared and enqueued." But from the spec, I did not get a clear answer
> for the question.
>
> If the `booleanSupplier` object could be cleared in a collection other than
> the `ref` collection, the current code may be safer.
True, knowing when GC is 'done' is not deterministic except for a specify
Reference to a specific object.
System.gc is just a request, the checking for an object can more quickly exit
the loop.
The code is as is, and already commented, might wait an extra cycle, but only
in the case of a race between the requested predicate and the object being
reclaimed. Ok as it.
-
PR: https://git.openjdk.org/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v10]
On Sat, 18 Jun 2022 05:55:32 GMT, Xue-Lei Andrew Fan wrote:
>> This is a follow up update per comments in [JDK-8287384
>> PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in
>> open part looks good to me. Please help to run Mach5 just case the closed
>> test cases are impacted.
>
> Xue-Lei Andrew Fan has updated the pull request with a new target base due to
> a merge or a rebase. The pull request now contains 13 commits:
>
> - Master
> - use Reference.refersTo
> - remove trailing whitespaces
> - use timeout factor
> - Merge
> - Merge master
> - Merge
> - add timeout parameter
> - rollback is not in this branch
> - rollback JDK-8287384
> - ... and 3 more: https://git.openjdk.org/jdk/compare/47b86690...0f196282
Sorry for the delay.
test/lib/jdk/test/lib/util/ForceGC.java line 37:
> 35: // The jtreg testing timeout factor.
> 36: private static final double TIMEOUT_FACTOR = Double.valueOf(
> 37: System.getProperty("test.timeout.factor", "1.0"));
If you don't mind the dependency, you can use jdk.test.lib.Utils.TIMEOUT_FACTOR
or `Utils.adjustTimeout(xx)` in the usage below.
test/lib/jdk/test/lib/util/ForceGC.java line 42:
> 40: * Causes the current thread to wait until the {@code booleanSupplier}
> 41: * returns true, or a specific waiting time elapses. The waiting time
> 42: * is 1 second scalled with the jtreg testing timeout factor.
typo: "scalled" -> "scaled"
test/lib/jdk/test/lib/util/ForceGC.java line 58:
> 56: Reference.reachabilityFence(ref);
> 57:
> 58: for (int retries = (int)(timeout / 200); retries >= 0; retries--)
> {
The logic around the timeout might be clearer if it was only based on the
number of retries,
and can be scaled by the TIMEOUT_FACTOR too.
test/lib/jdk/test/lib/util/ForceGC.java line 70:
> 68: // But it is fine. For most cases, the 1st GC is
> sufficient
> 69: // to trigger and complete the cleanup.
> 70: queue.remove(200L);
If `remove()` returns a non-null value, then it is safe to break out of the
loop.
The GC has cleared the ref. And the final `getAsBoolean()` below will return
the condition.
-
PR: https://git.openjdk.org/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v9]
On Thu, 16 Jun 2022 15:55:00 GMT, Xue-Lei Andrew Fan wrote: >> This is a follow up update per comments in [JDK-8287384 >> PR](https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/8907__;!!ACWV5N9M2RV99hQ!LTtncpyN_2kanJmZF3wz3O88hB5AwboRKEitwQ0UJpiaZQ7RhIZpzgU4kCL3gwrt_JRqji2j5Tb92sOTWGguKw$ >> ). The tier1 and tier2 test in open part looks good to me. Please help to >> run Mach5 just case the closed test cases are impacted. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > remove trailing whitespaces test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 64: > 62: assertNotNull(myOwnClassLoaderWeakReference.get()); > 63: > 64: assertTrue(ForceGC.wait(() -> myOwnClassLoaderWeakReference.get() > == null)); The call to ref.get() can create a strong reference to the object; depending on the timing it might interfere with the GC in process. `ref.refersTo(null)` is preferred over `ref.get() == null` Here and all subsequent changes. - PR: https://git.openjdk.org/jdk/pull/8979
