On Tue, 17 May 2022 12:23:56 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> 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 > > But you aren't keeping it on the stack versus TLS you are using a stack > object to set the TLS. All we have done in this final form is replace: > > > thread->set_in_asgct(true); > <stuff> > thread->set_in_asgct(false); > > with > > > ASGCTMark mark(thread); > <stuff> > > but now ASGCTMark has to worry about whether `thread` is NULL, whether it is > the current javaThread, when we've already done all that for `thread`. > > And this is the only place we will ever use ASGCTMark. Now if we had lots of > return points in `<stuff>` then RAII would definitely be better to ensure we > can't forget to reset the state. But we don't. So use of RAII adds complexity > cost now just in case in the future RAII might be useful. @dholmes-ora Oh, you are only objecting to the RAII mark, not to the fact that we store the state in Thread? Ok, sure. In my view, a mark prevents the "on" state from escaping the frame, e.g. if stray return calls sneak in with future changes. But I don't have strong emotions here. ------------- PR: https://git.openjdk.java.net/jdk/pull/8549
