Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692213003:
Use Windows 10 thread naming API, SetThreadDescription (Closed)
DescriptionUse Windows 10 thread naming API, SetThreadDescription
Starting from version 1607 of Windows 10, there is a SetThreadDescription
API for setting thread names. This API has several advantages over
the SetNameInternal function which is currently used to set thread
names in Chrome.
The only issue with this new API is that there is no tool support
for it yet. However, once tool support catches up, it should give
a better thread naming experience.
A command-line tool "ShowThreadNames" for testing this
SetThreadDescription API has been developed and checked in using
another CL. It can be found via crrev.com/2713773002.
BUG=684203
Review-Url: https://codereview.chromium.org/2692213003
Cr-Commit-Position: refs/heads/master@{#452653}
Committed: https://chromium.googlesource.com/chromium/src/+/f6988f8ff1fb0a3d8e5f0f9cc798f7f971a1baaa
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address comments. #
Total comments: 3
Patch Set 3 : Address nits. #
Total comments: 6
Patch Set 4 : Fix nits. #Messages
Total messages: 39 (31 generated)
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
You should put together a test app that prints all thread names, as a demo. This will also help us identify any unnamed threads. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:35: typedef HRESULT(WINAPI* SETTHREADDESCRIPTION)(HANDLE hThread, Somewhere you need a comment saying which version of Windows 10 brought in this feature. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:185: return; Oops! This will return early in those cases where SetThreadDescription would be helpful! You might want to update the comment, or mention elsewhere that SetThreadDescription works even if no debugger is attached. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:187: HMODULE kernal32dll = ::GetModuleHandle(L"Kernel32.dll"); Spelling - should be kernel32dll. Or "kernel32". Or, can call GetModuleHandle() inline and avoid the temporary entirely. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:189: SETTHREADDESCRIPTION set_thread_description_ptr = Consider using 'auto' for the type - no need to specify the type on the left and right of the equals sign. I'd also ditch "_ptr" - it doesn't add value, IMHO. Other uses of GetProcAddress use a _func suffix or no suffix. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:193: std::wstring name_wstring = base::UTF8ToWide(name_string); The initialization of name_wstring should be inside the final pointer check so it doesn't happen unless necessary. Also, is the name_string temporary needed?
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Test setThreadDescription Windows API. BUG=684203 ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. SetNameInternal in platform_thread_win.cc may be retired then. Another command line tool testing this SetNameInternal has been developed and will be checked in using another CL. BUG=684203 ==========
PTAL~ I think it is better to check in the tool using another CL. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:35: typedef HRESULT(WINAPI* SETTHREADDESCRIPTION)(HANDLE hThread, On 2017/02/21 22:12:40, brucedawson wrote: > Somewhere you need a comment saying which version of Windows 10 brought in this > feature. Done. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:185: return; On 2017/02/21 22:12:40, brucedawson wrote: > Oops! This will return early in those cases where SetThreadDescription would be > helpful! > > You might want to update the comment, or mention elsewhere that > SetThreadDescription works even if no debugger is attached. Done. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:187: HMODULE kernal32dll = ::GetModuleHandle(L"Kernel32.dll"); On 2017/02/21 22:12:40, brucedawson wrote: > Spelling - should be kernel32dll. Or "kernel32". Or, can call GetModuleHandle() > inline and avoid the temporary entirely. Done. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:189: SETTHREADDESCRIPTION set_thread_description_ptr = On 2017/02/21 22:12:40, brucedawson wrote: > Consider using 'auto' for the type - no need to specify the type on the left and > right of the equals sign. I'd also ditch "_ptr" - it doesn't add value, IMHO. > Other uses of GetProcAddress use a _func suffix or no suffix. Done. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:193: std::wstring name_wstring = base::UTF8ToWide(name_string); On 2017/02/21 22:12:40, brucedawson wrote: > The initialization of name_wstring should be inside the final pointer check so > it doesn't happen unless necessary. Also, is the name_string temporary needed? Done.
You should also update the description to fix the one line that is too long, and probably remove claims that SetNameInternal will be removed once tool support catches up. Otherwise, lgtm (but I am not an owner). https://codereview.chromium.org/2692213003/diff/40001/base/threading/platform... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2692213003/diff/40001/base/threading/platform... base/threading/platform_thread_win.cc:182: // tool support cataches up, e.g., the debugger in Visual Studio, the We won't be retiring the RaiseException technique anytime soon because we still want to be able to debug on older versions of Windows. https://codereview.chromium.org/2692213003/diff/40001/base/threading/platform... base/threading/platform_thread_win.cc:185: if (kernel32dll) { Note that this check can be omitted. It often is in Chrome, because kernel32.dll will *always* be present.
Description was changed from ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. SetNameInternal in platform_thread_win.cc may be retired then. Another command line tool testing this SetNameInternal has been developed and will be checked in using another CL. BUG=684203 ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. Another command line tool testing this SetNameInternal has been developed and will be checked in using another CL. BUG=684203 ==========
Description was changed from ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. Another command line tool testing this SetNameInternal has been developed and will be checked in using another CL. BUG=684203 ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. Another command line tool testing this SetNameInternal has been developed and will be checked in using another CL. BUG=684203 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chengx@chromium.org changed reviewers: + gab@chromium.org
Hi gab@, I think you are the owner of this file. PTAL~ https://codereview.chromium.org/2692213003/diff/40001/base/threading/platform... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2692213003/diff/40001/base/threading/platform... base/threading/platform_thread_win.cc:182: // tool support cataches up, e.g., the debugger in Visual Studio, the On 2017/02/22 21:36:10, brucedawson wrote: > We won't be retiring the RaiseException technique anytime soon because we still > want to be able to debug on older versions of Windows. Done.
lgtm w/ comments, thanks! https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform... base/threading/platform_thread_win.cc:37: PCWSTR lpThreadDescription); Prefer PascalCasing for SetThreadDescription https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform... base/threading/platform_thread_win.cc:186: base::StringPiece name_string(name); This is unnecessary (std::string will be implicitly converted to StringPiece if you pass |name| inline below) https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform... base/threading/platform_thread_win.cc:187: set_thread_description_func(GetCurrentThread(), ::GetCurrentThread() (prefer highlighting non-Chromium calls with explicit global namespace prefix -- helps readability)
Description was changed from ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. Another command line tool testing this SetNameInternal has been developed and will be checked in using another CL. BUG=684203 ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via https://codereview.chromium.org/2713773002 BUG=684203 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via https://codereview.chromium.org/2713773002 BUG=684203 ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via https://codereview.chromium.org/2713773002 BUG=684203 ==========
Description was changed from ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via https://codereview.chromium.org/2713773002 BUG=684203 ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command-line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via https://codereview.chromium.org/2713773002 BUG=684203 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2692213003/#ps80001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Gabriel and Bruce, I have addressed all your feedback in the new patch set. Thanks for the review! https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform... base/threading/platform_thread_win.cc:37: PCWSTR lpThreadDescription); On 2017/02/23 20:03:24, gab wrote: > Prefer PascalCasing for SetThreadDescription Done. https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform... base/threading/platform_thread_win.cc:186: base::StringPiece name_string(name); On 2017/02/23 20:03:24, gab wrote: > This is unnecessary (std::string will be implicitly converted to StringPiece if > you pass |name| inline below) Done. https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform... base/threading/platform_thread_win.cc:187: set_thread_description_func(GetCurrentThread(), On 2017/02/23 20:03:24, gab wrote: > ::GetCurrentThread() > > (prefer highlighting non-Chromium calls with explicit global namespace prefix -- > helps readability) Done.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487887456922970,
"parent_rev": "8782ff08a3ed45882f707658d63cb25cf403187f", "commit_rev":
"f6988f8ff1fb0a3d8e5f0f9cc798f7f971a1baaa"}
Message was sent while issue was closed.
Description was changed from ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command-line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via https://codereview.chromium.org/2713773002 BUG=684203 ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command-line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via https://codereview.chromium.org/2713773002 BUG=684203 Review-Url: https://codereview.chromium.org/2692213003 Cr-Commit-Position: refs/heads/master@{#452653} Committed: https://chromium.googlesource.com/chromium/src/+/f6988f8ff1fb0a3d8e5f0f9cc798... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f6988f8ff1fb0a3d8e5f0f9cc798...
Message was sent while issue was closed.
Description was changed from ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command-line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via https://codereview.chromium.org/2713773002 BUG=684203 Review-Url: https://codereview.chromium.org/2692213003 Cr-Commit-Position: refs/heads/master@{#452653} Committed: https://chromium.googlesource.com/chromium/src/+/f6988f8ff1fb0a3d8e5f0f9cc798... ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command-line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via crrev.com/2713773002. BUG=684203 Review-Url: https://codereview.chromium.org/2692213003 Cr-Commit-Position: refs/heads/master@{#452653} Committed: https://chromium.googlesource.com/chromium/src/+/f6988f8ff1fb0a3d8e5f0f9cc798... ==========
Message was sent while issue was closed.
Description was changed from ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command-line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via crrev.com/2713773002. BUG=684203 Review-Url: https://codereview.chromium.org/2692213003 Cr-Commit-Position: refs/heads/master@{#452653} Committed: https://chromium.googlesource.com/chromium/src/+/f6988f8ff1fb0a3d8e5f0f9cc798... ========== to ========== Use Windows 10 thread naming API, SetThreadDescription Starting from version 1607 of Windows 10, there is a SetThreadDescription API for setting thread names. This API has several advantages over the SetNameInternal function which is currently used to set thread names in Chrome. The only issue with this new API is that there is no tool support for it yet. However, once tool support catches up, it should give a better thread naming experience. A command-line tool "ShowThreadNames" for testing this SetThreadDescription API has been developed and checked in using another CL. It can be found via crrev.com/2713773002. BUG=684203 Review-Url: https://codereview.chromium.org/2692213003 Cr-Commit-Position: refs/heads/master@{#452653} Committed: https://chromium.googlesource.com/chromium/src/+/f6988f8ff1fb0a3d8e5f0f9cc798... ========== |
