Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception [v2]
> Similar to how `MethodHandle#invoke(Exact)` methods are already handled, this
> change adds special casing for `VarHandle.{access-mode}` methods.
>
> The exception message is less exact, but I think that's acceptable.
Hannes Greule has updated the pull request incrementally with one additional
commit since the last revision:
address comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/20015/files
- new: https://git.openjdk.org/jdk/pull/20015/files/fe43b749..e329ceb2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=20015&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=20015&range=00-01
Stats: 43 lines in 2 files changed: 17 ins; 16 del; 10 mod
Patch: https://git.openjdk.org/jdk/pull/20015.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20015/head:pull/20015
PR: https://git.openjdk.org/jdk/pull/20015
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 25 Jun 2024 19:33:02 GMT, Naoto Sato wrote: >> I did not mean to introduce a public API for this change (if we do, the fix >> cannot be backported). I thought we could define a package private one, but >> based on your observation, it may get messier... So I agree that falling >> back to `StringBuf` is the way to go, IMO. > >> So, considering all the information given, is it enough to start our new >> review process? @naotoj @liach @justin-curtis-lu > > Well, I was suggesting the same buffer proxying for other Format classes than > NumberFormat subclasses, such as DateFormat so that they would have the same > performance benefit. Would you be willing to do that too? Thanks all for your valuable suggestions. All suggestions were accepted. Is it enough to start our new review process? @naotoj @justin-curtis-lu @liach @irisclark - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2207831524
Re: RFR: 8335366: Improve String.format performance with fastpath [v13]
On Tue, 2 Jul 2024 14:04:52 GMT, Shaojin Wen wrote:
>> We need a String format solution with good performance. String Template was
>> once expected, but it has been removed. j.u.Formatter is powerful, but its
>> performance is not good enough.
>>
>> This PR implements a subset of j.u.Formatter capabilities. The performance
>> is good enough that it is a fastpath for commonly used functions. When the
>> supported functions are exceeded, it will fall back to using j.u.Formatter.
>>
>> The performance of this implementation is good enough, the fastpath has low
>> detection cost, There is no noticeable performance degradation when falling
>> back to j.u.Formatter via fastpath.
>>
>> Below is a comparison of String.format and concat-based and StringBuilder:
>>
>> * benchmark java code
>>
>> public class StringFormat {
>> @Benchmark
>> public String stringIntFormat() {
>> return "%s %d".formatted(s, i);
>> }
>>
>> @Benchmark
>> public String stringIntConcat() {
>> return s + " " + i;
>> }
>>
>> @Benchmark
>> public String stringIntStringBuilder() {
>> return new StringBuilder(s).append(" ").append(i).toString();
>> }
>> }
>>
>>
>> * benchmark number on macbook m1 pro
>>
>> BenchmarkMode Cnt Score Error Units
>> StringFormat.stringIntConcat avgt 15 6.541 ? 0.056 ns/op
>> StringFormat.stringIntFormat avgt 15 17.399 ? 0.133 ns/op
>> StringFormat.stringIntStringBuilder avgt 15 8.004 ? 0.063 ns/op
>>
>>
>> From the above data, we can see that the implementation of fastpath reduces
>> the performance difference between String.format and StringBuilder from 10
>> times to 2~3 times.
>>
>> The implementation of fastpath supports the following four specifiers, which
>> can appear at most twice and support a width of 1 to 9.
>>
>> d
>> x
>> X
>> s
>>
>> If necessary, we can add a few more.
>>
>>
>> Below is a comparison of performance numbers running on a MacBook M1,
>> showing a significant performance improvement.
>>
>> -Benchmark Mode CntScoreError Units
>> (baseline)
>> -StringFormat.complexFormat avgt 15 895.954 ? 52.541 ns/op
>> -StringFormat.decimalFormat avgt 15 277.420 ? 18.254 ns/op
>> -StringFormat.stringFormat avgt 15 66.787 ? 2.715 ns/op
>> -StringFormat.stringIntFormat avgt 15 81.046 ? 1.879 ns/op
>> -StringFormat.widthStringFormat avgt 15 38.897 ? 0.114 ns/op
>> -StringFormat.widthStringIntFormat avgt 15 109.841 ? 1.028 ns/op
>>
>> +Benchmark...
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> optimize width padding
As you said above, this optimization has costs, including maintenance and
fallback costs. I plan to optimize j.u.Formatter. however, the performance
optimization on j.u.Formatter cannot achieve this optimization effect.
The benefit of this optimization is that it approaches the performance of
String.concat and hand-written StringBuilder, but exceeds them in ease of use
based on String.format.
-
PR Comment: https://git.openjdk.org/jdk/pull/19956#issuecomment-2207674846
Re: RFR: 8335645: j.u.Formatter#trailingZeros improved with String repeat
On Wed, 3 Jul 2024 21:43:05 GMT, Shaojin Wen wrote: > In JDK 21, StringBuilder added a repeat method, which can be used to improve > j.u.Formatter#trailingZeros See https://openjdk.org/guide/#life-of-a-pr point 6 for the 24-hour wait rule. - PR Comment: https://git.openjdk.org/jdk/pull/20018#issuecomment-2207452838
Re: RFR: 8335645: j.u.Formatter#trailingZeros improved with String repeat
On Wed, 3 Jul 2024 21:43:05 GMT, Shaojin Wen wrote: > In JDK 21, StringBuilder added a repeat method, which can be used to improve > j.u.Formatter#trailingZeros As a general rule, please wait for at least 24 hours to integrate, for devs around the globe to take a look (unless emergency such as a build break). - PR Comment: https://git.openjdk.org/jdk/pull/20018#issuecomment-2207447047
Re: RFR: 8335645: j.u.Formatter#trailingZeros improved with String repeat
On Wed, 3 Jul 2024 21:43:05 GMT, Shaojin Wen wrote: > In JDK 21, StringBuilder added a repeat method, which can be used to improve > j.u.Formatter#trailingZeros @wenshao Your change (at version a98bd05c857d0a04dfad471dd28f905be1345376) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/20018#issuecomment-2207438128
Re: RFR: 8335645: j.u.Formatter#trailingZeros improved with String repeat
On Wed, 3 Jul 2024 21:43:05 GMT, Shaojin Wen wrote: > In JDK 21, StringBuilder added a repeat method, which can be used to improve > j.u.Formatter#trailingZeros Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20018#pullrequestreview-2157610512
Re: RFR: 8335645: j.u.Formatter#trailingZeros improved with String repeat
On Wed, 3 Jul 2024 21:43:05 GMT, Shaojin Wen wrote: > In JDK 21, StringBuilder added a repeat method, which can be used to improve > j.u.Formatter#trailingZeros +1 - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/20018#pullrequestreview-2157583915
Re: RFR: 8335645: j.u.Formatter#trailingZeros improved with String repeat
On Wed, 3 Jul 2024 21:43:05 GMT, Shaojin Wen wrote: > In JDK 21, StringBuilder added a repeat method, which can be used to improve > j.u.Formatter#trailingZeros Marked as reviewed by liach (Committer). - PR Review: https://git.openjdk.org/jdk/pull/20018#pullrequestreview-2157534378
RFR: 8335645: j.u.Formatter#trailingZeros improved with String repeat
In JDK 21, StringBuilder added a repeat method, which can be used to improve j.u.Formatter#trailingZeros - Commit messages: - use StringBuilder#repeat Changes: https://git.openjdk.org/jdk/pull/20018/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20018&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335645 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20018.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20018/head:pull/20018 PR: https://git.openjdk.org/jdk/pull/20018
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception
On Wed, 3 Jul 2024 19:43:05 GMT, Hannes Greule wrote:
> Similar to how `MethodHandle#invoke(Exact)` methods are already handled, this
> change adds special casing for `VarHandle.{access-mode}` methods.
>
> The exception message is less exact, but I think that's acceptable.
src/hotspot/share/prims/methodHandles.cpp line 1371:
> 1369: * invoked directly.
> 1370: */
> 1371: JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject mh, jobjectArray args)) {
Suggestion:
JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject vh, jobjectArray args)) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664836522
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception
On Wed, 3 Jul 2024 19:43:05 GMT, Hannes Greule wrote:
> Similar to how `MethodHandle#invoke(Exact)` methods are already handled, this
> change adds special casing for `VarHandle.{access-mode}` methods.
>
> The exception message is less exact, but I think that's acceptable.
Great work!
src/hotspot/share/prims/methodHandles.cpp line 1372:
> 1370: */
> 1371: JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject mh, jobjectArray args)) {
> 1372: THROW_MSG_NULL(vmSymbols::java_lang_UnsupportedOperationException(),
> "VarHandle access mode method a cannot be invoked reflectively");
Suggestion:
THROW_MSG_NULL(vmSymbols::java_lang_UnsupportedOperationException(),
"VarHandle access mode methods cannot be invoked reflectively");
Looks like a typo to me.
src/hotspot/share/prims/methodHandles.cpp line 1419:
> 1417: static JNINativeMethod VH_methods[] = {
> 1418: // UnsupportedOperationException throwers
> 1419: {CC "compareAndExchange", CC "([" OBJ ")" OBJ,
> FN_PTR(VH_UOE)},
I recommend ordering these by the order in `AccessMode`, which is also the
declaration order in `VarHandle`; that way, if we add a new access mode, it's
easier for us to maintain this list.
src/hotspot/share/prims/methodHandles.cpp line 1457:
> 1455: JVM_ENTRY(void, JVM_RegisterMethodHandleMethods(JNIEnv *env, jclass
> MHN_class)) {
> 1456: assert(!MethodHandles::enabled(), "must not be enabled");
> 1457: assert(vmClasses::MethodHandle_klass() != nullptr, "should be
> present");
Should we duplicate this assert for `vmClasses::VarHandle_klass()` too?
test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 1:
> 1: /*
The copyright header's year needs an update.
test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 69:
> 67: VarHandle v = handle();
> 68:
> 69: // Try a reflective invoke using a Method, with an array of 0
> arguments
Suggestion:
// Try a reflective invoke using a Method, with the minimal required
argument
test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 72:
> 70:
> 71: Method vhm = VarHandle.class.getMethod(accessMode.methodName(),
> Object[].class);
> 72: Object args = new Object[0];
I recommend naming this `arg`, as this is the single arg to the reflected
method. Had you inlined this, you would have called `vhm.invoke(v, (Object) new
Object[0]);`
-
PR Review: https://git.openjdk.org/jdk/pull/20015#pullrequestreview-2157341254
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664744641
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664741601
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664737631
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664753008
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664751627
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664751688
Re: RFR: 8325525: Create jtreg test case for JDK-8325203 [v2]
On Wed, 3 Jul 2024 04:11:40 GMT, Vanitha B P wrote:
>> Created jtreg test case for
>> [JDK-8325203](https://bugs.openjdk.org/browse/JDK-8325203) issue.
>>
>> The JpackageTest created tests that the child process started from the app
>> launched by jpackage launcher is not automatically terminated when the the
>> launcher is terminated.
>
> Vanitha B P has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8325525 Addressed review comments
test/jdk/tools/jpackage/apps/ThirdPartyAppLauncher.java line 27:
> 25: import java.util.logging.Logger;
> 26:
> 27: public class ThirdPartyAppLauncher {
Maybe `ChildProcessAppLauncher` to match test name?
test/jdk/tools/jpackage/apps/ThirdPartyAppLauncher.java line 32:
> 30:
> 31: public static void main(String[] args) throws IOException {
> 32: ProcessBuilder processBuilder = new ProcessBuilder("regedit");
`regedit` will trigger elevation prompt if UAC is enabled, so this test will
not be able to run without user interaction. My suggestion is to use something
that does not requires elevation. For example: `calc.exe`. Also, use full path
to executable, otherwise it is not clear what will be run. I think
`System.getenv("SYSTEM")` should give path to `%WINDIR%\system32` where `calc`
is located.
test/jdk/tools/jpackage/share/JpackageTest.java line 51:
> 49: import jdk.jpackage.test.TKit;
> 50:
> 51: public class JpackageTest {
Can we put it under `test/jdk/tools/jpackage/windows`, since it is Windows only
test? Also, it needs better name. For example: `WinChildProcessTest`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1664685484
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1664702036
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1664681991
RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception
Similar to how `MethodHandle#invoke(Exact)` methods are already handled, this
change adds special casing for `VarHandle.{access-mode}` methods.
The exception message is less exact, but I think that's acceptable.
-
Commit messages:
- add test (and find missing method)
- make reflective calls to signature polymorphic methods in VarHandle throw UOE
Changes: https://git.openjdk.org/jdk/pull/20015/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20015&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8335638
Stats: 75 lines in 2 files changed: 71 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/20015.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20015/head:pull/20015
PR: https://git.openjdk.org/jdk/pull/20015
Re: RFR: 8304929: MethodTypeDesc throws an unchecked exception than ReflectiveOperationException when a component class cannot be resolved
On Wed, 3 Jul 2024 13:41:29 GMT, Jorn Vernee wrote:
>> Simple fix for `MethodTypeDescImpl`'s violation of `resolveConstantDesc`
>> specification.
>
> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java
> line 228:
>
>> 226: mtype = mt;
>> 227: } catch (TypeNotPresentException ex) {
>> 228: throw (ClassNotFoundException) ex.getCause();
>
> On a side note, I wonder if it's better to re-wrap the exception here as a
> `ReflectiveOperationException`, instead of just getting the cause. That will
> retain the entire stack trace.
@JornVernee Here are a few traces for comparison:
https://gist.github.com/5d441ab2159833e808303d1accb66ee8
In all cases, the entire stacktrace is retained; this ClassNotFoundException
has the `MethodTypeDescImpl::resolveConstantDesc` in its trace already.
I believe directly unwrapping the `ClassNotFoundException` is the best:
1. In future optimization, we can parse the individual classes more directly
(such as via `ClassDesc.resolveConstantDesc`) and the new code can just throw
the CNFE directly without extra wrapping, as user don't anticipate wrapped
causes.
2. `IllegalAccessException` throwing is done directly.
Also, would you mind to review the associated CSR as well?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19991#discussion_r1664635640
Re: RFR: 8325525: Create jtreg test case for JDK-8325203 [v2]
On Wed, 3 Jul 2024 04:11:40 GMT, Vanitha B P wrote:
>> Created jtreg test case for
>> [JDK-8325203](https://bugs.openjdk.org/browse/JDK-8325203) issue.
>>
>> The JpackageTest created tests that the child process started from the app
>> launched by jpackage launcher is not automatically terminated when the the
>> launcher is terminated.
>
> Vanitha B P has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8325525 Addressed review comments
test/jdk/tools/jpackage/share/JpackageTest.java line 79:
> 77: // parse to get regedit PID
> 78: regeditPid = Long.parseLong(pidStr.split("=", 2)[1]);
> 79: logger.info("Regedit PID is " + regeditPid);
We use `TKit.log()` for logging in jpackage tests. You can do
`TKit.log("Regedit PID is " + regeditPid)`
test/jdk/tools/jpackage/share/JpackageTest.java line 97:
> 95: throw new RuntimeException(
> 96: "Test failed: Third party software is
> terminated");
> 97: }
Use `TKit.assertTrue(isAlive, "Check is regedit process is alive");` instead
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1664497233
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1664499364
Re: RFR: 8330954: since-checker - Fix remaining @ since tags in java.base [v7]
On Fri, 28 Jun 2024 11:11:51 GMT, Nizar Benalla wrote:
>> Please review this PR that aims to add all the remaining needed `@since`
>> tags in `java.base`, and group them into a single fix.
>> This is related to #18934 and my work around the `@since` checker feature.
>> Explicit `@since` tags are needed for some overriding methods for the
>> purpose of the checker.
>>
>> I will only give the example with the `CompletableFuture` class but here is
>> the before where the methods only appeared in "Methods declared in interface
>> N"
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac";>
>>
>>
>>
>> and after where the method has it's own javadoc, the main description is
>> added and the `@since` tags are added as intended.
>>
>> I don't have an account on `https://cr.openjdk.org/` but I could host the
>> generated docs somewhere if that is needed.
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043";>
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b";>
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8";>
>>
>>
>> TIA
>
> Nizar Benalla 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 14 additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into 8330954
> - Add new line when having multi-line doc comment
> - Merge remote-tracking branch 'upstream/master' into 8330954
> - Merge remote-tracking branch 'upstream/master' into 8330954
> - (C)
> - (C)
> - Move security classes changes to pr18373
> - remove couple extra lines
> - Pull request is now only about `@since` tags, might add an other commit
> - add one more `{inheritDoc}` to `CompletableFuture.state`
> - ... and 4 more: https://git.openjdk.org/jdk/compare/36d15a13...afca07bf
OK, I look at other areas and I believe they are fine.
-
PR Comment: https://git.openjdk.org/jdk/pull/18954#issuecomment-2206794418
Re: RFR: 8325525: Create jtreg test case for JDK-8325203 [v2]
On Wed, 3 Jul 2024 04:11:40 GMT, Vanitha B P wrote:
>> Created jtreg test case for
>> [JDK-8325203](https://bugs.openjdk.org/browse/JDK-8325203) issue.
>>
>> The JpackageTest created tests that the child process started from the app
>> launched by jpackage launcher is not automatically terminated when the the
>> launcher is terminated.
>
> Vanitha B P has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8325525 Addressed review comments
test/jdk/tools/jpackage/apps/ThirdPartyAppLauncher.java line 34:
> 32: ProcessBuilder processBuilder = new ProcessBuilder("regedit");
> 33: Process process = processBuilder.start();
> 34: logger.info("RegEdit id=" + process.pid());
Logging is excessive and not applicable here as it can be redirected. The test
shall print the PID to stdout.
test/jdk/tools/jpackage/share/JpackageTest.java line 59:
> 57: @Test
> 58: public static void test() throws Throwable {
> 59: JpackageTest test = new JpackageTest();
There is no need to explicitly call the ctor
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1664484472
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1664485737
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v4]
On Tue, 2 Jul 2024 16:24:49 GMT, Nizar Benalla wrote: >> Can I please get a review for this small change? The motivation is that >> javac does not recognize `package.html` files. >> >> The conversion was simple, I used a script to rename the files, append "*" >> on the left and remove some HTML tags like `` and ``. I did the >> conversion in place, renaming them in git but with the big amount of change >> `git` thinks it's a new file. >> >> I also added a new `package-info.java` file to `javax.naming.ldap.spi`. I >> hope that's fine. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > remove two unnecessary tags @nizarbenalla Your change (at version 657ef5c7532bc587cdead80d35486f30bb931d5e) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/19529#issuecomment-2206293817
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v4]
On Tue, 2 Jul 2024 16:24:49 GMT, Nizar Benalla wrote: >> Can I please get a review for this small change? The motivation is that >> javac does not recognize `package.html` files. >> >> The conversion was simple, I used a script to rename the files, append "*" >> on the left and remove some HTML tags like `` and ``. I did the >> conversion in place, renaming them in git but with the big amount of change >> `git` thinks it's a new file. >> >> I also added a new `package-info.java` file to `javax.naming.ldap.spi`. I >> hope that's fine. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > remove two unnecessary tags The latest changes look good to me - Marked as reviewed by aefimov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19529#pullrequestreview-2156608099
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v4]
On Tue, 2 Jul 2024 16:24:49 GMT, Nizar Benalla wrote: >> Can I please get a review for this small change? The motivation is that >> javac does not recognize `package.html` files. >> >> The conversion was simple, I used a script to rename the files, append "*" >> on the left and remove some HTML tags like `` and ``. I did the >> conversion in place, renaming them in git but with the big amount of change >> `git` thinks it's a new file. >> >> I also added a new `package-info.java` file to `javax.naming.ldap.spi`. I >> hope that's fine. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > remove two unnecessary tags Thank you Aleksei for your review! - PR Comment: https://git.openjdk.org/jdk/pull/19529#issuecomment-2206291461
Re: RFR: 8304929: MethodTypeDesc throws an unchecked exception than ReflectiveOperationException when a component class cannot be resolved
On Wed, 3 Jul 2024 13:41:29 GMT, Jorn Vernee wrote:
>> Simple fix for `MethodTypeDescImpl`'s violation of `resolveConstantDesc`
>> specification.
>
> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java
> line 228:
>
>> 226: mtype = mt;
>> 227: } catch (TypeNotPresentException ex) {
>> 228: throw (ClassNotFoundException) ex.getCause();
>
> On a side note, I wonder if it's better to re-wrap the exception here as a
> `ReflectiveOperationException`, instead of just getting the cause. That will
> retain the entire stack trace.
Fyi the exception was thrown at
https://github.com/openjdk/jdk/blob/5a8af2b8b93672de9b3a3e73e6984506980da932/src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java#L95.
I will make builds with both approaches, and see how their traces differ. I
will stay on rethrow only if the original stacktrace is already informative
enough.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19991#discussion_r1664266161
Re: RFR: 8304929: MethodTypeDesc throws an unchecked exception than ReflectiveOperationException when a component class cannot be resolved
On Tue, 2 Jul 2024 16:20:54 GMT, Chen Liang wrote:
> Simple fix for `MethodTypeDescImpl`'s violation of `resolveConstantDesc`
> specification.
src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line
228:
> 226: mtype = mt;
> 227: } catch (TypeNotPresentException ex) {
> 228: throw (ClassNotFoundException) ex.getCause();
On a side note, I wonder if it's better to re-wrap the exception here as a
`ReflectiveOperationException`, instead of just getting the cause. That will
retain the entire stack trace.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19991#discussion_r1664215860
Re: RFR: 8304929: MethodTypeDesc throws an unchecked exception than ReflectiveOperationException when a component class cannot be resolved
On Tue, 2 Jul 2024 16:20:54 GMT, Chen Liang wrote: > Simple fix for `MethodTypeDescImpl`'s violation of `resolveConstantDesc` > specification. Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19991#pullrequestreview-2156417137
Re: RFR: 8304929: MethodTypeDesc throws an unchecked exception than ReflectiveOperationException when a component class cannot be resolved
On Wed, 3 Jul 2024 12:04:37 GMT, Chen Liang wrote: >> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java >> line 226: >> >>> 224: } >>> 225: }); >>> 226: mtype = mt; >> >> Can you drop this intermediate variable, and just assign to `mtype` directly? > > Then I would have to suppress the deprecation warning for the security > manager over the whole method. Is that acceptable? Ah, I see. It's fine like this then. - PR Review Comment: https://git.openjdk.org/jdk/pull/19991#discussion_r1664171625
Re: RFR: 8304929: MethodTypeDesc throws an unchecked exception than ReflectiveOperationException when a component class cannot be resolved
On Wed, 3 Jul 2024 11:57:31 GMT, Jorn Vernee wrote: >> Simple fix for `MethodTypeDescImpl`'s violation of `resolveConstantDesc` >> specification. > > src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java > line 226: > >> 224: } >> 225: }); >> 226: mtype = mt; > > Can you drop this intermediate variable, and just assign to `mtype` directly? Then I would have to suppress the deprecation warning for the security manager over the whole method. Is that acceptable? - PR Review Comment: https://git.openjdk.org/jdk/pull/19991#discussion_r1664081082
Re: RFR: 8304929: MethodTypeDesc throws an unchecked exception than ReflectiveOperationException when a component class cannot be resolved
On Tue, 2 Jul 2024 16:20:54 GMT, Chen Liang wrote: > Simple fix for `MethodTypeDescImpl`'s violation of `resolveConstantDesc` > specification. src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line 226: > 224: } > 225: }); > 226: mtype = mt; Can you drop this intermediate variable, and just assign to `mtype` directly? - PR Review Comment: https://git.openjdk.org/jdk/pull/19991#discussion_r1664072718
Re: RFR: 8328995: Launcher can't open jar files where the offset of the manifest is >4GB [v6]
On Thu, 30 May 2024 20:26:38 GMT, Liam Miller-Cushon wrote: >> Liam Miller-Cushon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Move test to test/jdk/tools/launcher > > @bridgekeeper please keep this open Hello Liam @cushon, please keep this open. Some of us have this on our list to review. - PR Comment: https://git.openjdk.org/jdk/pull/18479#issuecomment-2205652636
Re: RFR: 8301341: LinkedTransferQueue does not respect timeout for poll() [v10]
On Fri, 21 Jul 2023 16:42:25 GMT, Doug Lea wrote: >> This update addresses performance issues across both LinkedTransferQueue and >> SynchronousQueue by creating a common basis for implementation across them >> (mainly in LinkedTransferQueue). Pasting from internal doc summary of >> changes: >> * * Class DualNode replaces Qnode, with fields and methods >> * that apply to any match-based dual data structure, and now >> * usable in other j.u.c classes. in particular, SynchronousQueue. >> * * Blocking control (in class DualNode) accommodates >> * VirtualThreads and (perhaps virtualized) uniprocessors. >> * * All fields of this class (LinkedTransferQueue) are >> * default-initializable (to null), allowing further extension >> * (in particular, SynchronousQueue.Transferer) >> * * Head and tail fields are lazily initialized rather than set >> * to a dummy node, while also reducing retries under heavy >> * contention and misorderings, and relaxing some accesses, >> * requiring accommodation in many places (as well as >> * adjustments in WhiteBox tests). > > Doug Lea has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 13 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8301341 > - Address more review comments > - Address review comments > - nitpicks > - Merge branch 'openjdk:master' into JDK-8301341 > - Accommodate white-box tests; use consistent constructions; minor > improvements > - Merge branch 'openjdk:master' into JDK-8301341 > - Simplify contention handling; fix test > - Fix inverted test assert; improve internal documentation; simplify code > - Merge branch 'openjdk:master' into JDK-8301341 > - ... and 3 more: https://git.openjdk.org/jdk/compare/633a84d0...f53cee67 Just my 2 cents—given the size of this change, I'd be hesitant to have it backported. Also, more than this PR would need to be backported, for instance: https://github.com/openjdk/jdk/pull/19271 - PR Comment: https://git.openjdk.org/jdk/pull/14317#issuecomment-2205567436
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException
On Fri, 10 May 2024 01:24:15 GMT, Chen Liang wrote: >> This change overrides mutator methods in the implementation returned by >> `Map.of().entrySet()` to throw `UnsupportedOperationException`. > > @stuart-marks Can you take a look at this? I've had a look here, and it functionally looks good to me. The remaining challenge is the impact of the change on existing code (likely written between 11 and now, as <10 code would not have the problem AFAICT). /cc @liach - PR Comment: https://git.openjdk.org/jdk/pull/18522#issuecomment-2205561866
