Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(610)

Issue 2692213003: Use Windows 10 thread naming API, SetThreadDescription (Closed)

Created:
3 years, 5 months ago by chengx
Modified:
3 years, 5 months ago
Reviewers:
gab, brucedawson
CC:
chromium-reviews, vmpstr+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -0 lines) Patch
M base/threading/platform_thread_win.cc View 1 2 3 3 chunks +14 lines, -0 lines 0 comments Download

Messages

Total messages: 39 (31 generated)
brucedawson
You should put together a test app that prints all thread names, as a demo. ...
3 years, 5 months ago (2017-02-21 22:12:40 UTC) #2
chengx
PTAL~ I think it is better to check in the tool using another CL. https://codereview.chromium.org/2692213003/diff/1/base/threading/platform_thread_win.cc ...
3 years, 5 months ago (2017-02-22 21:24:34 UTC) #13
brucedawson
You should also update the description to fix the one line that is too long, ...
3 years, 5 months ago (2017-02-22 21:36:11 UTC) #14
chengx
Hi gab@, I think you are the owner of this file. PTAL~ https://codereview.chromium.org/2692213003/diff/40001/base/threading/platform_thread_win.cc File base/threading/platform_thread_win.cc ...
3 years, 5 months ago (2017-02-22 23:20:29 UTC) #22
gab
lgtm w/ comments, thanks! https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform_thread_win.cc File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2692213003/diff/60001/base/threading/platform_thread_win.cc#newcode37 base/threading/platform_thread_win.cc:37: PCWSTR lpThreadDescription); Prefer PascalCasing for ...
3 years, 5 months ago (2017-02-23 20:03:24 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2692213003/80001
3 years, 5 months ago (2017-02-23 22:05:13 UTC) #33
chengx
Hi Gabriel and Bruce, I have addressed all your feedback in the new patch set. ...
3 years, 5 months ago (2017-02-23 22:06:21 UTC) #34
commit-bot: I haz the power
3 years, 5 months ago (2017-02-23 22:14:50 UTC) #37
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f6988f8ff1fb0a3d8e5f0f9cc798...

Powered by Google App Engine
This is Rietveld 408576698