On Tue, 17 May 2022 08:13:11 GMT, Jaroslav Bachorik <jbacho...@openjdk.org>
wrote:
>> src/hotspot/share/prims/forte.hpp line 53:
>>
>>> 51: ASGCTMark() : ASGCTMark(JavaThread::current()) {}
>>> 52: ~ASGCTMark() {
>>> 53: JavaThread::current()->set_in_asgct(false);
>>
>> You can't call `JavaThread::current()` in any of this code as it is not safe
>> from a signal handling context. (There is an existing use in ASGCT that is
>> also unsafe.) I suggest not having the no-arg constructor and require the
>> current JavaThread, or null, to be passed in (having already been safely
>> obtained by the caller). You can then assert using
>> `Thread::current_or_null_safe()`.
>>
>> Personally I find the ASGCTMark complete overkill for this situation as
>> there is only ever going to be a single use - sorry @tstuefe - it is just
>> complicating things IMO.
>
> Ok, I fixed the `ASGCTMark` to be safe to use from a signal handler.
>
> I have no strong opinion about whether we should keep it or not - it makes
> the code in `forte.cpp` slightly more resilient in case of further
> modifications for the price of more complexity introduced by the mark class 🤷
Hi David,
> You can't call `JavaThread::current()` in any of this code as it is not safe
> from a signal handling context. (There is an existing use in ASGCT that is
> also unsafe.) I suggest not having the no-arg constructor and require the
> current JavaThread, or null, to be passed in (having already been safely
> obtained by the caller). You can then assert using
> `Thread::current_or_null_safe()`.
>
> Personally I find the ASGCTMark complete overkill for this situation as there
> is only ever going to be a single use - sorry @tstuefe - it is just
> complicating things IMO.
No problem, that's why we talk :)
To make an argument for this: if you compare the first iterations of this patch
to the current one, the current one is a lot simpler. I really dislike
funneling arguments through many layers down to a utility function, only to
fine control one specific behavioral aspect of that tiny function, and that
aspect is only relevant for one outlier use case. I dislike this more than the
RAII object.
Look at it this way. We have a thread-specific state (we are in AGCT and deep
down somewhere we want to do something different depending on that state). We
need to pass that information down. The only difference is where we keep the
"we are in thread-specific state" info - on the stack as an argument chain, or
anchored to TLS.
Putting it into arguments is one way, but I find this rather ugly. Now every
caller of this function has to care. With the RAII, we have the information as
a mark exactly where it matters - in the entrance frame of AGCT - and no
outside code needs to care.
Cheers, Thomas
-------------
PR: https://git.openjdk.java.net/jdk/pull/8549
