Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. I don't think that was the intention. There's a comment on `Method::_code` stating that the pointer can change at any point (so the pointer was supposed to be volatile), and `class CompiledMethod` does not have any `volatile` - qualified methods. There are similar cases where `const` pointer to a class was returned involving `JavaThread`, `ReferenceProcessor`, `PSCardTable` and `ShenandoahHeapRegion`; I suppose we could review if `const` could be added to the returned class, but that's outside of the scope of this PR. - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1612435102
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v2]
On Wed, 28 Jun 2023 07:37:00 GMT, Serguei Spitsyn wrote:
> The test is great. I realize it is not easy to make it fully reliable though.
> So, will need another pass.
Good point about spurious wakeups.
Updated the test to handle them.
Now for WAITING scenarios the test suspends test thread and ensures it's
waiting. While the thread is suspended (either carrier or virtual) we can
safely verify thread states
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
> line 117:
>
>> 115: } catch (InterruptedException ex) {
>> 116: // expected, ignore
>> 117: }
>
> Shout this code account for spurious wakeups?
Fixed
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
> line 241:
>
>> 239: private static native void waitInNative();
>> 240: // Signals waitInNative() to exit.
>> 241: private static native void endWait();
>
> Q: Where is this native method invoked?
Good catch. Test thread inNative testcase never exits, it's terminated on the
test exit.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
> line 73:
>
>> 71: jint unexpected = state - actual_full;
>> 72: LOG(" ERROR: some unexpected bits are set (%x): %s\n",
>> 73:unexpected, TranslateState(unexpected));
>
> Nit: lines 65, 73 have wrong indent.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
> line 176:
>
>> 174: while (!time_to_exit) {
>> 175: sleep_ms(100);
>> 176: }
>
> Is this loop reliable in terms of thread states?
The code is pure native (sleep_ms uses native functions, it does not interact
with JVM), so there is no thread state changes
-
PR Comment: https://git.openjdk.org/jdk/pull/14622#issuecomment-1612423518
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246097094
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246096797
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246098484
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246098388
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v3]
> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier > should return STATE_WAITING) > New test tests GetThreadState for different thread states. > The test detected a bug in the implementation, new issue is created: > JDK-8310584 > Corresponding testcases are commented now in the test, fix for JDK-8310584 > will uncomment them Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: spurious wakeups - Changes: - all: https://git.openjdk.org/jdk/pull/14622/files - new: https://git.openjdk.org/jdk/pull/14622/files/b5b39bcd..6c517cef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14622&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14622&range=01-02 Stats: 124 lines in 2 files changed: 88 ins; 0 del; 36 mod Patch: https://git.openjdk.org/jdk/pull/14622.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14622/head:pull/14622 PR: https://git.openjdk.org/jdk/pull/14622
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
On Wed, 28 Jun 2023 21:21:11 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > [skip ci] handle pending HotSpot exception closer to site causing exception This is now isolated to JVMCI code that I'm not familiar with, so don't feel competent to review. The general idea seems okay but I'm not familiar enough with the details. Someone from the compiler team should review this now. Thanks. - PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1504459576
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
On Wed, 28 Jun 2023 21:21:11 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > [skip ci] handle pending HotSpot exception closer to site causing exception That's more clear to me. Thanks! - Marked as reviewed by never (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1504385398
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Wed, 28 Jun 2023 18:28:37 GMT, Tom Rodriguez wrote: > Why can't the pending exception handling be in the respective virtual? Done: 9236128ad1b7297c8c4e9d3022b88c3ab3278022 - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1612113760
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: [skip ci] handle pending HotSpot exception closer to site causing exception - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/734903a8..9236128a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=03-04 Stats: 145 lines in 3 files changed: 102 ins; 25 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. In the example, was the intention perhaps `volatile CompiledMethod*` instead of `CompiledMethod* volatile`? - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1612090389
Integrated: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Tue, 27 Jun 2023 12:41:50 GMT, Coleen Phillimore wrote: > This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. This pull request has now been integrated. Changeset: 9f46fc28 Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/9f46fc28426630399ca39d443403cc3a7be58854 Stats: 60 lines in 32 files changed: 8 ins; 0 del; 52 mod 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. Reviewed-by: iklam, fparain - PR: https://git.openjdk.org/jdk/pull/14675
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 16:15:39 GMT, Ioi Lam wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use pointer_delta_as_int for the name that uses pointer_delta, fix
>> negative case to just do checked_cast.
>
> src/hotspot/share/utilities/globalDefinitions.hpp line 529:
>
>> 527: template
>> 528: inline int pointer_delta_as_int(const volatile T* left, const volatile
>> T* right) {
>> 529: return checked_cast(pointer_delta(left, right, sizeof(T)));
>
> For clarity, I think you should add a comment saying the returned value is
> always non-negative.
done, thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1245681394
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 13:17:21 GMT, Coleen Phillimore wrote: >> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but >> I've added a pointer delta function in globalDefinitions.hpp to use for >> these pointer diff calculations that return int everywhere. If the name is >> agreeable, I'll fix the other cases of this like this. It's better than raw >> casts. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Use pointer_delta_as_int for the name that uses pointer_delta, fix negative > case to just do checked_cast. Thanks Ioi and Fred and Stefan for suggestions. - PR Comment: https://git.openjdk.org/jdk/pull/14675#issuecomment-1612003549
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v5]
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Clarify that pointer_delta_as_int() returns a non-negative int. - Changes: - all: https://git.openjdk.org/jdk/pull/14675/files - new: https://git.openjdk.org/jdk/pull/14675/files/9fed8c89..856cafd4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. David: I think this part of the spec is relevant here: > A non-class non-array prvalue cannot be > [cv-qualified](https://en.cppreference.com/w/cpp/language/cv), [...]. (Note: > a function call or cast expression may result in a prvalue of non-class > cv-qualified type, but the cv-qualifier is generally immediately stripped > out.) [source](https://en.cppreference.com/w/cpp/language/value_category) given that the cv qualifiers are immediately stripped by the compiler, there's no point in providing them. In the particular volatile pointer case: the function performs a volatile read to get the pointer value (address). That address can then be used in a non-volatile manner. Kim: I realize that it's a big change, so thank you very much for reviewing it anyway! I was prepared to split it up, just wanted to know if this warning is something we want to fix. - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1611905364
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 21:29:53 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > revert to lazy loading of VMSupport Right, I was forgetting about the virtual nature of this code. Why can't the pending exception handling be in the respective virtual? The partition between virtual and shared is kind of ugly. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1611888908
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 13:17:21 GMT, Coleen Phillimore wrote: >> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but >> I've added a pointer delta function in globalDefinitions.hpp to use for >> these pointer diff calculations that return int everywhere. If the name is >> agreeable, I'll fix the other cases of this like this. It's better than raw >> casts. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Use pointer_delta_as_int for the name that uses pointer_delta, fix negative > case to just do checked_cast. Looks good to me. - Marked as reviewed by fparain (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14675#pullrequestreview-1503631614
RFR: 8311043: Remove trailing blank lines in source files
Remove trailing "blank" lines in source files. I like to use global-whitespace-cleanup-mode, but I can not use it if the files are "dirty" to begin with. This fix will make more files "clean". I also considered adding a check for this in jcheck for Skara, however it seems jcheck code handling hunks does not track end-of-files. Thus I will only clean the files. The fix removes trailing lines matching ^[[:space:]]*$ in - *.java - *.cpp - *.hpp - *.c - *.h I have applied the following bash script to each file: file="$1" while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do truncate -s -1 "$file" done `git diff --ignore-space-change --ignore-blank-lines master` displays no changes `git diff --ignore-blank-lines master` displays one change - Commit messages: - h - c - hpp - cpp - 8311043: Remove trailing blank lines in source files Changes: https://git.openjdk.org/jdk/pull/14698/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14698&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311043 Stats: 4529 lines in 4382 files changed: 0 ins; 4529 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14698.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14698/head:pull/14698 PR: https://git.openjdk.org/jdk/pull/14698
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 13:17:21 GMT, Coleen Phillimore wrote:
>> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but
>> I've added a pointer delta function in globalDefinitions.hpp to use for
>> these pointer diff calculations that return int everywhere. If the name is
>> agreeable, I'll fix the other cases of this like this. It's better than raw
>> casts.
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use pointer_delta_as_int for the name that uses pointer_delta, fix negative
> case to just do checked_cast.
LGTM.
src/hotspot/share/utilities/globalDefinitions.hpp line 529:
> 527: template
> 528: inline int pointer_delta_as_int(const volatile T* left, const volatile
> T* right) {
> 529: return checked_cast(pointer_delta(left, right, sizeof(T)));
For clarity, I think you should add a comment saying the returned value is
always non-negative.
-
Marked as reviewed by iklam (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14675#pullrequestreview-1503498625
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1245463745
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. Looks good. Though mind-numbingly large; breaking something like this up into easier to digest chunks can make reviewing easier. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14674#pullrequestreview-1503384263
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 28 Jun 2023 13:16:30 GMT, Alan Bateman wrote:
>> Hi Alan, regarding usage of class VM I get
>> 'package jdk.internal.misc is declared in module java.base, which does not
>> export it to module java.sql'
>> Is there any concern to export it as well to module java.sql ?
>> And btw did you mean to use it like this, in the if ?
>>
>> `
>> if (callerCL == null || VM.isSystemDomainLoader(callerCL)) {
>> callerCL = Thread.currentThread().getContextClassLoader();
>> }
>> `
>
>> Hi Alan, regarding usage of class VM I get 'package jdk.internal.misc is
>> declared in module java.base, which does not export it to module java.sql'
>> Is there any concern to export it as well to module java.sql ? And btw did
>> you mean to use it like this, in the if ?
>>
>> `if (callerCL == null || VM.isSystemDomainLoader(callerCL)) { callerCL =
>> Thread.currentThread().getContextClassLoader(); }`
>
> It was just a passing comment, I didn't meant to suggest changing it as part
> of this PR. We have always think twice before adding qualified exports from
> java.base and this is case where java.sql is very "non-core", we don't want
> to give it any access to java.base internals.
Hi Alan, thanks for clarifying. So I should only adjust the comment, correct ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1245204791
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 28 Jun 2023 12:54:10 GMT, Matthias Baesken wrote:
> Hi Alan, regarding usage of class VM I get 'package jdk.internal.misc is
> declared in module java.base, which does not export it to module java.sql' Is
> there any concern to export it as well to module java.sql ? And btw did you
> mean to use it like this, in the if ?
>
> `if (callerCL == null || VM.isSystemDomainLoader(callerCL)) { callerCL =
> Thread.currentThread().getContextClassLoader(); }`
It was just a passing comment, I didn't meant to suggest changing it as part of
this PR. We have always think twice before adding qualified exports from
java.base and this is case where java.sql is very "non-core", we don't want to
give it any access to java.base internals.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1245197137
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Use pointer_delta_as_int for the name that uses pointer_delta, fix negative case to just do checked_cast. - Changes: - all: https://git.openjdk.org/jdk/pull/14675/files - new: https://git.openjdk.org/jdk/pull/14675/files/858e9cfb..9fed8c89 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=02-03 Stats: 22 lines in 14 files changed: 0 ins; 1 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Thu, 22 Jun 2023 14:20:30 GMT, Alan Bateman wrote:
>> There are a few references to rt.jar in comments and in the codebase itself.
>> Some of them might be removed or adjusted.
>
> src/java.sql/share/classes/java/sql/DriverManager.java line 658:
>
>> 656: * (which is invoking this class indirectly)
>> 657: * classloader, so that the JDBC driver class outside the image
>> 658: * can be loaded from here.
>
> This code should probably be changed to use VM.isSystemDomainLoader(callerCL).
>
> I think the comment should be replaced because it doesn't match what it
> actually does and it's nothing to do with the whether the JDBC driver is in
> the run-time image or not. How about:
>
> "If the caller is defined to the bootstrap or platform class loader then use
> the Thread CCL as the initiating class loader so that a JDBC on the class
> path, or bundled with an application, is found."
Hi Alan, regarding usage of class VM I get
'package jdk.internal.misc is declared in module java.base, which does not
export it to module java.sql'
Is there any concern to export it as well to module java.sql ?
And btw did you mean to use it like this, in the if ?
`if (callerCL == null || VM.isSystemDomainLoader(callerCL)) {
callerCL = Thread.currentThread().getContextClassLoader();
}`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1245164604
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v3]
On Wed, 28 Jun 2023 07:52:35 GMT, Stefan Karlsson wrote: >> Taking out that cast does work, so I've fixed that. > >> pointer_delta has different semantics > > Right. That was "recently" added to pointer_delta with JDK-8260046. It begs > the question why felt the need to add it there but feel that it is OK to skip > it for delta_as_int? Is there some usage of delta_as_int that gives back > negative values? Could that call site be changed? There are sites where the result is negative but this is a good suggestion because it makes the name more consistent. I can change those to plain check_casts. - PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1245141370
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > warning: 'volatile' type qualifier on return type has no effect Can't say that I understand that warning. If I pass in a volatile pointer and return the same pointer then I would not expect to lose the volatile aspect of it. @kbarrett can you explain this one to me? - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1611299893
[jdk21] Integrated: 8303916: ThreadLists.java inconsistent results
On Fri, 16 Jun 2023 09:15:52 GMT, Kevin Walls wrote: > Clean backport from latest jdk repo to jdk21 for the test change: > 8303916: ThreadLists.java inconsistent results This pull request has now been integrated. Changeset: 8d9ebb69 Author:Kevin Walls URL: https://git.openjdk.org/jdk21/commit/8d9ebb6981a163a7c223eff154eb5ad966e146f1 Stats: 77 lines in 1 file changed: 54 ins; 1 del; 22 mod 8303916: ThreadLists.java inconsistent results Reviewed-by: sspitsyn Backport-of: 8c9b85a990d955487f9141207cc83d0051defc57 - PR: https://git.openjdk.org/jdk21/pull/24
Re: RFR: JDK-8306441: Two-stage Segmented Heap Dump [v5]
On Tue, 16 May 2023 18:41:26 GMT, Chris Plummer wrote: >> Hi, can I have a review for this patch? > > @y1yang0 Sorry no one has been able to review this so far. The serviceability > team is very busy for the next few weeks finishing up JDK 21 changes before > RDP1. It's unlikely we'll find time for the review before them. > > I did take a very quick look at the changes just to understand the scope. One > thing I noticed that makes this PR hard to review is the code refactoring and > relocating that you did. At first it looks like a lot of old code was deleted > and a lot of new code added, but in fact most of the new code is just > relocated old code. It makes it very hard to tell if there have been any > changes to this code. Is there anything you can do to lessen the amount of > apparent new code that is actually just moved code? Hi @plummercj @kevinjwalls @alexmenkov do you have any plans to review this patch recently? I have addressed some bugs and it passed manual tests& all jtreg tests. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/13667#issuecomment-1610946603
Re: RFR: JDK-8306441: Two-stage Segmented Heap Dump [v14]
> ### Motivation and proposal > Hi, heap dump brings about pauses for application's execution(STW), this is a > well-known pain. JDK-8252842 have added parallel support to heapdump in an > attempt to alleviate this issue. However, all concurrent threads > competitively write heap data to the same file, and more memory is required > to maintain the concurrent buffer queue. In experiments, we did not feel a > significant performance improvement from that. > > The minor-pause solution, which is presented in this PR, is a two-stage > segmented heap dump: > > 1. Stage One(STW): Concurrent threads directly write data to multiple heap > files. > 2. Stage Two(Non-STW): Merge multiple heap files into one complete heap dump > file. > > Now concurrent worker threads are not required to maintain a buffer queue, > which would result in more memory overhead, nor do they need to compete for > locks. The changes in the overall design are as follows: > >  > Fig1. Before > >  > Fig1. After this patch > > ### Performance evaluation > | memory | numOfThread | STW | Total | Compression | > | --- | - | -- | | | > | 8g | 1 thread | 15.612 secs | 15.612 secs | N | > | 8g | 32 thread | 2.5617250 secs | 14.498 secs | N | > | 8g | 32 thread | 2.3084878 secs | 3.198 secs | Compress1 | > | 8g | 32 thread | 10.9355128 secs | 21.882 secs | Compress2 | > | 8g | 96 thread | 2.6790452 secs | 14.012 secs | N | > | 8g | 96 thread | 2.3044796 secs | 3.589 secs | Compress1 | > | 8g | 96 thread | 9.7585151 secs | 20.219 secs| Compress2 | > | 16g | 1 thread | 26.278 secs | 26.278 secs | N | > | 16g | 32 thread | 5.2313740 secs | 26.417 secs | N | > | 16g | 32 thread | 5.6946983 secs | 6.538 secs | Compress1 | > | 16g | 32 thread | 21.8211105 secs | 41.133 secs | Compress2 | > | 16g | 96 thread | 6.2445556 secs | 27.141 secs | N | > | 16g | 96 thread | 4.6007096 secs | 6.259 secs | Compress1 | > | 16g | 96 thread | 19.2965783 secs | 39.007 secs | Compress2 | > | 32g | 1 thread | 48.149 secs | 48.149 secs | N | > | 32g | 32 thread | 10.7734677 secs | 61.643 secs | N | > | 32g | 32 thread | 10.1642097 secs | 10.903 secs | Compress1 | > | 32g | 32 thread | 43.8407607 secs | 88.152 secs | Compress2 | > | 32g | 96 thread | 13.1522042 secs | 61.432 secs | N | > | 32g | 96 thread | 9.0954641 secs | 9.885 secs | C... Yi Yang has updated the pull request incrementally with one additional commit since the last revision: memory leak - Changes: - all: https://git.openjdk.org/jdk/pull/13667/files - new: https://git.openjdk.org/jdk/pull/13667/files/ecab3116..2012b5ca Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13667&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13667&range=12-13 Stats: 52 lines in 1 file changed: 22 ins; 13 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/13667.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13667/head:pull/13667 PR: https://git.openjdk.org/jdk/pull/13667
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v3]
On Tue, 27 Jun 2023 15:11:34 GMT, Coleen Phillimore wrote: >> Hm. Maybe this would be ok. Our original idea was to make it T* not T until >> this cast. I don't know how many other cases there are that I haven't >> gotten to yet. But it would eliminate a cast, so that's good (unless these >> aren't the same). Some instances have ptr - constant that gets promoted I >> think. >> The reason we didn't pick pointer_delta_as_int because pointer_delta has >> different semantics. pointer_delta insists on positive results. > > Taking out that cast does work, so I've fixed that. > pointer_delta has different semantics Right. That was "recently" added to pointer_delta with JDK-8260046. It begs the question why felt the need to add it there but feel that it is OK to skip it for delta_as_int? Is there some usage of delta_as_int that gives back negative values? Could that call site be changed? - PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1244829942
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v2]
On Tue, 27 Jun 2023 20:54:27 GMT, Alex Menkov wrote:
>> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier
>> should return STATE_WAITING)
>> New test tests GetThreadState for different thread states.
>> The test detected a bug in the implementation, new issue is created:
>> JDK-8310584
>> Corresponding testcases are commented now in the test, fix for JDK-8310584
>> will uncomment them
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added spaces in synchronized
The test is great.
I realize it is not easy to make it fully reliable though.
So, will need another pass.
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
line 117:
> 115: } catch (InterruptedException ex) {
> 116: // expected, ignore
> 117: }
Shout this code account for spurious wakeups?
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
line 241:
> 239: private static native void waitInNative();
> 240: // Signals waitInNative() to exit.
> 241: private static native void endWait();
Q: Where is this native method invoked?
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
line 73:
> 71: jint unexpected = state - actual_full;
> 72: LOG(" ERROR: some unexpected bits are set (%x): %s\n",
> 73:unexpected, TranslateState(unexpected));
Nit: lines 65, 73 have wrong indent.
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
line 176:
> 174: while (!time_to_exit) {
> 175: sleep_ms(100);
> 176: }
Is this loop reliable in terms of thread states?
-
PR Review: https://git.openjdk.org/jdk/pull/14622#pullrequestreview-1502440716
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244797022
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244791733
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244809438
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244799190
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 23:06:49 GMT, Tom Rodriguez wrote: > I don't think pushing it down that far improves things. encode always > precedes decode so why not resolve it before the encode call. Because the `VMSupport` klass is only needed if calling into HotSpot so it's better to push it down into `HotSpotToSharedLibraryExceptionTranslation::encode`. Also, if the `VMSupport` klass is used for encoding, it's *not* needed for decoding (the libgraal `VMSupport` jclass value is used instead). - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1610903263
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 23:08:22 GMT, Tom Rodriguez wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert to lazy loading of VMSupport
>
> src/hotspot/share/jvmci/jvmciEnv.cpp line 402:
>
>> 400: }
>> 401: int res = encode(THREAD, buffer, buffer_size);
>> 402: if (_from_env != nullptr && !_from_env->is_hotspot() &&
>> _from_env->has_pending_exception()) {
>
> Why is this check before the `HAS_PENDING_EXCEPTION` check? Couldn't you end
> up returning with a pending exception in this path?
If `encode` is `SharedLibraryToHotSpotExceptionTranslation::encode` there is no
possibility of a pending HotSpot exception upon it returning. If it's
`HotSpotToSharedLibraryExceptionTranslation::encode` then
`_from_env->is_hotspot()` is `true` and so this `if` branch is not taken.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1244794019
Re: RFR: 8294977: Convert test/jdk/java tests from ASM library to Classfile API [v9]
> Summaries: > 1. A few recommendations about updating the constant API is made at > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000233.html > and I may update this patch shall the API changes be integrated before > 2. One ASM library-specific test, `LambdaAsm` is removed. Others have their > code generation infrastructure upgraded from ASM to Classfile API. > 3. Most tests are included in tier1, but some are not: > In `:jdk_io`: (tier2, part 2) > > test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java > test/jdk/java/io/Serializable/records/ProhibitedMethods.java > test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java > > In `:jdk_instrument`: (tier 3) > > test/jdk/java/lang/instrument/RetransformAgent.java > test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java > test/jdk/java/lang/instrument/asmlib/Instrumentor.java > > > @asotona Would you mind reviewing? Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - Classfile object update - Merge branch 'master' into invoke-test-classfile - Merge branch 'master' into invoke-test-classfile - Switch to ConstantDescs for and void constants - Merge AnnotationsTest, remove ModuleTargetAttribute call - Merge branch 'invoke-test-classfile' of https://github.com/liachmodded/jdk into invoke-test-classfile - Update test/jdk/java/lang/invoke/8022701/MHIllegalAccess.java Co-authored-by: Andrey Turbanov - Merge branch 'master' into invoke-test-classfile - Fix LambdaStackTrace after running - formatting - ... and 4 more: https://git.openjdk.org/jdk/compare/526dba1a...d0b6c2ae - Changes: https://git.openjdk.org/jdk/pull/13009/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13009&range=08 Stats: 1954 lines in 31 files changed: 307 ins; 883 del; 764 mod Patch: https://git.openjdk.org/jdk/pull/13009.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13009/head:pull/13009 PR: https://git.openjdk.org/jdk/pull/13009
