Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v28]
On Mon, 4 Nov 2024 02:12:40 GMT, David Holmes wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use lazySubmitRunContinuation when blocking
>
> src/hotspot/share/classfile/javaClasses.cpp line 2107:
>
>> 2105:
>> 2106: jlong java_lang_VirtualThread::waitTimeout(oop vthread) {
>> 2107: return vthread->long_field(_timeout_offset);
>
> Not sure what motivated the name change but it seems odd to have the method
> named differently to the field it accesses. ??
It was initially parkTimeout and waitTimeout but it doesn't require two fields
as you can't be waiting in Object.wait(timeout) and LockSupport.parkNanos at
the same time. So the field was renamed, the accessors here should probably be
renamed too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1827219720
Re: RFR: 8343497: Missing DEF_STATIC_JNI_OnLoad in libjimage and libsaproc native libraries [v2]
> Please review this trivial fix that adds missing DEF_STATIC_JNI_OnLoad in > libjimage and libsaproc native libraries. Thanks. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Fix macos build issue. - Changes: - all: https://git.openjdk.org/jdk/pull/21861/files - new: https://git.openjdk.org/jdk/pull/21861/files/0744bd82..5a948674 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21861&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21861&range=00-01 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21861.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21861/head:pull/21861 PR: https://git.openjdk.org/jdk/pull/21861
Re: RFR: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking [v7]
> This is an implementation of [JEP 483: Ahead-of-Time Class Loading & > Linking](https://openjdk.org/jeps/483). > > > Note: this is a combined PR of the following individual PRs > - https://github.com/openjdk/jdk/pull/20516 > - https://github.com/openjdk/jdk/pull/20517 > - https://github.com/openjdk/jdk/pull/20843 > - https://github.com/openjdk/jdk/pull/20958 > - https://github.com/openjdk/jdk/pull/20959 > - https://github.com/openjdk/jdk/pull/21049 > - https://github.com/openjdk/jdk/pull/21143 Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 172 commits: - fixed merge - Merge branch 'master' into jep-483-candidate - 8343493: Perform module checks during MetaspaceShared::map_archives() - reverted changes in modules.cpp to make it easy to merge with mainline - Fixed whitespace; fixed minimal build - Fixed 8343245: AOT cache creation crashes with "assert(HeapShared::is_archivable_hidden_klass(ik)) failed: sanity" - fixed comments - @ashu-mehra comment - renamed function to SystemDictionaryShared::should_be_excluded(); added comments and asserts; make sure the class is indeed checked - Backed out 58233cc20fa22abfd711bb59aafb78e20fabc195 - @ashu-mehra comment - rename/comment AOTClassLinker::add_new_candidate() and added asserts for thread safety - ... and 162 more: https://git.openjdk.org/jdk/compare/29882bfe...935dcc68 - Changes: https://git.openjdk.org/jdk/pull/21642/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21642&range=06 Stats: 7256 lines in 105 files changed: 6408 ins; 519 del; 329 mod Patch: https://git.openjdk.org/jdk/pull/21642.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21642/head:pull/21642 PR: https://git.openjdk.org/jdk/pull/21642
Re: RFR: 8304824: NMT should not use ThreadCritical [v9]
On Fri, 1 Nov 2024 21:56:36 GMT, Markus Grönlund wrote: >This include is not needed because there are no uses that require the >definition of Thread. Right, seems like the forward declaration used to be provided by `memory/allocation.hpp`. Let's get rid of the include and use a forward declaration of our own instead. - PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1827311285
Re: RFR: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking [v6]
On Fri, 1 Nov 2024 03:54:24 GMT, Dan Heidinga wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed whitespace; fixed minimal build > > src/hotspot/share/cds/aotClassInitializer.cpp line 348: > >> 346: } >> 347: JavaValue result(T_VOID); >> 348: JavaCalls::call_static(&result, ik, > > Based on the discussions in JDK-8342283, do we need a memory fence after the > call to runtimeSetup? > > I think we do to be consistent with the non-AOTCache run as there is a fence > after `` (when this code would normally run) due to the > initialization lock. I think this is already done: `runtimeSetup()` is called inside `InstanceKlass::call_class_initializer()`. When that returns, we will proceed to `InstanceKlass::set_initialization_state_and_notify()` which will perform the memory fencing. https://github.com/iklam/jdk/blob/6eebd18fc2820ffb179d431f179fc6af6d1be247/src/hotspot/share/oops/instanceKlass.cpp#L1276-L1300 - PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1827146321
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v28]
On Fri, 1 Nov 2024 19:37:14 GMT, Patricio Chilano Mateo
wrote:
>> This is the implementation of JEP 491: Synchronize Virtual Threads without
>> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for
>> further details.
>>
>> In order to make the code review easier the changes have been split into the
>> following initial 4 commits:
>>
>> - Changes to allow unmounting a virtual thread that is currently holding
>> monitors.
>> - Changes to allow unmounting a virtual thread blocked on synchronized
>> trying to acquire the monitor.
>> - Changes to allow unmounting a virtual thread blocked in `Object.wait()`
>> and its timed-wait variants.
>> - Changes to tests, JFR pinned event, and other changes in the JDK libraries.
>>
>> The changes fix pinning issues for all 4 ports that currently implement
>> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added
>> recently and stand in its own commit after the initial ones.
>>
>> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default
>> locking mode, (and `LM_MONITOR` which comes for free), but not when using
>> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been
>> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)),
>> with the intention to remove `LM_LEGACY` code in future releases.
>>
>>
>> ## Summary of changes
>>
>> ### Unmount virtual thread while holding monitors
>>
>> As stated in the JEP, currently when a virtual thread enters a synchronized
>> method or block, the JVM records the virtual thread's carrier platform
>> thread as holding the monitor, not the virtual thread itself. This prevents
>> the virtual thread from being unmounted from its carrier, as ownership
>> information would otherwise go wrong. In order to fix this limitation we
>> will do two things:
>>
>> - We copy the oops stored in the LockStack of the carrier to the stackChunk
>> when freezing (and clear the LockStack). We copy the oops back to the
>> LockStack of the next carrier when thawing for the first time (and clear
>> them from the stackChunk). Note that we currently assume carriers don't hold
>> monitors while mounting virtual threads.
>>
>> - For inflated monitors we now record the `java.lang.Thread.tid` of the
>> owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This
>> allows us to tie the owner of the monitor to a `java.lang.Thread` instance,
>> rather than to a JavaThread which is only created per platform thread. The
>> tid is already a 64 bit field so we can ignore issues of the counter
>> wrapping around.
>>
>> General notes about this part:
>>
>> - Since virtual th...
>
> Patricio Chilano Mateo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use lazySubmitRunContinuation when blocking
src/hotspot/share/classfile/javaClasses.cpp line 2107:
> 2105:
> 2106: jlong java_lang_VirtualThread::waitTimeout(oop vthread) {
> 2107: return vthread->long_field(_timeout_offset);
Not sure what motivated the name change but it seems odd to have the method
named differently to the field it accesses. ??
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1827128518
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v17]
On Fri, 1 Nov 2024 16:04:55 GMT, Magnus Ihse Bursie wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove superfluous check for 64-bit on Windows in > MacroAssembler::call_clobbered_xmm_registers > - Remove windows-32-bit code in CompilerConfig::ergo_initialize Changes requested by dholmes (Reviewer). src/hotspot/share/adlc/adlc.hpp line 43: > 41: > 42: /* Make sure that we have the intptr_t and uintptr_t definitions */ > 43: #ifdef _WIN32 As this is a synonym for `_WINDOWS` it is not obvious this deletion is correct. - PR Review: https://git.openjdk.org/jdk/pull/21744#pullrequestreview-2412031267 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1827135809
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v28]
On Fri, 1 Nov 2024 19:37:14 GMT, Patricio Chilano Mateo wrote: >> This is the implementation of JEP 491: Synchronize Virtual Threads without >> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for >> further details. >> >> In order to make the code review easier the changes have been split into the >> following initial 4 commits: >> >> - Changes to allow unmounting a virtual thread that is currently holding >> monitors. >> - Changes to allow unmounting a virtual thread blocked on synchronized >> trying to acquire the monitor. >> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` >> and its timed-wait variants. >> - Changes to tests, JFR pinned event, and other changes in the JDK libraries. >> >> The changes fix pinning issues for all 4 ports that currently implement >> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added >> recently and stand in its own commit after the initial ones. >> >> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default >> locking mode, (and `LM_MONITOR` which comes for free), but not when using >> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been >> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), >> with the intention to remove `LM_LEGACY` code in future releases. >> >> >> ## Summary of changes >> >> ### Unmount virtual thread while holding monitors >> >> As stated in the JEP, currently when a virtual thread enters a synchronized >> method or block, the JVM records the virtual thread's carrier platform >> thread as holding the monitor, not the virtual thread itself. This prevents >> the virtual thread from being unmounted from its carrier, as ownership >> information would otherwise go wrong. In order to fix this limitation we >> will do two things: >> >> - We copy the oops stored in the LockStack of the carrier to the stackChunk >> when freezing (and clear the LockStack). We copy the oops back to the >> LockStack of the next carrier when thawing for the first time (and clear >> them from the stackChunk). Note that we currently assume carriers don't hold >> monitors while mounting virtual threads. >> >> - For inflated monitors we now record the `java.lang.Thread.tid` of the >> owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This >> allows us to tie the owner of the monitor to a `java.lang.Thread` instance, >> rather than to a JavaThread which is only created per platform thread. The >> tid is already a 64 bit field so we can ignore issues of the counter >> wrapping around. >> >> General notes about this part: >> >> - Since virtual th... > > Patricio Chilano Mateo has updated the pull request incrementally with one > additional commit since the last revision: > > Use lazySubmitRunContinuation when blocking src/java.base/share/classes/jdk/internal/vm/Continuation.java line 62: > 60: NATIVE(2, "Native frame or on stack"), > 61: MONITOR(3, "Monitor held"), > 62: CRITICAL_SECTION(4, "In critical section"); Is there a reason that the `reasonCode` values does not match the `freeze_result` reason values used in `pinnedReason(int reason)` to create one of these? I cannot see that it is used either. Only seem to be read for JFR VirtualThreadPinned Event which only uses the string. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1827276764
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage [v4]
On Thu, 31 Oct 2024 07:18:35 GMT, Julian Waters wrote: >> After 8339120, gcc began catching many different instances of unused code in >> the Windows specific codebase. Some of these seem to be bugs. I've taken the >> effort to mark out all the relevant globals and locals that trigger the >> unused warnings and addressed all of them by commenting out the code as >> appropriate. I am confident that in many cases this simplistic approach of >> commenting out code does not fix the underlying issue, and the warning >> actually found a bug that should be fixed. In these instances, I will be >> aiming to fix these bugs with help from reviewers, so I recommend anyone >> reviewing who knows more about the code than I do to see whether there is >> indeed a bug that needs fixing in a different way than what I did > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Remove global memHandle, would've liked to keep it as a comment :( @alexeysemenyukoracle Sorry for the page, needed a reviewer for jpackage. Is jpackage good to go? - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2453881416
RFR: 8339134: Callers of Exceptions::fthrow should ensure exception message lengths avoid the INT_MAX limits of os::vsnprintf
This is mostly an audit of the callers of `Exceptions::fthrow` to ensure unbounded strings can't appear. There is a code change in DiagnosticCmd parsing to extend the string length limit already used in part of that code. Testing: - tier 1-3 (sanity) Thanks - Commit messages: - 8339134: Callers of Exceptions::fthrow should ensure exception message lengths avoid the INT_MAX limits of os::vsnprintf Changes: https://git.openjdk.org/jdk/pull/21867/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21867&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8339134 Stats: 50 lines in 9 files changed: 44 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/21867.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21867/head:pull/21867 PR: https://git.openjdk.org/jdk/pull/21867
RFR: 8343497: Missing DEF_STATIC_JNI_OnLoad in libjimage and libsaproc native libraries
Please review this trivial fix that adds missing DEF_STATIC_JNI_OnLoad in libjimage and libsaproc native libraries. Thanks. - Commit messages: - Add DEF_STATIC_JNI_OnLoad to libjimage and libsaproc native libraries.Q Changes: https://git.openjdk.org/jdk/pull/21861/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21861&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8343497 Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/21861.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21861/head:pull/21861 PR: https://git.openjdk.org/jdk/pull/21861
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v6]
On Sun, 3 Nov 2024 11:25:13 GMT, Daniel Fuchs wrote:
>> src/java.base/share/classes/java/lang/System.java line 1364:
>>
>>> 1362: *
>>> 1363: * It is the responsibility of the provider of
>>> 1364: * the concrete {@code LoggerFinder} implementation to ensure that
>>
>> This is still a part of the paragraph related to the security manager.
>
> Right - this paragraph - lines 1620-1625 (old file) / 1362-1367 (new file) is
> no longer relevant and should be removed too. Thanks for spotting that.
Removed in jep486 branch in sandbox so will get picked up when PR is refreshed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1826972198
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v6]
On Sat, 2 Nov 2024 22:25:06 GMT, ExE Boss wrote:
>> Sean Mullan has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 200 commits:
>>
>> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
>> - Modify three RMI tests to work without the security manager:
>> - test/jdk/java/rmi/registry/classPathCodebase/ClassPathCodebase.java
>> - test/jdk/java/rmi/registry/readTest/CodebaseTest.java
>> -
>> test/jdk/java/rmi/server/RMIClassLoader/useCodebaseOnly/UseCodebaseOnly.java
>>Also remove them from the problem list.
>> - Remove two obsolete RMI tests:
>> - test/jdk/java/rmi/server/RMIClassLoader/spi/ContextInsulation.java
>> -
>> test/jdk/sun/rmi/transport/tcp/disableMultiplexing/DisableMultiplexing.java
>>Adjust two tests to run without the Security Manager:
>> -
>> test/jdk/java/rmi/server/RMIClassLoader/loadProxyClasses/LoadProxyClasses.java
>> - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java
>>Remove all of these tests from the problem list.
>> - In staticPermissionsOnly(), change "current policy binding" to "current
>> policy" so wording is consistent with the API note that follows.
>> - Added API Notes to ProtectionDomain clarifying that the current policy
>> always
>>grants no permissions. A few other small changes to Policy and PD.
>> - Merge branch 'master' into jep486
>> - JAXP tests: organize imports of a few tests
>> - Improve description of Executors.privilegedThreadFactory
>> - rename TestAppletLoggerContext.java as suggested in util test review
>> - clientlibs: Javadoc cleanup
>> - ... and 190 more: https://git.openjdk.org/jdk/compare/158ae51b...7958ee2b
>
> src/java.base/share/classes/java/lang/System.java line 1364:
>
>> 1362: *
>> 1363: * It is the responsibility of the provider of
>> 1364: * the concrete {@code LoggerFinder} implementation to ensure that
>
> This is still a part of the paragraph related to the security manager.
Right - this paragraph - lines 1620-1625 (old file) / 1362-1367 (new file) is
no longer relevant and should be removed too. Thanks for spotting that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1826959976
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v6]
On Sat, 2 Nov 2024 22:18:09 GMT, ExE Boss wrote: >> Sean Mullan has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 200 commits: >> >> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 >> - Modify three RMI tests to work without the security manager: >> - test/jdk/java/rmi/registry/classPathCodebase/ClassPathCodebase.java >> - test/jdk/java/rmi/registry/readTest/CodebaseTest.java >> - >> test/jdk/java/rmi/server/RMIClassLoader/useCodebaseOnly/UseCodebaseOnly.java >>Also remove them from the problem list. >> - Remove two obsolete RMI tests: >> - test/jdk/java/rmi/server/RMIClassLoader/spi/ContextInsulation.java >> - >> test/jdk/sun/rmi/transport/tcp/disableMultiplexing/DisableMultiplexing.java >>Adjust two tests to run without the Security Manager: >> - >> test/jdk/java/rmi/server/RMIClassLoader/loadProxyClasses/LoadProxyClasses.java >> - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java >>Remove all of these tests from the problem list. >> - In staticPermissionsOnly(), change "current policy binding" to "current >> policy" so wording is consistent with the API note that follows. >> - Added API Notes to ProtectionDomain clarifying that the current policy >> always >>grants no permissions. A few other small changes to Policy and PD. >> - Merge branch 'master' into jep486 >> - JAXP tests: organize imports of a few tests >> - Improve description of Executors.privilegedThreadFactory >> - rename TestAppletLoggerContext.java as suggested in util test review >> - clientlibs: Javadoc cleanup >> - ... and 190 more: https://git.openjdk.org/jdk/compare/158ae51b...7958ee2b > > src/java.base/share/classes/java/lang/System.java line 2338: > >> 2336: * Invoked by VM. Phase 3 is the final system initialization: >> 2337: * 1. eagerly initialize bootstrap method factories that might >> interact >> 2338: *negatively with custom security managers and custom class >> loaders > > They might still interact negatively with custom class loaders though. The context here is custom a security manager doing string concatenation, this has required StringConcatFactory be eagerly initialized or security managers set on the command line to have been compiled with -XDstringConcat=inline. I think your question is about overriding the system class loader and whether its initialisation can use string concatenation reliably. That's a good thing to add a test for. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1826916876
