On Thu, 31 Oct 2024 00:52:02 GMT, Dean Long <dl...@openjdk.org> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix typos in comments
>
> src/hotspot/share/runtime/continuationJavaClasses.inline.hpp line 189:
>
>> 187:
>> 188: inline uint8_t jdk_internal_vm_StackChunk::lockStackSize(oop chunk) {
>> 189: return
>> Atomic::load(chunk->field_addr<uint8_t>(_lockStackSize_offset));
>
> If these accesses need to be atomic, could you add a comment explaining why?
It is read concurrently by GC threads. Added comment.
> src/hotspot/share/runtime/deoptimization.cpp line 125:
>
>> 123:
>> 124: void DeoptimizationScope::mark(nmethod* nm, bool inc_recompile_counts) {
>> 125: if (!nm->can_be_deoptimized()) {
>
> Is this a performance optimization?
No, this might be a leftover. When working on the change for Object.wait I was
looking at the deopt code and thought this check was missing. It seems most
callers already filter this case except WB_DeoptimizeMethod.
> src/hotspot/share/runtime/objectMonitor.cpp line 1612:
>
>> 1610:
>> 1611: static void vthread_monitor_waited_event(JavaThread *current,
>> ObjectWaiter* node, ContinuationWrapper& cont, EventJavaMonitorWait* event,
>> jboolean timed_out) {
>> 1612: // Since we might safepoint set the anchor so that the stack can we
>> walked.
>
> I was assuming the anchor would have been restored to what it was at
> preemption time. What is the state of the anchor at resume time, and is it
> documented anywhere?
> I'm a little fuzzy on what frames are on the stack at this point, so I'm not
> sure if entry_sp and entry_pc are the best choice or only choice here.
The virtual thread is inside the thaw call here which is a leaf VM method, so
there is no anchor. It is still in the mount transition before thawing frames.
The top frame is Continuation.enterSpecial so that's what we set the anchor to.
> src/hotspot/share/runtime/objectMonitor.inline.hpp line 44:
>
>> 42: inline int64_t ObjectMonitor::owner_from(JavaThread* thread) {
>> 43: int64_t tid = thread->lock_id();
>> 44: assert(tid >= 3 && tid < ThreadIdentifier::current(), "must be
>> reasonable");
>
> Should the "3" be a named constant with a comment?
Yes, changed to use ThreadIdentifier::initial().
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824792648
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824793200
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824791832
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824793737
