RFR: 8282475: SafeFetch should not rely on existence of Thread::current [v6]

[Anton Kozlov]

On Thu, 10 Mar 2022 12:41:11 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> You are saying that Java Threads may write too. And CompilerThreads, in the case of SafeFetch at least, may run generated code. So switching has to be done as a stack mark. Have I gotten this right?

Right.

> Depending on what the pthread library call does, and if it's a real function call into a library, it would be more expensive than that.

Yes, unfortunately we need something like this.

> > If we compare approaches in general (not only SafeFetch), the Thread is already in hands, so we should to compare a read of the field from a C++ object, and the read of a TLS variable. The former could not be slower than the latter.
>
> To me most of the invocations of `ThreadWXEnable` seem to use `Thread::current()`. Only those who retrieve the thread from the JNI environment don't.

Right, JNI env is used e.g. in interfaceSupport.hpp where the most VM entries are defined. I found only few instances of ThreadWXEnable to receive Thread::current() as the argument immediately. In the rest, the Thread is there somewhere in the context.

>
> IIRC, TLS, at least on Linux, lives at the front of the thread stack, so accessing it should be quite cheap.
>
> I see the performance point of an option to pass in Thread* in case one already has it. I dislike it a bit because it gives the illusion that you could pass in arbitrary threads when in fact you must pass in Thread::current. But an assertion could help clarifying here.

There is the assert in Thread::enable_wx, where the implementation actually is unable to handle anything except the current threads.

> > Is it possible to change SafeFetch only? Switch to WXExec before calling the stub and switch WXWrite back unconditionally? We won't need to provide assert in ThreadWXEnable. But SafeFetch can check the assumption with assert via Thread, if it exists.
>
> But SafeFetch could be used from outside code as well as VM code. In case of the latter, prior state can either be WXWrite or WXExec. It needs to restore the prior state after the call.

I'm not sure I understand what is the "outside code". The SafeFetch is the private hotspot function, it cannot be linked with non-JVM code, isn't it?


> To summarize the different proposals:
>
> * you propose to use Thread* when available and assume WXWrite as prior state when not. You argue that if there is no Thread::current, we are not a VM thread and we should need no nesting, so a simple switchback to wxwrite should suffice after leaving SafeFetch, right?

So far I like the another approach more, that is, always assume WXWrite and use the Thread only to assert the state. But I did not understand your concern above.

> * Johannes proposes to use TLS, and just always support nesting, regardless of who calls.
>
> What I like about Johannes proposal is that its simple. It has fewer dependencies on VM infrastructure and we can mostly just hide it in the platform tree.

I'm not opposing the refactoring, it has some advantages, but I'd want to separate functional change from the refactoring. This would aid backporting, at least.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7727