On Thu, 9 Jan 2025 14:18:52 GMT, Patricio Chilano Mateo
wrote:
> Hi all,
>
> This pull request contains a backport of commit
> [ea495377](https://github.com/openjdk/jdk/commit/ea49537726db6530f0ddcc04d9938df3d6d18250)
> from the [openjdk/jdk](https://git.openjdk.org/jdk) repos
On Thu, 9 Jan 2025 14:18:52 GMT, Patricio Chilano Mateo
wrote:
> Hi all,
>
> This pull request contains a backport of commit
> [ea495377](https://github.com/openjdk/jdk/commit/ea49537726db6530f0ddcc04d9938df3d6d18250)
> from the [openjdk/jdk](https://git.openjdk.org/jdk) repos
Hi all,
This pull request contains a backport of commit
[ea495377](https://github.com/openjdk/jdk/commit/ea49537726db6530f0ddcc04d9938df3d6d18250)
from the [openjdk/jdk](https://git.openjdk.org/jdk) repository.
The commit being backported was authored by Patricio Chilano Mateo on 8 Jan
2025
On Mon, 6 Jan 2025 21:20:28 GMT, David Holmes wrote:
>> Please review the following fix. In method
>> `JvmtiEventControllerPrivate::recompute_thread_enabled()`, we are missing to
>> call `leave_interp_only_mode()` for the case where `should_be_interp` is
>> computed as false and `state->is_pen
On Mon, 6 Jan 2025 16:41:21 GMT, Patricio Chilano Mateo
wrote:
> Please review the following fix. In method
> `JvmtiEventControllerPrivate::recompute_thread_enabled()`, we are missing to
> call `leave_interp_only_mode()` for the case where `should_be_interp` is
> computed as fals
On Wed, 8 Jan 2025 05:35:34 GMT, Serguei Spitsyn wrote:
> The fix looks good. It is a great and important finding - thanks! What was
> bothering me is that the `EnterInterpOnlyModeClosure` is used in the
> `JvmtiEventControllerPrivate::enter_interp_only_mode()` but no
> `HandshakeClosure` is u
Please review the following fix. In method
`JvmtiEventControllerPrivate::recompute_thread_enabled()`, we are missing to
call `leave_interp_only_mode()` for the case where `should_be_interp` is
computed as false and `state->is_pending_interp_only_mode()` is true. I added
the full trace leading t
On Wed, 4 Dec 2024 15:47:26 GMT, Patricio Chilano Mateo
wrote:
>> Please review this small renaming patch. During the review of JDK-8338383
>> there were some comments about improving the naming for
>> `ObjectMonitor::owner_from()` and `JavaThread::_lock_id`. These orig
On Tue, 3 Dec 2024 19:10:55 GMT, Patricio Chilano Mateo
wrote:
> Please review this small renaming patch. During the review of JDK-8338383
> there were some comments about improving the naming for
> `ObjectMonitor::owner_from()` and `JavaThread::_lock_id`. These originate
> from
On Wed, 4 Dec 2024 01:36:42 GMT, David Holmes wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix parameter name
>
> src/hotspot/share/runtime/javaThread.h
w record the
> `java.lang.Thread.tid` of the owner in the ObjectMonitor's `_owner` field
> instead of a `JavaThread*`. I renamed `_lock_id` as `_monitor_owner_id` and
> `owner_from()` as `owner_id_from()`.
>
> Thanks,
> Patricio
Patricio Chilano Mateo has updated the pull request
Please review this small renaming patch. During the review of JDK-8338383 there
were some comments about improving the naming for `ObjectMonitor::owner_from()`
and `JavaThread::_lock_id`. These originate from the changes introduced to
inflated monitors, where we now record the `java.lang.Thread.
On Thu, 14 Nov 2024 22:34:24 GMT, Patricio Chilano Mateo
wrote:
> Small follow-up change after JDK-8338383. This moves the `objectWaiter` field
> from the stackChunk to the VirtualThread instance. Only the top stackChunk
> uses this field so we could save some memory by just saving
On Thu, 14 Nov 2024 22:34:24 GMT, Patricio Chilano Mateo
wrote:
> Small follow-up change after JDK-8338383. This moves the `objectWaiter` field
> from the stackChunk to the VirtualThread instance. Only the top stackChunk
> uses this field so we could save some memory by just saving
Small follow-up change after JDK-8338383. This moves the `objectWaiter` field
from the stackChunk to the VirtualThread instance. Only the top stackChunk uses
this field so we could save some memory by just saving it in the virtual thread
instance instead. Also, related methods
`stackChunkOopDes
On Thu, 17 Oct 2024 14:28:30 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 eas
On Tue, 12 Nov 2024 02:59:59 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 m
On Tue, 12 Nov 2024 02:59:59 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 m
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 no
On Fri, 8 Nov 2024 13:43:14 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/share/oops/stackChunkOop.inline.hpp line 189:
>>
>>> 187: inline ObjectMonitor* stackChunkOopDesc::current_pending_monitor()
>>> const {
>>> 188: ObjectWaiter* waiter =
On Wed, 30 Oct 2024 17:23:31 GMT, Coleen Phillimore wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Fix in JvmtiEnvBase::get_locked_objects_in_frame()
>> - Add ObjectW
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.
>
>
On Thu, 7 Nov 2024 09:45:40 GMT, Amit Kumar wrote:
> > I think we can add @requires vm.continuations to this test. It's not useful
> > to run with the alternative virtual thread implementation.
>
> Sure, that sounds ok. Thanks.
>
Added `@requires vm.continuations` to the test.
-
P
On Thu, 7 Nov 2024 18:32:14 GMT, Serguei Spitsyn wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use JvmtiVTMSTransitionDisabler::VTMS_vthread_mount/unmount
>
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 no
On Wed, 6 Nov 2024 15:57:55 GMT, Serguei Spitsyn wrote:
> The two extension events were designed to be posted when the current thread
> identity is virtual, so this behavior > needs to be considered as a bug. My
> understanding is that it is not easy to fix.
>
If we want to post the mount/unmou
On Thu, 7 Nov 2024 00:38:40 GMT, Patricio Chilano Mateo
wrote:
>>> So, it feels like it should not be a problem. I'm thinking if adding an
>>> assert at the VTMS transition end would help.
>>>
>> The problem here is that for monitorenter the top frame w
On Thu, 7 Nov 2024 00:37:17 GMT, Patricio Chilano Mateo
wrote:
>> Thank you for the comment! I'm okay with your modified suggestion in general
>> if there are no road blocks.
>>
>>> but does the specs say the event has to be posted in the context of the
>&g
On Thu, 7 Nov 2024 00:37:53 GMT, Patricio Chilano Mateo
wrote:
>> Great, I applied the suggested simplification. I had to update test
>> `VThreadEventTest.java` to check the stack during the mount/unmount events
>> to only count the real cases. This is because now we are
On Wed, 6 Nov 2024 16:31:24 GMT, Serguei Spitsyn wrote:
>> Regarding the pop_frame/early_ret/async_exception conditions, not checking
>> for them after we started the transition would be an issue.
>> For pop_frame/early_ret checks, the problem is that if any of them are
>> installed in `JvmtiUn
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.
>
>
On Sat, 26 Oct 2024 05:39:32 GMT, Alan Bateman wrote:
>> test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java line
>> 30:
>>
>>> 28: * by reflection API
>>> 29: * @library /test/lib/
>>> 30: * @requires vm.compMode != "Xcomp"
>>
>> If there is a problem with this te
On Tue, 29 Oct 2024 02:09:24 GMT, Dean Long 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 sp
On Mon, 28 Oct 2024 00:29:25 GMT, David Holmes wrote:
>> I was wondering what this was too but the _previous_owner_tid is the os
>> thread id, not the Java thread id.
>>
>>
>> $ grep -r JFR_THREAD_ID
>> jfr/support/jfrThreadId.hpp:#define JFR_THREAD_ID(thread)
>> (JfrThreadLocal::external_thr
On Mon, 21 Oct 2024 08:01:09 GMT, Andrey Turbanov 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 b
On Wed, 6 Nov 2024 06:36:58 GMT, Axel Boldt-Christmas
wrote:
> A small note on `_cont_fastpath`, as it is now also used for synchronised
> native method calls (native wrapper) maybe the comment should be updated to
> reflect this.
>
> ```
> // the sp of the oldest known interpreted/call_stub
On Mon, 4 Nov 2024 07:59:22 GMT, Alan Bateman wrote:
>> 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");
>>
>>
On Mon, 28 Oct 2024 09:19:48 GMT, Alan Bateman wrote:
>> Thanks for the explanation but that needs to be documented somewhere.
>
> The comment in afterYield has been expanded in the loom repo, we may be able
> to bring that update in.
Brought the comment from the loom repo.
-
PR R
On Wed, 23 Oct 2024 05:18:10 GMT, David Holmes wrote:
>> Thread identity switches to the carrier so Thread.currentThread() is the
>> carrier thread and JavaThread._lock_id is the thread identifier of the
>> carrier. setCurrentLockId changes JavaThread._lock_id back to the virtual
>> thread's
On Thu, 31 Oct 2024 20:28:06 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java line
>> 57:
>>
>>> 55: static {
>>> 56: try {
>>> 57:
>>> MethodHandles.lookup().ensureInitialized(AnchorCertificates.class);
>>
>> Why is th
On Wed, 23 Oct 2024 20:34:48 GMT, Patricio Chilano Mateo
wrote:
>> If the virtual thread is un-mounting from the carrier, why do we need to set
>> the "lock id" back to the virtual thread's id? Sorry I'm finding this quite
>> confusing.
>>
>>
On Thu, 24 Oct 2024 02:55:18 GMT, David Holmes wrote:
>>> Also JavaThread::_lock_id in the VM means "the java.lang.Thread thread-id
>>> to use for locking" - correct?
>>>
>> Yes.
>
> I guess I don't understand where this piece code fits in the overall
> transition of the virtual thread to being
On Wed, 23 Oct 2024 11:32:54 GMT, Alan Bateman wrote:
>> Suggestion: `timedWaitCounter` ?
>
> We could rename it to timedWaitSeqNo if needed.
Ok, renamed to timedWaitSeqNo.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1813240667
On Mon, 28 Oct 2024 11:59:57 GMT, Coleen Phillimore wrote:
>> The thread passed in need not be the current thread, and IIUC is the thread
>> that should become the owner of the newly inflated monitor (either current
>> thread or a suspended thread). The actual inflation is always done by the
>
On Fri, 25 Oct 2024 21:28:22 GMT, Patricio Chilano Mateo
wrote:
>> If it's always the current thread, then it should be called 'current' imo.
>
> I see that in lightweightSynchronizer.cpp we already use the name
> `locking_thread` (althou
On Mon, 28 Oct 2024 00:35:11 GMT, David Holmes wrote:
>> This vthread was unmounted on the call to `Object.wait`. Now it is mounted
>> and "running" again, and we need to check which case it is in: notified,
>> interrupted or timed-out. "First time" means it is the first time it's
>> running a
On Thu, 31 Oct 2024 02:26:42 GMT, David Holmes wrote:
>> src/hotspot/share/runtime/objectMonitor.inline.hpp line 207:
>>
>>> 205: }
>>> 206:
>>> 207: inline bool ObjectMonitor::has_successor() {
>>
>> Why are _succ accesses atomic here when previously they were not?
>
> General convention is t
On Thu, 24 Oct 2024 08:08:56 GMT, Stefan Karlsson 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 b
On Fri, 25 Oct 2024 12:00:43 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/synchronizer.cpp line 1440:
>>
>>> 1438: }
>>> 1439:
>>> 1440: ObjectMonitor* ObjectSynchronizer::inflate_impl(JavaThread*
>>> inflating_thread, oop object, const InflateCause cause) {
>>
>> `inflating_thr
On Thu, 31 Oct 2024 16:34:41 GMT, Patricio Chilano Mateo
wrote:
>> General convention is that racily accessed variables should be accessed via
>> Atomic::load/store to make it clear(er) they are racy accesses. But I agree
>> it seems odd when direct accesses to `_succ` in
On Mon, 28 Oct 2024 13:12:22 GMT, Richard Reingruber wrote:
>> src/hotspot/share/runtime/objectMonitor.hpp line 202:
>>
>>> 200:
>>> 201: // Used in LM_LEGACY mode to store BasicLock* in case of inflation
>>> by contending thread.
>>> 202: BasicLock* volatile _stack_locker;
>>
>> IIUC the
On Wed, 23 Oct 2024 09:58:44 GMT, Alan Bateman wrote:
>> src/hotspot/share/runtime/javaThread.hpp line 166:
>>
>>> 164: // current _vthread object, except during creation of the primordial
>>> and JNI
>>> 165: // attached thread cases where this field can have a temporary value.
>>> 166:
On Tue, 22 Oct 2024 11:51:47 GMT, Alan Bateman wrote:
>> src/hotspot/share/runtime/javaThread.cpp line 1545:
>>
>>> 1543: if (is_vthread_mounted()) {
>>> 1544: // _lock_id is the thread ID of the mounted virtual thread
>>> 1545: st->print_cr(" Carrying virtual thread #" INT64_F
On Wed, 23 Oct 2024 05:33:55 GMT, Axel Boldt-Christmas
wrote:
>> Ok, I'll change copy_lockstack to both load and clear the oops in the same
>> method. Now, when we call do_barriers on recurse_thaw we don't clear the
>> oops, we just load and store the loaded value again. Is it the case that we
On Mon, 4 Nov 2024 09:24:13 GMT, Stefan Karlsson wrote:
>> If I recall correctly this was a bug where one of the stackChunk fields was
>> allocated in that gap, but since we didn't zeroed it out it would start with
>> some invalid value. I guess the reason why we are not hitting this today is
On Thu, 31 Oct 2024 21:11:39 GMT, Fredrik Bredberg
wrote:
>> There was one value that meant to be for the regular freeze from java. But
>> it was not used so I removed it.
>
> Fair enough, but I would prefer if you start at zero. Just so people like me
> don't start scratching their head tryin
On Wed, 23 Oct 2024 09:53:44 GMT, Alan Bateman wrote:
>> The problem is that within that window we don't have access to the virtual
>> thread's tid. The current thread has already been changed and we haven't yet
>> set the lock id back. Since this will be a rare corner case maybe we can
>> jus
On Tue, 5 Nov 2024 23:50:29 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/share/runtime/continuation.cpp line 134:
>>
>>> 132: return true;
>>> 133: }
>>> 134: #endif // INCLUDE_JVMTI
>>
>> Could you, please, consider the simplificati
On Wed, 30 Oct 2024 19:02:05 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1411:
>>
>>> 1409: // zero out fields (but not the stack)
>>> 1410: const size_t hs = oopDesc::header_size();
>>> 1411: oopDesc::set_klass_gap(mem, 0);
>>
>> Why,
On Tue, 29 Oct 2024 02:56:30 GMT, Serguei Spitsyn 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 b
On Mon, 28 Oct 2024 10:37:21 GMT, Yudi Zheng 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 s
On Tue, 5 Nov 2024 08:19:34 GMT, Alan Bateman wrote:
>> src/hotspot/share/jfr/metadata/metadata.xml line 160:
>>
>>> 158:
>>> 159:
>>> 160:
>>
>> Previously, the event was in the "Java Application" category. I think that
>> was a better fit because it meant it was visualized in the sa
On Tue, 5 Nov 2024 07:51:05 GMT, Erik Gahlin 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 s
On Mon, 28 Oct 2024 00:55:34 GMT, David Holmes wrote:
>> H ... I guess we either slow down the monitor code by having the thread
>> search for and remove itself, or we allow for this and handle it correctly
>> ... okay.
>
> That said such a scenario is not about concurrently pushing the sa
On Mon, 4 Nov 2024 05:52:16 GMT, Alan Bateman wrote:
>> 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
On Fri, 25 Oct 2024 18:39:54 GMT, Patricio Chilano Mateo
wrote:
>>> The "waiting list" here is just a list of virtual threads that need
>>> unparking by the Unblocker thread - right?
>>>
>> Yes.
>
>> Some comments here about the ope
On Fri, 25 Oct 2024 18:39:23 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/share/classfile/javaClasses.cpp line 2082:
>>
>>> 2080: }
>>> 2081:
>>> 2082: bool java_lang_VirtualThread::set_onWaitingList(oop vthread,
>>> OopHandle& list_h
On Mon, 28 Oct 2024 07:55:02 GMT, Richard Reingruber 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 hav
On Tue, 29 Oct 2024 08:29:55 GMT, David Holmes wrote:
>> It's conceivable that in the future we might have more native methods we
>> want to preempt. Instead of enumerating them all, we could set a flag on
>> the method.
>>
>> I was assuming that David was suggesting we have the Java caller d
On Mon, 28 Oct 2024 01:13:05 GMT, David Holmes 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
On Tue, 22 Oct 2024 02:14:23 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/cpu/x86/assembler_x86.cpp line 2866:
>>
>>> 2864: emit_int32(0);
>>> 2865: }
>>> 2866: }
>>
>> Is it possible to make this more general and explicit instead
On Tue, 5 Nov 2024 00:23:37 GMT, Fei Yang wrote:
>>> As the same code on aarch64 and x86-64 uses `frame::sender_sp_offset` I
>>> suggested to change the literal 2 into `frame::sender_sp_offset` in order
>>> to increase the readability, but I forgot that `frame::sender_sp_offset` is
>>> 0 on ri
On Sat, 2 Nov 2024 02:41:44 GMT, Fei Yang wrote:
>> Changed.
>
> Note that `frame::sender_sp_offset` is 0 instead of 2 on linux-riscv64, which
> is different from aarch64 or x86-64. So I think we should revert this change:
> https://github.com/openjdk/jdk/pull/21565/commits/12213a70c1cf0639555f
On Mon, 4 Nov 2024 18:22:42 GMT, Patricio Chilano Mateo
wrote:
>> Note that `frame::sender_sp_offset` is 0 instead of 2 on linux-riscv64,
>> which is different from aarch64 or x86-64. So I think we should revert this
>> change:
>> https://github.com/openjd
On Mon, 28 Oct 2024 23:59:55 GMT, Patricio Chilano Mateo
wrote:
>> So it sounds like the adjustment at line 119 is a bug fix, but what I don't
>> understand is why we weren't seeing problems before. Something in this PR
>> exposed the need for this change.
>
&
On Mon, 28 Oct 2024 23:21:14 GMT, Dean Long wrote:
>> What are we counting now with MaskFillerForNativeFrame that we weren't
>> counting before this change? in MaskFillerForNative::set_one.
>
> So it sounds like the adjustment at line 119 is a bug fix, but what I don't
> understand is why we w
On Mon, 28 Oct 2024 20:10:16 GMT, Dean Long wrote:
>> It's the offset of the mirror passed to static native calls. It pre-existed
>> saving the mirror in all frames to keep the Method alive, and is duplicated.
>> I think this could be cleaned up someday, which would remove this special
>> ca
On Tue, 29 Oct 2024 10:06:01 GMT, Fredrik Bredberg
wrote:
>> Right. We want to take the slow path to find the compiled native wrapper
>> frame and fail to freeze. Otherwise the fast path won't find it since we
>> don't walk the stack.
>
> It would be nice if Coleen's question and your answer c
On Mon, 28 Oct 2024 18:51:31 GMT, Dean Long wrote:
>> Its indeed difficult to see how the value is propagaged. I think it goes
>> like this:
>>
>> - read from the frame anchor and set as pc of `_last_frame`:
>> https://github.com/pchilano/jdk/blob/66d5385f8a1c84e73cdbf385239089a7a9932a9e/src/h
On Wed, 30 Oct 2024 12:48:02 GMT, Fredrik Bredberg
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
On Wed, 23 Oct 2024 05:56:48 GMT, Axel Boldt-Christmas
wrote:
>> Also, is it better to have this without assignment. Which is a nit.
>> Address dst(rthread, JavaThread::held_monitor_count_offset());
>
> The `=` in a variable definition is always construction, never assignment.
>
> That said,
On Tue, 22 Oct 2024 15:50:15 GMT, Andrew Haley 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
On Mon, 28 Oct 2024 00:53:40 GMT, David Holmes wrote:
>> _cont_fastpath is what we check in freeze_internal to decide if we can take
>> the fast path. Since we are calling from the interpreter we have to take the
>> slow path. Added a comment.
>
> It seems somewhat of an oxymoron that to force
On Tue, 5 Nov 2024 06:30:55 GMT, Fei Yang wrote:
>> Great, thanks Dean. I removed `possibly_adjust_frame()` and the related code.
>> @RealFYang I made the equivalent change for riscv, could you verify it's
>> okay?
>
> @pchilano : Hi, Great to see `possibly_adjust_frame()` go away. Nice cleanup!
On Fri, 1 Nov 2024 20:08:51 GMT, Dean Long wrote:
>> It turns out if we try to set last pc to the instruction after the
>> adjustment, then we need an oopmap there, and that would require more C2
>> changes. Then I thought about restoring SP from FP or last_Java_fp, but I
>> don't think we ca
On Thu, 31 Oct 2024 02:33:30 GMT, Dean Long wrote:
> OK, so you're saying it's the stack adjustment that's the problem. It sounds
> like there is code that is using rsp instead of last_Java_sp to compute the
> frame boundary. Isn't that a bug that should be fixed?
>
It's not a bug, it's just th
On Mon, 28 Oct 2024 23:38:43 GMT, Dean Long wrote:
>>> Could the problem be solved with a resume adapter instead, like the
>>> interpreter uses?
>>>
>> It will just move the task of adjusting the size of the frame somewhere else.
>
>> One way to get rid of this would be to have c2 just set last_
On Mon, 28 Oct 2024 18:56:25 GMT, Patricio Chilano Mateo
wrote:
>> Could the problem be solved with a resume adapter instead, like the
>> interpreter uses?
>
> The issue with the c2 runtime stub on aarch64 (and riscv) is that
> cb->frame_size() doesn't match t
On Sat, 26 Oct 2024 01:58:30 GMT, Dean Long wrote:
>> src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 310:
>>
>>> 308: sp -= 2;
>>> 309: sp[-2] = sp[0];
>>> 310: sp[-1] = sp[1];
>>
>> This also seems fragile. This seems to depend on an intimate knowledge of
On Wed, 23 Oct 2024 22:59:19 GMT, Coleen Phillimore 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
On Tue, 22 Oct 2024 15:56:21 GMT, Andrew Haley wrote:
>> Note also that `inc_held_monitor_count` clobbers `rscratch2`. That might be
>> worth a comment at the call site.
>> I guess `inc_held_monitor_count` is so hot that we can't push and pop
>> scratch registers, in which case it'd clobber no
On Mon, 21 Oct 2024 06:38:28 GMT, Axel Boldt-Christmas
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
On Mon, 28 Oct 2024 17:31:45 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp line 188:
>>
>>> 186: // Avoid using a leave instruction when this frame may
>>> 187: // have been frozen, since the current value of rfp
>&g
On Tue, 5 Nov 2024 01:47:29 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 m
On Thu, 17 Oct 2024 14:28:30 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 eas
On Mon, 28 Oct 2024 23:46:09 GMT, Dean Long wrote:
> > regardless of when you freeze, while doing the freezing the monitor could
> > have been released already. So trying to acquire the monitor after freezing
> > can always succeed, which means we don't want to unmount but continue
> > executi
On Sat, 26 Oct 2024 02:15:29 GMT, Dean Long wrote:
> > On failure to acquire a monitor inside `ObjectMonitor::enter` a virtual
> > thread will call freeze to copy all Java frames to the heap. We will add
> > the virtual thread to the ObjectMonitor's queue and return back to Java.
> > Instead o
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 unmou
On Tue, 5 Nov 2024 14:35:11 GMT, Axel Boldt-Christmas
wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with five
>> additional commits since the last revision:
>>
>> - Add oopDesc::has_klass_gap() check
>> - Rename waitTim
On Tue, 5 Nov 2024 11:35:29 GMT, Serguei Spitsyn wrote:
> Is this posted after the VirtualThreadMount extension event posted?
>
It's posted before. We post the mount event at the end of thaw only if we are
able to acquire the monitor:
https://github.com/openjdk/jdk/blob/124efa0a6b8d05909e10005
1 - 100 of 343 matches
Mail list logo
