Re: RFR: 8261449: Micro-optimize JVM_LatestUserDefinedLoader [v2]
On Wed, 10 Feb 2021 15:26:50 GMT, Thomas Stuefe wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Added a comment > > Looks good. I find it simpler too. > > You could run the tests with sun.reflect.inflationThreshold=0. Should > increase the chance of encountering reflection delegator loaders a lot. > > Cheers, Thomas Thanks! Any reviewers from JDK side? @AlanBateman? - PR: https://git.openjdk.java.net/jdk/pull/2485
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote:
> This issue adds a new event to improve diagnostic information of Java
> deserialization. The event captures the details of deserialization activity
> from ObjectInputStream. The event details are similar to that of the serial
> filter, but is agnostic of whether a filter is installed or not. The event
> also captures the filter status, if there is one.
src/java.base/share/classes/java/io/ObjectInputStream.java line 1366:
> 1364: DeserializationEvent event = new DeserializationEvent();
> 1365: if (event.shouldCommit()) {
> 1366: event.filterStatus = status == null ? "n/a" : status.name();
We use null for missing value, so no need to have "n/a"
src/java.base/share/classes/java/io/ObjectInputStream.java line 1372:
> 1370: event.depth = depth;
> 1371: event.bytesRead = bytesRead;
> 1372: event.exception = Objects.toString(ex, "n/a");
You may want to change the name of the field to "exceptionMessage" to make it
clear it's the message, not the class.
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 45:
> 43:
> 44: @Label ("Class")
> 45: public String clazz;
We typically use "type" when referring to a class.
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 45:
> 43:
> 44: @Label ("Class")
> 45: public String clazz;
Is it possible to make the field of type Class?
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 51:
> 49:
> 50: @Label ("Reference count")
> 51: public long totalObjectRefs;
"Reference count" sounds a bit like resource counter? Is that the case? If not,
perhaps "Object References" is better. We tried to avoid abbreviations. How
about naming the field "totalObjectReferences" or just "objectReferences"?
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:29:11 GMT, Stuart Marks wrote: >> Okay, I see. > > The note itself should be here, but it's demarcated with an `@apiNote` tag. > This introduces a subhead "API Note:" in the rendered javadoc. Thus, it's not > necessary to start the text with "Note:". My thinking here was to include the exact string "Note: this class has a natural ordering that is inconsistent with equals." in the text of BigDecimal since that is the phrase java.lang.Comparable recommends. This should improve grep-ability, etc. for such classes in the JDK. The class-level summary has a semantically equivalent statement using different wording, which left the compareTo method as a place to introduce the exact phrase. I use an @apiNote both because it is an informative statement and to prevent the text from necessarily being applied to BigDecimal subclasses. BigDecimal is not final and people can and sometimes do subclass it and those subclasses, in principle, could have a (different) natural order that was consistent with equals. - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:24:40 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> src/java.base/share/classes/java/math/BigDecimal.java line 97:
>
>> 95: * contrast, the {@link equals equals} method requires both the
>> 96: * numerical value and representation to be the same for equality to
>> 97: * hold.
>
> Note, discussing "representation" is ok here since the context is discussing
> the representation of BigDecimal (in contrast to the text in Comparable).
It might be reasonable to add a bit of rationale here and give an example. I
think the reason that members of the same cohort might not be considered
`equals()` to one another is that they are not substitutable. For example,
consider 2.0 and 2.00. They are members of the same cohort, because
new BigDecimal("2.0").compareTo(new BigDecimal("2.00")) == 0
is true. However,
new BigDecimal("2.0").equals(new BigDecimal("2.00"))
is false. They aren't considered `equals()` because they aren't substitutable,
since using them in an arithmetic expression can give different results. For
example:
new BigDecimal("2.0").divide(new BigDecimal(3), RoundingMode.HALF_UP)
==> 0.7
new BigDecimal("2.00").divide(new BigDecimal(3), RoundingMode.HALF_UP)
==> 0.67
I think that's the right rationale, and a reasonable example to illustrate it.
Edit to taste. I think it would be good to have material like this, though,
because people's immediate reaction to BD being inconsistent with equals is
"well that's wrong." Well, no, it's right, and I think this is the reason.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Wed, 10 Feb 2021 01:49:55 GMT, Joe Darcy wrote:
>> A follow-up of sorts to JDK-8257086, this change aims to improve the
>> discussion of the relationship between Object.equals and compareTo and
>> compare methods. The not-consistent-with-equals natural ordering of
>> BigDecimal get more explication too. While updating Object, I added some
>> uses of apiNote and implSpec, as appropriate. While a signum method did not
>> exist when Comparable was added, it has existed for many years so I removed
>> obsolete text that defined a "sgn" function to compute signum.
>>
>> Once the changes are agreed to, I'll reflow paragraphs and update the
>> copyright years.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix typos in javadoc tags found during review.
Overall very nice. Mostly my comments are wordsmithing. Two issues are worth
considering; see detailed comments inline.
1) Do we want a general statement on the stability of the value returned by
toString()?
2) Add rationale and example for why BigDecimal's natural ordering is
inconsistent with equals.
src/java.base/share/classes/java/lang/Comparable.java line 68:
> 66: * orderings that are consistent with equals. One exception is
> 67: * {@link java.math.BigDecimal}, whose {@linkplain
> java.math.BigDecimal#compareTo natural ordering} equates
> 68: * {@code BigDecimal} objects with equal numerical values and different
> representations
Nothing here talks about the representation of BigDecimal. I think it would be
preferable to say that in BigDecimal, the `equals()` method considers 4.0 and
4.00 unequal because they have different precisions; however, `compareTo()`
considers them equal because it considers only their numerical values.
src/java.base/share/classes/java/lang/Comparable.java line 90:
> 88: * of the {@code equals} method and the equivalence classes defined by
> 89: * the quotient of the {@code compareTo} method are the same.
> 90: *
I think in both cases it should be "the equivalence class defined by" That
is, "equivalence class" should be singular. But there are two of them, so the
sentence still properly concludes "... are the same."
src/java.base/share/classes/java/lang/Comparable.java line 110:
> 108: * {@link Integer#signum signum}{@code (x.compareTo(y)) ==
> -signum(y.compareTo(x))}
> 109: * for all {@code x} and {@code y}. (This
> 110: * implies that {@code x.compareTo(y)} must throw an exception iff
I'd suggest replacing "iff" with "if and only if" because it looks like a typo,
and writing out the long form emphasizes the bidirectional nature of the
implication.
src/java.base/share/classes/java/lang/Object.java line 135:
> 133: * into equivalence classes; all the members of an
> 134: * equivalence class are equal to each other. The equal objects
> 135: * are substitutable for each other, at least for some purposes.
Since the preceding sentence mentions the members of an equivalence class, it
might read better to say "Members are substitutable..." or possibly "Members of
an equivalence class are substitutable"
src/java.base/share/classes/java/lang/Object.java line 149:
> 147: *
> 148: * @apiNote
> 149: * Note that it is generally necessary to override the {@link
> hashCode hashCode}
The `@apiNote` introduces the paragraph with a subhead "API Note:" so it's a
bit redundant to say "Note that" Maybe just start with "It is generally
necessary"
src/java.base/share/classes/java/lang/Object.java line 236:
> 234: * be a concise but informative representation that is easy for a
> 235: * person to read.
> 236: * It is recommended that all subclasses override this method.
Do we want to have a general note here or somewhere that the exact format of
the result of `toString()` is generally not stable, and that it is subject to
change without notice?
src/java.base/share/classes/java/math/BigDecimal.java line 97:
> 95: * contrast, the {@link equals equals} method requires both the
> 96: * numerical value and representation to be the same for equality to
> 97: * hold.
Note, discussing "representation" is ok here since the context is discussing
the representation of BigDecimal (in contrast to the text in Comparable).
src/java.base/share/classes/java/util/Comparator.java line 95:
> 93: * equals, the equivalence classes defined by the equivalence relation
> 94: * of the {@code equals} method and the equivalence classes defined by
> 95: * the quotient of the {@code compare} method are the same.
As before, suggest "equivalence class" be singular in both cases.
src/java.base/share/classes/java/util/Comparator.java line 159:
> 157: * and it imposes the same ordering as this comparator. Thus,
> 158: * {@code comp1.equals(comp2)} implies that {@code
> signum(comp1.compare(o1,
> 159: * o2))==signum(comp2.compare(o1, o2))} for every
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Wed, 10 Feb 2021 02:55:14 GMT, Brian Burkhalter wrote: >> This is the exact text recommended in java.lang.Comparable when a type's >> natural ordering is inconsistent with equals. The statement to that effect >> at the top of BigDecimal didn't use that exact wording > > Okay, I see. The note itself should be here, but it's demarcated with an `@apiNote` tag. This introduces a subhead "API Note:" in the rendered javadoc. Thus, it's not necessary to start the text with "Note:". - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261482: Adjust jmap histo command to accept noparallel option to inspect heap serially
On Thu, 11 Feb 2021 02:36:01 GMT, Lin Zang wrote: > 8261482: Adjust jmap histo command to accept noparallel option to inspect > heap serially Dear All, Stories and discussions related with this PR could be found at #2379 and #2261. Just FYI. BRs, Lin - PR: https://git.openjdk.java.net/jdk/pull/2519
RFR: 8261482: Adjust jmap -histo to accept noparallel option to inspect heap serially
8261482: Adjust jmap -histo to accept noparallel option to inspect heap serially - Commit messages: - 8261482: Adjust jmap -histo to accept noparallel option to inspect heap serially Changes: https://git.openjdk.java.net/jdk/pull/2519/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2519=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261482 Stats: 50 lines in 4 files changed: 7 ins; 22 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/2519.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2519/head:pull/2519 PR: https://git.openjdk.java.net/jdk/pull/2519
Re: RFR: 8261418: Reduce decoder creation overheads for sun.nio.cs.ext Charsets [v3]
On Wed, 10 Feb 2021 23:35:54 GMT, Claes Redestad wrote: >> This refactor some `sun.nio.cs.ext` charsets, such as ISO-2022-CN-GB, >> ISO-2022-CN-CNS, ISO-2022-KR and a few others to use static rather than >> per-instance auxiliary decoders. Doing so reduce overheads of calling >> `charset.newDecoder()`. This reduce or eliminate regressions on `new >> String(byte[], String)` operations due the removal of thread-local decoder >> caching in [JDK-8259842](https://bugs.openjdk.java.net/browse/JDK-8259842) >> >> Most ISO-2022 Charsets define a specialized decoder already. The >> `ISO2022.Decoder` class was only used by `ISO2022_KR`, so folding it into >> that implementation and simplifying the code brings a rather significant >> speed-up, both to decoder creation and on actual decoding. >> >> Testing: tier1-3, manual runs of sun.nio.cs tests > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment about removing the generic ISO2022.Decoder Thanks. The newly added comment to ISO2022 is helpful. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2480
Re: RFR: 8261418: Reduce decoder creation overheads for sun.nio.cs.ext Charsets [v3]
> This refactor some `sun.nio.cs.ext` charsets, such as ISO-2022-CN-GB, > ISO-2022-CN-CNS, ISO-2022-KR and a few others to use static rather than > per-instance auxiliary decoders. Doing so reduce overheads of calling > `charset.newDecoder()`. This reduce or eliminate regressions on `new > String(byte[], String)` operations due the removal of thread-local decoder > caching in [JDK-8259842](https://bugs.openjdk.java.net/browse/JDK-8259842) > > Most ISO-2022 Charsets define a specialized decoder already. The > `ISO2022.Decoder` class was only used by `ISO2022_KR`, so folding it into > that implementation and simplifying the code brings a rather significant > speed-up, both to decoder creation and on actual decoding. > > Testing: tier1-3, manual runs of sun.nio.cs tests Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add comment about removing the generic ISO2022.Decoder - Changes: - all: https://git.openjdk.java.net/jdk/pull/2480/files - new: https://git.openjdk.java.net/jdk/pull/2480/files/aa24f031..fa593915 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2480=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2480=01-02 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2480.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2480/head:pull/2480 PR: https://git.openjdk.java.net/jdk/pull/2480
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]
On Tue, 9 Feb 2021 14:08:10 GMT, Philippe Marschall
wrote:
>> src/java.base/share/classes/java/io/Reader.java line 198:
>>
>>> 196: } else {
>>> 197: int remaining = target.remaining();
>>> 198: char cbuf[] = new char[Math.min(remaining,
>>> TRANSFER_BUFFER_SIZE)];
>>
>> As `cbuf` for the off-heap case is used in a synchronized block, is there
>> the opportunity for some sort of cached array here and would it help?
>
> That would be possible. It would help in cases where a large Reader is read
> into one or several relatively small off-heap CharBuffers, requiring multiple
> #read calls. This can only be done when the caller is able to work with only
> a partial input. I don't know how common this case is.
>
> We could re-purpose #skipBuffer, it has the same maximum size (8192) but
> determined by a different constant (#maxSkipBufferSize instead of
> #TRANSFER_BUFFER_SIZE). That would likely require it to be renamed and maybe
> we should even remove #maxSkipBufferSize. We could also do the reallocation
> and growing similar as is currently done in #skip.
Perhaps a static final `WORK_BUFFER_SIZE` could be added with value 8192 and
`maxSkipBufferSize` and `TRANSFER_BUFFER_SIZE` replaced with that? Then
`skipBuffer` could be renamed to `workBuffer` and used in both
`read(CharBuffer)` and `skip(long)`. That shouldn't be a problem as both uses
are in synchronized blocks. Also I suggest putting the declaration of
`workBuffer` just below that of `lock` instead of lower down the file where
`skipBuffer` is.
Lastly you mentioned C-style array declarations like `char buf[]`. As there are
only four of these in the file it might be good to just go ahead and change
them, I don't think that adds much noise or risk.
-
PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote:
> This issue adds a new event to improve diagnostic information of Java
> deserialization. The event captures the details of deserialization activity
> from ObjectInputStream. The event details are similar to that of the serial
> filter, but is agnostic of whether a filter is installed or not. The event
> also captures the filter status, if there is one.
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 35:
> 33:
> 34: @Category({"Java Development Kit", "Serialization"})
> 35: @Label("Deserialization events")
This seems to differ from the format of other event labels defined in this
package, e.g:
@Label("Process Start")
@Label("File Read")
...
What would you think of changing it to one of:
@Label("Deserialization")
@Label("Deserialized Object")
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote: > This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote: > This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Marked as reviewed by coffeys (Reviewer). test/jdk/java/io/Serializable/serialFilter/GlobalFilterTest.java line 50: > 48: * -Dexpected-jdk.serialFilter=java.** GlobalFilterTest > 49: * @run testng/othervm/policy=security.policy GlobalFilterTest > 50: * @run testng/othervm/policy=security.policy You may want to add a "@requires vm.hasJFR" condition to this test - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote: > This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. The logging in ObjectInputStream remains conditional on whether a filter is installed, which that seems reasonable since the logging is filter specific, while the JRF event is not (but both carry effectively the same information). The new jdk.Deserialization event uses a String to carry the filterStatus value. The value could be represented by its enum ordinal, but then the tool consuming the event would need to map that back to its string value to be meaningful. - PR: https://git.openjdk.java.net/jdk/pull/2479
RFR: 8261160: Add a deserialization JFR event
This issue adds a new event to improve diagnostic information of Java deserialization. The event captures the details of deserialization activity from ObjectInputStream. The event details are similar to that of the serial filter, but is agnostic of whether a filter is installed or not. The event also captures the filter status, if there is one. - Commit messages: - minor cleanup; all tests passing - Unconditionally fire a deser event regardless of filter, and add test - commit 2 - commit 1 Changes: https://git.openjdk.java.net/jdk/pull/2479/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2479=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261160 Stats: 516 lines in 12 files changed: 496 ins; 5 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261449: Micro-optimize JVM_LatestUserDefinedLoader [v2]
On Wed, 10 Feb 2021 07:34:59 GMT, Aleksey Shipilev wrote: >> `JVM_LatestUserDefinedLoader` is called normally from >> `ObjectInputStream.resolveClass` -> `VM.latestUserDefinedLoader0`. And it >> takes a measurable time to walk the stack. There is JDK-8173368 that wants >> to replace it with `StackWalker`, but we can tune up the >> `JVM_LatestUserDefinedLoader` itself without changing the semantics of it >> (thus providing the backportability, including the releases that do not have >> `StackWalker`) and improving performance (thus providing a more aggressive >> baseline for `StackWalker` rewrite). >> >> The key is to recognize that out of two checks: 1) checking for two special >> subclasses; 2) checking for user classloader -- the first one usually >> passes, and second one fails much more frequently. First check also requires >> traversing the superclasses upwards looking for match. Reversing the order >> of the checks, plus inlining the helper method improves performance without >> changing the semantics. >> >> Out of curiosity, my previous patch dropped the first check completely, >> replacing it by asserts, and we definitely run into situation where that >> check is needed on some tests. >> >> On my machine, `VM.latestUserDefinedLoader` invocation time drops from 115 >> to 100 ns/op. Single-threaded SPECjvm2008:serial improves about 3% with this >> patch. >> >> Additional testing: >> - [x] Ad-hoc benchmarks >> - [x] Linux x86_64 fastdebug, `tier1`, `tier2`, `tier3` >> >> - >> ### Progress >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> - [ ] Change must be properly reviewed >> >> >> >> ### Download >> `$ git fetch https://git.openjdk.java.net/jdk pull/2485/head:pull/2485` >> `$ git checkout pull/2485` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Added a comment Looks good. I find it simpler too. You could run the tests with sun.reflect.inflationThreshold=0. Should increase the chance of encountering reflection delegator loaders a lot. Cheers, Thomas - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2485
Integrated: 8261298: LinuxPackage.c, getJvmLauncherLibPath RPM->DEB typo
On Mon, 8 Feb 2021 08:49:17 GMT, Aleksey Shipilev wrote:
> SonarCloud instance reports as new warning after JDK-8254702:
>
> This branch can not be reached because the condition duplicates a previous
> condition in the same sequence of "if/else if" statements.
>
> char* getJvmLauncherLibPath(void) {
>...
> if (PACKAGE_TYPE_RPM == pkg->type) {
> pkgQueryCmd = "rpm -ql '%s' 2>/dev/null";
> } else if (PACKAGE_TYPE_RPM == pkg->type) { <--- here
> pkgQueryCmd = "dpkg -L '%s' 2>/dev/null";
>
> Seems like an obvious typo.
>
> Additional tests:
> - [x] Linux x86_64 (Ubuntu) `tools/jpackage`
This pull request has now been integrated.
Changeset: a7726390
Author:Aleksey Shipilev
URL: https://git.openjdk.java.net/jdk/commit/a7726390
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
8261298: LinuxPackage.c, getJvmLauncherLibPath RPM->DEB typo
Reviewed-by: asemenyuk, almatvee, herrick
-
PR: https://git.openjdk.java.net/jdk/pull/2452
Re: RFR: 8258588: MD5 MessageDigest in java.util.UUID should be cached
On Wed, 10 Feb 2021 14:08:22 GMT, PROgrm_JARvis wrote: >>> Hi Claes, >>> Would flattening the state of MD5 bring any further improvements? >>> [plevart@92bf48f](https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083) >> >> I think it might, marginally, but it seemed to me that the implCompress0 MD5 >> intrinsic is using `state` so I've not tried that yet since rewriting and >> checking the intrinsic code is a lot of work. (I've blogged about my >> experiments in this area and mentioned this issue in passing: >> https://cl4es.github.io/2021/01/04/Investigating-MD5-Overheads.html#accidental-constraints) > >> >> >> @JarvisCraft This pull request has been inactive for more than 4 weeks and >> will be automatically closed if another 4 weeks passes without any activity. >> To avoid this, simply add a new comment to the pull request. Feel free to >> ask for assistance if you need help with progressing this pull request >> towards integration! FWIW, I ended up doing two related improvements: - #1855 - which reduced overheads of constructing MD5, SHA1, SHA2, SHA5 `MessageDigest` objects, and slightly improving digest performance - #1933 - reducing cost of `MessageDigest.getInstance` in general by better internal caching of `Constructor` objects, among other things. With this there's now no extra allocations when looking up something that has been looked up before. Together these enhancements bring the overheads of `UUID.nameUUIDFromBytes` down close to what you can get from cloning from a `ThreadLocal` `MD5` object as suggested here. Taking the ambient overheads of `ThreadLocal`s into account I'd say it's not worth adding this cache, and would suggest withdrawing this PR and closing the RFE. - PR: https://git.openjdk.java.net/jdk/pull/1821
Re: RFR: 8261298: LinuxPackage.c, getJvmLauncherLibPath RPM->DEB typo
On Mon, 8 Feb 2021 08:49:17 GMT, Aleksey Shipilev wrote:
> SonarCloud instance reports as new warning after JDK-8254702:
>
> This branch can not be reached because the condition duplicates a previous
> condition in the same sequence of "if/else if" statements.
>
> char* getJvmLauncherLibPath(void) {
>...
> if (PACKAGE_TYPE_RPM == pkg->type) {
> pkgQueryCmd = "rpm -ql '%s' 2>/dev/null";
> } else if (PACKAGE_TYPE_RPM == pkg->type) { <--- here
> pkgQueryCmd = "dpkg -L '%s' 2>/dev/null";
>
> Seems like an obvious typo.
>
> Additional tests:
> - [x] Linux x86_64 (Ubuntu) `tools/jpackage`
Marked as reviewed by herrick (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/2452
Re: RFR: 8258588: MD5 MessageDigest in java.util.UUID should be cached
On Thu, 7 Jan 2021 17:09:14 GMT, Claes Redestad wrote: >> Hi Claes, >> Would flattening the state of MD5 bring any further improvements? >> https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083 > >> Hi Claes, >> Would flattening the state of MD5 bring any further improvements? >> [plevart@92bf48f](https://github.com/plevart/jdk/commit/92bf48ff58f0ce9648e49466dbf1befebbf49083) > > I think it might, marginally, but it seemed to me that the implCompress0 MD5 > intrinsic is using `state` so I've not tried that yet since rewriting and > checking the intrinsic code is a lot of work. (I've blogged about my > experiments in this area and mentioned this issue in passing: > https://cl4es.github.io/2021/01/04/Investigating-MD5-Overheads.html#accidental-constraints) > > > @JarvisCraft This pull request has been inactive for more than 4 weeks and > will be automatically closed if another 4 weeks passes without any activity. > To avoid this, simply add a new comment to the pull request. Feel free to ask > for assistance if you need help with progressing this pull request towards > integration! - PR: https://git.openjdk.java.net/jdk/pull/1821
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v8]
On Wed, 10 Feb 2021 07:46:02 GMT, Michael McMahon wrote: >> Could I get the following change reviewed please? It fixes a problem (in >> JEP380) on Windows where some file operations on Unix domain sockets were >> not working and led to the feature being disabled on Windows 2019 Server in >> JDK 16. So, the fix re-enables the feature on all versions of Windows that >> support it. >> >> The test checks all the file APIs that were affected by the problem. The >> change touches the code that handles symbolic links in Windows (since they >> are implemented as NTFS reparse points, like Unix sockets), but I didn't add >> any specific testing in this area, as I assume the existing unit tests for >> NIO symbolic links should cover that. If I should add more tests here, then >> I can do that. >> >> Thanks, >> Michael > > Michael McMahon 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 16 additional > commits since the last revision: > > - Rearrange test in WindowsPath. Change to Security.java test to explicitly > delete socket files > - Merge branch 'master' into 8252971-socket-attributes > - update > - fix mistake in last push > - update > - Merge branch 'master' into 8252971-socket-attributes > - add specific check for unix domain socket > - Merge branch 'master' into 8252971-socket-attributes > - update > - update > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/f61a6ff5...70832057 Changes LGTM. - PR: https://git.openjdk.java.net/jdk/pull/2424
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 1 Feb 2021 20:26:54 GMT, Andrew John Hughes wrote:
>> Anybody willing to review this?
>
>> Anybody willing to review this?
>
> I can have a go.
>
> I have two main concerns:
>
> 1. There seems to be little documentation on the new additions. I'm
> particularly concerned about things like CgroupV1Subsystem.java where a big
> chunk of documentation on the formats being read is removed and I don't see
> it being moved elsewhere. Documentation on the new getInstance methods would
> be helpful too.
> 2. With the new volatile fields, I think it would be better if the lock being
> held to initialise them was only held when necessary to perform the
> assignment. Holding it while performing an extensive process of parsing
> multiple files that could potentially fail seems a little dangerous. i.e.
> something like:
> if (INSTANCE == null) { CGroupV1Subsystem tmp = initSubSystem(infos);
> synchronized (CgroupV1Subsystem.class) { if (INSTANCE == null) { INSTANCE =
> tmp; } } }
@gnu-andrew Let me know what you think of the updated patch! Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/1393
