On 26/03/2021 6:06 pm, Baesken, Matthias wrote:
Hi David, thanks for the info .
I found
https://docs.microsoft.com/en-us/windows/win32/procthread/thread-security-and-access-rights
so it looks like we need THREAD_QUERY_INFORMATION or
THREAD_QUERY_LIMITED_INFORMATION access right for GetThreadTimes .
On the other hand , the test in os::is_thread_cpu_time_supported() on
Windows might (temporary ?) fail for other reasons too , it is not
clear to me if this is really always
related to the wrong access rights ?
Sure it could fail for other reasons (unfortunately win32 docs don't
actually list them). So I tend to agree that this kind of check as a
global "do we support thread cpu times" is not the right test to do. The
question is really about "does this platform provide an API for getting
the thread cpu time" - and it does. Whether you can query a given target
thread at runtime is a different matter altogether.
I wish I knew exactly what caused this check to be put in place as it
doesn't seem appropriate. Maybe someone from serviceablity (cc'd) remembers?
And at some places in HS code like jfrThreadCPULoadEvent.cpp ,
os::thread_cpu_time is called anyway without checking for
os::is_thread_cpu_time_supported() ;
same for thread.cpp / Thread::print_on but this is just printing some
output so it is most likely not really a big issue on Windows .
As long as they can tolerate the -1 return if it fails then it is up to
that code whether or not to bother with the check. But I think the check
is primarily for the M&M/JVMTI capability checking.
Cheers,
David
Best regards, Matthias
On 25/03/2021 11:49 pm, Baesken, Matthias wrote:
Hello, I wonder , should we just return true in
os::is_thread_cpu_time_supported() on Windows ?
See
https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L4588
According to MSDN
https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getthreadtimes
GetThreadTimes is supported on Win2003/XP and higher . This should be
fine for OpenJDK .
Yes it should be fine. There may be other Windows archaisms in the code
that could be cleaned up now.
The issue was not API availability (we got rid of that check a long time
ago) but security permissions. We actually just returned "true" prior to
JDK 5 but that was changed by JDK-4884677 when the JVM TI support was added.
It is a bit messy. We use the result of
os::is_thread_cpu_time_supported() at initialization time, on the main
thread to then decide the global availability of this feature. And via
the normal launcher that thread will have all access bits set and so we
will flag thread_cpu_time as being available. At runtime we might
encounter a thread for which the access bits are not present and so the
actual get_thread_cpu_time call may return -1. In theory the JVM could
be loaded on a thread without full permissions and so we would then
globally disable thread_cpu_time.
So I think this code has to stay.
