Re: RFR: JDK-8283415: Update java.lang.ref to use sealed classes
On Sat, 19 Mar 2022 22:30:13 GMT, Joe Darcy wrote: > Another refactoring to use sealed classes where possible, this time in the > java.lang.ref package. > > Copyright dates will be updated, if needed, before a push. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7874
RFR: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes
Can I please get a review of this change which handles https://bugs.openjdk.java.net/browse/JDK-8283411? The commit here moves the temporary byte array from being a member of the class to a local variable within the `skip` method which is the only place where it is used as a temporary buffer. tier1, tier2, tier3 tests have been run successfully with this change. - Commit messages: - 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes Changes: https://git.openjdk.java.net/jdk/pull/7875/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7875=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283411 Stats: 3 lines in 1 file changed: 1 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7875/head:pull/7875 PR: https://git.openjdk.java.net/jdk/pull/7875
FYI, candidate list of classes to be updated to sealed in JDK-8283414: Update java.base to use sealed classes (umbrella)
Hello, With a number of efforts to update various class hierarchies in the JDK to sealed classes, I wrote an annotation processor to find candidates to be changed to sealed. The candidates for this analysis are non-final public classes with at least one package access constructor and no public constructors. That is, the candidate class cannot be generally subclassable. Several dozen candidates were identified and listed in JDK-8283414, including two cases where changing the classes in question to sealed was already out for review (JDK-8282536 and JDK-8283237). I've created subtasks for a number of other cases in core-libs; the cases in security libs could also be reviewed. -Joe
RFR: JDK-8283415: Update java.lang.ref to use sealed classes
Another refactoring to use sealed classes where possible, this time in the java.lang.ref package. Copyright dates will be updated, if needed, before a push. - Commit messages: - JDK-8283415: Update java.lang.ref to use sealed classes Changes: https://git.openjdk.java.net/jdk/pull/7874/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7874=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283415 Stats: 6 lines in 5 files changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7874.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7874/head:pull/7874 PR: https://git.openjdk.java.net/jdk/pull/7874
Re: RFR: JDK-8277520: Implement JDK-8 default methods for IdentityHashMap [v4]
On Mon, 21 Feb 2022 23:36:19 GMT, liach wrote: >> Might need a CSR as now `computeIfAbsent` `computeIfPresent` `compute` >> `merge` would throw CME if the functions modified the map itself, and there >> are corresponding specification changes. > > liach has updated the pull request incrementally with two additional commits > since the last revision: > > - merge branch 'identityhashmap-bench' of https://github.com/liachmodded/jdk > into identityhashmap-default > - fix whitespace Also re stuart: The relative performance of `putIfAbsent` versus `put` has always improved with this patch, now being 1.01 times of `put` call time while previously it was 1.2 times of it, regardless of jvm instance noise. My main concern is the performance hit brought by my move of the while loops in every method into a consolidated `findInterestingIndex`. This part is somehow harder to get consistent results, or should I make a clean copy of `IdentityHashMap` on new implementation and test (works only if there is no existing hotspot optimizations on `IdentityHashMap`) - PR: https://git.openjdk.java.net/jdk/pull/6532
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote:
>> Change `CallSite` to a sealed class, as `CallSite` is an abstract class
>> which does not allow direct subclassing by users per its documentation.
>> Since I don't have a JBS account, I posted the content for the CSR in a
>> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and
>> wish someone can submit a CSR for me.
>
> liach has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Remove homemade callsite from test
Hmm, I think the purpose of this test is just to fill up the class constant
pool with random garbage and to ensure it runs, and using either type of indy
doesn't seem to make big differences.
Moreover, these tests [need to be
reworked](https://github.com/openjdk/jdk/blob/d8893fad23d1ee6841336b96c34599643edb81ce/test/hotspot/jtreg/vmTestbase/README.md),
but I don't think I am suited for such a rework for this test.
--
If we do tweak the test, a subclass of `ConstantCallSite` would work too:
public class CustomCallSite extends ConstantCallSite {
public CustomCallSite(MethodHandles.Lookup lookup, String callName,
MethodType mtype, MethodHandle handle) {
super(handle);
}
}
And we write bytecode that makes an indy using this ctor as bootstrap method
and additionally pass a `Handle` with `fullClassName`, `TARGET_METHOD_NAME`,
`TARGET_METHOD_SIGNATURE`.
https://github.com/openjdk/jdk/blob/d8893fad23d1ee6841336b96c34599643edb81ce/test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/share/GenManyIndyCorrectBootstrap.java#L107-L109
Becomes
mw.visitInvokeDynamicInsn(TARGET_METHOD_NAME,
TARGET_METHOD_SIGNATURE,
bsm,
customCallSite ? new Object[] {TARGET_HANDLE} : new Object[0]);
The problem with this approach is:
1. Where should I put this `CustomCallSite` class? The bootstrap is only a
class generator as opposed to a runtime, and this class probably shouldn't be
put into the patches for `java.lang`. (In fact, the patches on asm should be
replaced by patches on a shaded standalone asm library to reduce compile time
for tests, too)
- Should I generate it in the `generateBytecodes` method instead?
2. The approach of using a indy + a MethodHandle constant would use an extra
constant, wonder if it hurts the goal of filling up the constant pool without
exceeding the limit.
-
PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: JDK-8277520: Implement JDK-8 default methods for IdentityHashMap [v4]
On Mon, 21 Feb 2022 23:36:19 GMT, liach wrote:
>> Might need a CSR as now `computeIfAbsent` `computeIfPresent` `compute`
>> `merge` would throw CME if the functions modified the map itself, and there
>> are corresponding specification changes.
>
> liach has updated the pull request incrementally with two additional commits
> since the last revision:
>
> - merge branch 'identityhashmap-bench' of https://github.com/liachmodded/jdk
> into identityhashmap-default
> - fix whitespace
src/java.base/share/classes/java/util/IdentityHashMap.java line 523:
> 521: * mapping was in the map
> 522: */
> 523: private boolean removeMapping(Object key, Object value) {
Note that `IdentityHashMap.remove(Object, Object)` needs to call this method,
as the default implementation calls `Objects.equals(…)`:
-
PR: https://git.openjdk.java.net/jdk/pull/6532
Re: RFR: JDK-8277520: Implement JDK-8 default methods for IdentityHashMap [v4]
On Sat, 19 Mar 2022 00:45:14 GMT, Stuart Marks wrote: >> @stuart-marks Could you help me with this? >> >> In my JMH benchmark runs, I often find that ambient differences (from the >> jdk processes, the random test data generated, etc.) hide the actual >> performance difference caused by the patches. >> >> Hence, I need help in these two area: >> 1. I probably need to make a baseline benchmark that can discount the >> fundamental differences between jdk processes. What should it be? >> 2. How do I generate consistent data set for all test runs to avoid >> contributing to errors? > > @liach I don't have much time to spend on this. Several comments. > > JMH benchmarking is more than a bit of an art. There's a lot of information > in the JMH samples, so I'd recommend going through them in detail if you > haven't already. There are a couple of obvious things to look at, such as to > make sure to return values produced by the computation (or use black holes); > to fork multiple JVMs during the benchmark run; to reduce or eliminate > garbage generation during the test, or account for it if it can't be > eliminated; and so forth. > > In this particular case it's not clear to me how much performance there is to > be gained from overriding the default methods. Suppose an entry exists and > `compute(k, bifunc)` is called. The default method calls `get` to obtain the > value, calls the bifunction, then calls `put(k, newVal)`. An optimized > implementation would remember the location of the mapping so that the new > value could be stored without probing again. But probing in an > IdentityHashMap is likely pretty inexpensive: the identity hashcode is > cached, finding the table index is integer arithmetic, and searching for the > right mapping is reference comparisons on table entries that are probably > already cached. It doesn't seem likely to me that there is much to be gained > here in the first place. > > Then again, one's intuition about performance, including mine, is easily > wrong! But: if you're having trouble writing a benchmark that demonstrates a > performance improvement, maybe that means there isn't much performance to be > gained. > > As a general comment I'd suggest pursuing performance improvements only when > there's a demonstrated performance issue. This kind of looks to me like a > case of starting with "I bet I can speed this up by changing this code" and > then trying to justify that with benchmarks. If so, that's kind of backwards, > and it can easily lead to a lot of time being wasted. > @stuart-marks Note that the default implementation of those methods assume > that the map uses `Object.equals(…)` and `Object.hashCode()`, which doesn’t > apply to `IdentityHashMap`. > > This means that the performance argument is secondary to implementation > correctness. That was tracked in a separate ticket [JDK-8178355](https://bugs.openjdk.java.net/browse/JDK-8178355). My current implementations utilize the newly added `findInterestingIndex`, which I plan to use in a fix for that bug in addition, as opposed to writing a while loop in virtually every method in the class. - PR: https://git.openjdk.java.net/jdk/pull/6532
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test The purpose of `GenManyIndyCorrectBootstrap`’s `NewInvokeSpecialCallSite` is to check that bootstrap methods work correctly with a `REF_newInvokeSpecial` method handle. Instead, it should probably be implemented by subclassing `MutableCallSite`. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: JDK-8277520: Implement JDK-8 default methods for IdentityHashMap [v4]
On Sat, 19 Mar 2022 00:45:14 GMT, Stuart Marks wrote: >> @stuart-marks Could you help me with this? >> >> In my JMH benchmark runs, I often find that ambient differences (from the >> jdk processes, the random test data generated, etc.) hide the actual >> performance difference caused by the patches. >> >> Hence, I need help in these two area: >> 1. I probably need to make a baseline benchmark that can discount the >> fundamental differences between jdk processes. What should it be? >> 2. How do I generate consistent data set for all test runs to avoid >> contributing to errors? > > @liach I don't have much time to spend on this. Several comments. > > JMH benchmarking is more than a bit of an art. There's a lot of information > in the JMH samples, so I'd recommend going through them in detail if you > haven't already. There are a couple of obvious things to look at, such as to > make sure to return values produced by the computation (or use black holes); > to fork multiple JVMs during the benchmark run; to reduce or eliminate > garbage generation during the test, or account for it if it can't be > eliminated; and so forth. > > In this particular case it's not clear to me how much performance there is to > be gained from overriding the default methods. Suppose an entry exists and > `compute(k, bifunc)` is called. The default method calls `get` to obtain the > value, calls the bifunction, then calls `put(k, newVal)`. An optimized > implementation would remember the location of the mapping so that the new > value could be stored without probing again. But probing in an > IdentityHashMap is likely pretty inexpensive: the identity hashcode is > cached, finding the table index is integer arithmetic, and searching for the > right mapping is reference comparisons on table entries that are probably > already cached. It doesn't seem likely to me that there is much to be gained > here in the first place. > > Then again, one's intuition about performance, including mine, is easily > wrong! But: if you're having trouble writing a benchmark that demonstrates a > performance improvement, maybe that means there isn't much performance to be > gained. > > As a general comment I'd suggest pursuing performance improvements only when > there's a demonstrated performance issue. This kind of looks to me like a > case of starting with "I bet I can speed this up by changing this code" and > then trying to justify that with benchmarks. If so, that's kind of backwards, > and it can easily lead to a lot of time being wasted. @stuart-marks Note that the default implementation of those methods assume that the map uses `Object.equals(…)` and `Object.hashCode()`, which doesn’t apply to `IdentityHashMap`. This means that the performance argument is secondary to implementation correctness. - PR: https://git.openjdk.java.net/jdk/pull/6532
Visual Studio 2005/2008 code in 'src/java.base/share/native/launcher/main.c'
Hello. During my review of the JDK codebase, I found that launcher's 'main.c' still has code, which included only with Visual Studio 2005 and 2008. https://github.com/openjdk/jdk/blob/3f923b82c31325504430b50dee262fd460004e7b/src/java.base/share/native/launcher/main.c#L38 #if _MSC_VER > 1400 && _MSC_VER < 1600 /* * When building for Microsoft Windows, main has a dependency on msvcr??.dll. * * When using Visual Studio 2005 or 2008, that must be recorded in #ifdef _MSC_VER I wonder, shouldn't such code be removed, when JDK drops support of old compiler/toolchains? As I know, JDK now supports only recent versions of Visual Studio. (2017+). Or this file should be compilable with older versions as well? Andrey Turbanov
Unused paramter 'boolean newln' in java.lang.VersionProps#print(boolean err, boolean newln)
Hello. I found a suspicious method java.lang.VersionProps#print with unused parameter 'boolean newln'. This class is generated from template - https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/VersionProps.java.template#L203 It's unused since integration of 'JDK-8169069 Module system implementation refresh (11/2016)' - https://github.com/openjdk/jdk/commit/fbe85300bfcc69cb4dd56e4df33ceea632366283 Andrey Turbanov
Integrated: 8283287: ClassLoader.c cleanups
On Wed, 16 Mar 2022 21:25:37 GMT, Tyler Steele wrote: > As mentioned in the issue, I'd like to perform the following tidying on > ClassLoader.c > > - Alphabetize includes. > - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. > - Replace 'return 0' with 'return NULL'. This pull request has now been integrated. Changeset: 3e58a438 Author:Tyler Steele Committer: Thomas Stuefe URL: https://git.openjdk.java.net/jdk/commit/3e58a438e9051d4c976273eea35e36d37d5428c3 Stats: 33 lines in 1 file changed: 8 ins; 5 del; 20 mod 8283287: ClassLoader.c cleanups Reviewed-by: stuefe, alanb, rriggs - PR: https://git.openjdk.java.net/jdk/pull/7846
