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 Tue, 5 Nov 2024 00:18:17 GMT, Fei Yang wrote:
>>> Also, does this mean that the changes from 2 to frame::sender_sp_offset in
>>> all of the lines (267, 271 and 273) should be reverted?
>>>
>> I think the previous lines are okay because we are constructing the fp,
>> whereas in here we want t
On Mon, 4 Nov 2024 18:23:23 GMT, Patricio Chilano Mateo
wrote:
>> Sorry, I also thought it matched the aarch64 one without checking.
>> @RealFYang should I change it for `hf.sp() + frame::link_offset` or just
>> leave it as it was?
>
>> Also, does this mean that the changes from 2 to frame::se
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/openjdk/jdk/pull/21565/commits/12213a70c1cf
On Fri, 25 Oct 2024 04:40:24 GMT, David Holmes wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Rename set/has_owner_anonymous to set/has_anonymous_owner
>> - Fix comments in javaThread.hpp and Thread.
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 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 Thu, 31 Oct 2024 20:02:31 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/cpu/riscv/continuationFreezeThaw_riscv.inline.hpp line 273:
>>
>>> 271: ? frame_sp + fsize - frame::sender_sp_offset
>>> 272: // we need to re-read fp because it may be an oop and we might
>>> have f
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 Thu, 31 Oct 2024 20:05:18 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/share/runtime/continuation.hpp line 66:
>>
>>> 64:
>>> 65: enum preempt_kind {
>>> 66: freeze_on_monitorenter = 1,
>>
>> Is there a reason why the first enumerator doesn't start at zero?
>
> There was one val
On Wed, 30 Oct 2024 12:48:02 GMT, Fredrik Bredberg
wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Rename set/has_owner_anonymous to set/has_anonymous_owner
>> - Fix comments in javaThread.hpp and Th
On Thu, 24 Oct 2024 21:08:26 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 change
On Mon, 28 Oct 2024 22:02:02 GMT, Patricio Chilano Mateo
wrote:
>> That said such a scenario is not about concurrently pushing the same thread
>> to the list from different threads. So I'm still somewhat confused about the
>> concurrency control here. Specifically I can't see how the cmpxchg o
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 Mon, 28 Oct 2024 00:31:27 GMT, David Holmes wrote:
>> It is, we still increment _waiters for the vthread case.
>
> Sorry the target of my comment was not clear. `thread_of_waiter` looks
> suspicious - will JVMTI find the vthread from the JavaThread?
If the ObjectWaiter is associated with a
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, 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 Mon, 28 Oct 2024 00:38:39 GMT, David Holmes wrote:
>> I thought locking_thread there may not be the current thread for enter_for()
>> in deopt. It's the thread that should hold the lock but not the current
>> thread. But it might be different now.
>
> The thread passed in need not be the c
On Mon, 28 Oct 2024 00:43:47 GMT, David Holmes wrote:
>>> I'm struggling to understand how a thread can already be on this list?
>>>
>> With the removal of the _Responsible thread, it's less likely but it could
>> still happen. One case is when the virtual thread acquires the monitor after
>> a
On Fri, 25 Oct 2024 22:29:56 GMT, Coleen Phillimore wrote:
>>> If it's always the current thread, then it should be called 'current' imo.
>>>
>> The inflating thread is always the current one but it's not always equal to
>> `inflating_thread`.
>
> I thought locking_thread there may not be the cu
On Fri, 25 Oct 2024 18:40:51 GMT, Patricio Chilano Mateo
wrote:
>>> Some comments here about the operation would be useful.
>>>
>> Added a comment.
>
>> I'm struggling to understand how a thread can already be on this list?
>>
> With the removal of the _Responsible thread, it's less likely but i
On Fri, 25 Oct 2024 18:46:52 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/share/runtime/objectMonitor.cpp line 2028:
>>
>>> 2026: // First time we run after being preempted on Object.wait().
>>> 2027: // Check if we were interrupted or the wait timed-out, and in
>>> 2028: // that cas
On Fri, 25 Oct 2024 18:42:29 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/share/runtime/objectMonitor.hpp line 349:
>>
>>> 347: ObjectWaiter* first_waiter()
>>> { return _WaitSet; }
>>> 348: ObjectWaiter* next_waiter(ObjectWaiter* o)
On Fri, 25 Oct 2024 11:59:03 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/objectMonitor.hpp line 174:
>>
>>> 172:
>>> 173: int64_t volatile _owner; // Either tid of owner, NO_OWNER,
>>> ANONYMOUS_OWNER or DEFLATER_MARKER.
>>> 174: volatile uint64_t _previous_owner_tid; // t
On Fri, 25 Oct 2024 21:29:05 GMT, Patricio Chilano Mateo
wrote:
>> I see that in lightweightSynchronizer.cpp we already use the name
>> `locking_thread` (although
>> `LightweightSynchronizer::inflate_into_object_header` still uses
>> `inflating_thread`). So how about using `locking_thread` in
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` (although
> `LightweightSynchronizer::inflate_into_object_heade
On Fri, 25 Oct 2024 05:17:51 GMT, David Holmes wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Rename set/has_owner_anonymous to set/has_anonymous_owner
>> - Fix comments in javaThread.hpp and Thread.
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 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 operation would be useful.
>>
> Added a comment.
> I'm struggling
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_head) {
>>
>> Some comments here about the operation w
On Fri, 25 Oct 2024 00:26:24 GMT, David Holmes wrote:
> The "waiting list" here is just a list of virtual threads that need unparking
> by the Unblocker thread - right?
>
Yes.
> src/hotspot/share/classfile/javaClasses.cpp line 2086:
>
>> 2084: jboolean vthread_on_list = Atomic::load(addr);
>
On Fri, 25 Oct 2024 03:51:08 GMT, David Holmes wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Rename set/has_owner_anonymous to set/has_anonymous_owner
>> - Fix comments in javaThread.hpp and Thread.
> 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:
>
> - Change
On Thu, 24 Oct 2024 21:08:26 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 change
On Thu, 24 Oct 2024 21:08:26 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 change
35 matches
Mail list logo
