-
Notifications
You must be signed in to change notification settings - Fork 6k
8238649: Call new Win32 API SetThreadDescription in os::set_native_thread_name #4297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into |
@dholmes-ora The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
/contributor add Markus GaisBauer markus.gaisbauer@dynatrace.com |
@dholmes-ora |
I've checked the performance impact and there are no regressions across a set of sanity footprint and performance benchmarks in product mode. For fastdebug there is a very slight regression in footprint, but that seems likely to be a false result and is not important for fastdebug anyway. |
I wonder if I could entice any of our Microsoft contributors to give this a technical review (even if not Reviewers) - @luhenry ? Thanks |
Mailing list message from David Holmes on hotspot-runtime-dev: Trying again to get some attention on this PR. :) Thanks, On 4/06/2021 11:06 am, David Holmes wrote: |
1 similar comment
Mailing list message from David Holmes on hotspot-runtime-dev: Trying again to get some attention on this PR. :) Thanks, On 4/06/2021 11:06 am, David Holmes wrote: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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
@@ -885,8 +885,62 @@ uint os::processor_id() { | |||
return (uint)GetCurrentProcessorNumber(); | |||
} | |||
|
|||
// For dynamic lookup of SetThreadDescription API | |||
typedef HRESULT (WINAPI *SetThreadDescriptionFnPtr)(HANDLE, PCWSTR); | |||
typedef HRESULT (WINAPI *GetThreadDescriptionFnPtr)(HANDLE, PWSTR*); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEBUG_ONLY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is only a typedef I didn't think the DEBUG_ONLY ugliness was warranted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I find it inconsistent, but its also not important. Okay.
-1, // null-terminated | ||
thread_name, | ||
-1 // null-terminated | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply because I wasn't aware of it. I'm not at all familiar with Unicode/string API's on Windows and just looked around for something that would seem to do the job. All these types seem to be wchar_t in some form under the covers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, its not that well documented. But we fall back to wcs... APIs in a couple of places. But if your code works there is no need to change it.
Another issue with the check just occurred to me which is that there may be (its not documented) a limit to the thread name length. There is one on Linux. Which means a check would need to do a substring comparison. |
Mailing list message from David Holmes on hotspot-runtime-dev: Hi Thomas, Thanks for taking a look at this. On 11/06/2021 4:05 pm, Thomas Stuefe wrote:
To be clear it isn't our use of this mechanism that is being labelled
For the old version and other platforms we can easily check the result I'll respond to other comments in the PR UI. Thanks, |
Mailing list message from David Holmes on hotspot-runtime-dev: On 11/06/2021 4:21 pm, Thomas Stuefe wrote:
I'm assuming that if no limitations are documented then they don't Cheers, |
1 similar comment
Mailing list message from David Holmes on hotspot-runtime-dev: On 11/06/2021 4:21 pm, Thomas Stuefe wrote:
I'm assuming that if no limitations are documented then they don't Cheers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@@ -4311,6 +4367,24 @@ jint os::init_2(void) { | |||
jdk_misc_signal_init(); | |||
} | |||
|
|||
// Lookup SetThreadDescription - the docs state we must use runtime-linking of | |||
// kernelbase.dll, so that is what we do. | |||
HINSTANCE _kernelbase = LoadLibrary(TEXT("kernelbase.dll")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kernel32.dll
would be the better dll to look into (per https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setthreaddescription and the .NET source code).
Mailing list message from David Holmes on hotspot-runtime-dev: On 11/06/2021 6:31 pm, Ludovic Henry wrote:
Thanks for reviewing it!
Those docs state: "Windows Server 2016, Windows 10 LTSB 2016 and Windows 10 version 1607: so if I use kernel32 does that mean it will fail on the above Windows Thanks, |
1 similar comment
Mailing list message from David Holmes on hotspot-runtime-dev: On 11/06/2021 6:31 pm, Ludovic Henry wrote:
Thanks for reviewing it!
Those docs state: "Windows Server 2016, Windows 10 LTSB 2016 and Windows 10 version 1607: so if I use kernel32 does that mean it will fail on the above Windows Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David, I am fine with your change in its current form (if you want to reshape it, I'll look again). I see no other concerns.
@dholmes-ora This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 203 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Let’s go with |
Thanks again for the reviews. /integrate |
Going to push as commit 9f3c7e7.
Your commit was automatically rebased without conflicts. |
@dholmes-ora Pushed as commit 9f3c7e7. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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:
Testing:
Thanks,
David
Progress
Issue
Reviewers
Contributors
<markus.gaisbauer@dynatrace.com>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4297/head:pull/4297
$ git checkout pull/4297
Update a local copy of the PR:
$ git checkout pull/4297
$ git pull https://git.openjdk.java.net/jdk pull/4297/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4297
View PR using the GUI difftool:
$ git pr show -t 4297
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4297.diff