On Mon, 28 Oct 2024 23:46:09 GMT, Dean Long <dl...@openjdk.org> 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 > > execution, i.e cancel the preemption. > > Is this purely a performance optimization, or is there a correctness issue if > we don't notice the monitor was released and cancel the preemption? It seems > like the monitor can be released at any time, so what makes freeze special > that we need to check afterwards? We aren't doing the monitor check > atomically, so the monitor could get released right after we check it. So I'm > guessing we choose to check after freeze because freeze has non-trivial > overhead. > After adding the ObjectWaiter to the _cxq we always have to retry acquiring the monitor; this is the same for platform threads. So freezing before that, implies we have to retry. As for whether we need to cancel the preemption if we acquire the monitor, not necessarily. We could still unmount with a state of YIELDING, so the virtual thread will be scheduled to run again. So that part is an optimization to avoid the unmount. >> No, it just happens to be stored at the sender_sp marker. We were already >> making room for two words but only using one. > > `sender_sp_offset` is listed under "All frames", but I guess that's wrong and > should be changed. Can we fix the comments to match x86, which lists this > offset under "non-interpreter frames"? I think aarch64 is the correct one. For interpreter frames we also have a sender_sp() that we get through that offset value: https://github.com/openjdk/jdk/blob/7404ddf24a162cff445cd0a26aec446461988bc8/src/hotspot/cpu/x86/frame_x86.cpp#L458 I think the confusion is because we also have interpreter_frame_sender_sp_offset where we store the unextended sp. >> We read this value from the freeze/thaw code in several places. Since the >> only compiled native frame we allow to freeze is Object.wait0 the old value >> would be zero too. But I think the correct thing is to just set it to zero >> always since a value > 0 is only meaningful for Java methods. > > Isn't it possible that we might allow more compiled native frames in the > future, and then we would have to undo this change? I think this change > should be reverted. If continuations code wants to assert that this is 0, > then that should be in continuations code, the nmethod code doesn't need to > know how this field is used. However, it looks like continuations code is > the only client of this field, so I can see how it would be tempting to just > set it to 0 here, but it doesn't feel right. Any compiled native frame would still require a value of zero. This field should be read as the size of the argument area in the caller frame that this method(callee) might access during execution. That's why we set it to zero for OSR nmethods too. The thaw code uses this value to see if we need to thaw a compiled frame with stack arguments that reside in the caller frame. The freeze code also uses it to check for overlap and avoid copying these arguments twice. Currently we have a case for "nmethods" when reading this value, which includes both Java and native. I'd rather not add branches to separate these cases, specially given that we already have this field available in the nmethod class. >> `PreemptStatus` is meant to be used with `tryPreempt()` which is not >> implemented yet, i.e. there is no method yet that maps between these values >> and the PreemptStatus enum. The closest is `Continuation.pinnedReason` which >> we do use. So if you want I can remove the reference to PreemptStatus and >> use pinnedReason instead. > > Yes, that would be better for now. Changed. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21565#issuecomment-2445106760 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823495787 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824785565 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824788898
