RFR: 8238649: Call new Win32 API SetThreadDescription in os::set_native_thread_name

[David Holmes]

Hi Thomas,

Thanks for taking a look at this.

On 11/06/2021 4:05 pm, Thomas Stuefe wrote:
> On Wed, 2 Jun 2021 02:19:52 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>  From Windows 10 and Windows 2016 server, we have a direct API for setting the thread name/description. Use of this API was suggested by Markus Gaisbauer:
>>
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-February/030366.html
>>
>> Using the new API was quite straight forward, but verifying that it had worked correctly was far more challenging. It seems there are no tools that use the new GetThreadDescription API to display thread names, so no easy check that this had worked. While Visual Studio will use it, it also uses the old debugger mechanism, so we wouldn't be able to tell the difference.
>>
>> Writing a Windows-only test was one possibility, but the conversion to/from Unicode and java.lang.String would make that test very cumbersome in itself (for something that should be trivial!).
>>
>> So instead for debug builds I read back the thread name using GetThreadDescription and check that the name we set and the name we read are the same. I'm a bit concerned about the impact this may have on performance so I'm going to run some benchmarks.
>>
>> I will also run benchmarks to watch for issues with the unicode conversion costs related to this.
>>
>> The logging strategy is as follows:
>> - info: show whether the new API is available or not
>> - debug: report failures that are ignored (as we fallback to debugger mechanism)
>> - trace: report successes for full tracking
>>
>> Testing:
>>    - internal self-verification in debug builds as previously described
>>   - verified the logging output on different Windows systems that have, and don't have, the new API
>>   - sanity testing for tiers 1-3
>>
>> Thanks,
>> David
> 
> Hi David,
> 
> I think the original code was one of the first contributions I made to OpenJDK: https://mail.openjdk.java.net/pipermail/hotspot-dev/2014-October/015528.html
> 
> I would not call it "weird" though. At that point it was the only available and officially documented way to set thread names on Windows.

To be clear it isn't our use of this mechanism that is being labelled 
"weird" but the actual win32 mechanism itself. :)

> I am not sure if the error check is worth the complexity, especially since it only kind of checks itself (we may just set and read an invisible variable for all we know). We had no such checks for the old version, nor for the linux implementation.

For the old version and other platforms we can easily check the result 
using external tools, like debuggers and process viewers. But that is 
not the case for this new API. While the newer VS versions support it I 
don't think there would be a way to know that we are observing use of 
the old or the new API for certain (and I don't have direct access to VS 
anyway). Hence the attempt to at least sanity check - though you are 
right I may not actually have set the correct name in the first place. I 
thought about comparing with the passed in char* but the unicode 
gymnastics was more than I could stomach. :)

I'll respond to other comments in the PR UI.

Thanks,
David

> I read https://docs.microsoft.com/en-us/visualstudio/debugger/how-to-set-a-thread-name-in-native-code?view=vs-2019. I believe the documentation is incorrect insofar as that the old version worked for WinDbg too, not only VS, I tested this in 2014.
> 
> Cheers, Thomas
> 
> src/hotspot/os/windows/os_windows.cpp line 890:
> 
>> 888: // For dynamic lookup of SetThreadDescription API
>> 889: typedef HRESULT (WINAPI *SetThreadDescriptionFnPtr)(HANDLE, PCWSTR);
>> 890: typedef HRESULT (WINAPI *GetThreadDescriptionFnPtr)(HANDLE, PWSTR*);
> 
> DEBUG_ONLY?
> 
> src/hotspot/os/windows/os_windows.cpp line 930:
> 
>> 928:                                    thread_name,
>> 929:                                    -1  // null-terminated
>> 930:                                    );
> 
> Why not just use wcscmp? (WCHAR == wchar_t, and also you use %ls below which implies wchar_t, so you may use wcscmp here too).
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/4297
>