Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713773002:
ShowThreadNames tool to get thread ID/name pairs in a Chrome process (Closed)
DescriptionShowThreadNames tool to get thread ID/name pairs in a Chrome process
This tool is designed to test the usage of the SetThreadDescription
WinAPI in Chrome. In Chrome, the SetThreadDescription API has been
enabled to set thread names. However, since there is no tool support to
retrieve thread names set by GetThreadDescription, we will still rely on
SetNameInternal function in platform_thread_win.cc to set thread names.
Despite this, we need a tool to demo the SetThreadDescription API works,
even without the debugger to be present.
This tool incorporates the GetThreadDescription API trying to get names
of all threads in a process specified by its ID. If the thread names
have been set by SetThreadDescription API call like in Chrome, all
thread ID/name pairs are returned.
BUG=684203
Review-Url: https://codereview.chromium.org/2713773002
Cr-Commit-Position: refs/heads/master@{#452617}
Committed: https://chromium.googlesource.com/chromium/src/+/b97c0922116dbeb62d81790daf475495a399d264
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address CR feedback. #
Total comments: 10
Patch Set 3 : Add support for getting error msg. #
Total comments: 10
Patch Set 4 : Fix nits. #
Messages
Total messages: 54 (44 generated)
Description was changed from ========== Add ShowThreadNames tool. BUG=684203 ========== to ========== ShowThreadNames tool to get thread ID/name pairs in a Chrome process This tool is designed to test the usage of the SetThreadDescription WinAPI in Chrome. In Chrome, the SetThreadDescription API has been enabled to set thread names. However, since there is no tool support to retrieve thread names set by GetThreadDescription, we will still rely on SetNameInternal function in platform_thread_win.cc to set thread names. Despite this, we need a tool to demo the SetThreadDescription API works, even without the debugger to be present. This tool incorporates the GetThreadDescription API trying to get names of all threads in a process specified by its ID. If the thread names have been set by SetThreadDescription API call like in Chrome, all thread ID/name pairs are returned. BUG=684203 ==========
chengx@chromium.org changed reviewers: + brucedawson@chromium.org
Patchset #3 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
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...
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
chengx@chromium.org changed reviewers: + stanisc@chromium.org
PTAL~
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 description needs a bit of scrubbing, and you also need to add a .gitignore file so that "git status" will be clean after loading and building the solution. But, it looks cool! https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... File tools/win/ShowThreadNames/ReadMe.txt (right): https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ReadMe.txt:7: demo the SetThreadDescription API works, even without the debugger to be present. One character too long... There are a few others that are one character too long later on. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ReadMe.txt:22: [Complie the code] Typo https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ReadMe.txt:27: Unicode Character Set" in "Character Set" section. This should be stored in the project file so this shouldn't be needed. It should just work. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ReadMe.txt:30: Run "ShowThreadNames.exe" either from the build direcotory or from Visual Studio. directory https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:62: if (!kernel32dll) { I'd skip this error checking 'cause kernel32.dll will always be present. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:99: OpenThread(THREAD_ALL_ACCESS, FALSE, te32.th32ThreadID); You can probably use THREAD_QUERY_LIMITED_INFORMATION or THREAD_QUERY_INFORMATION https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:115: std::multimap<std::wstring, DWORD> name_id_map = flip_map(id_name_map); If you're going to put it in a multimap<wstring,DWORD> you should just do that directly instead of putting it in first in a multimap<DWORD,wstring>. Or, alternately, append the pairs to a std::vector and then sort it. For performance critical code this would be better because of much better memory access patterns, but for this it is highly irrelevant, so do whichever is simpler. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:124: CloseHandle(hThreadSnap); For tidiness consider closing the handle as soon as you're done with it.
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
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...
https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:21: // The GetThreadDescription API is available since Windows 10, version 1607. Perhaps you should mention why you have to bind to the function this way rather than just use the SDK. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:76: HANDLE hThreadSnap = INVALID_HANDLE_VALUE; I'd suggest to not use Hungarian notation i.e. handle variable starting with h. Use Chrome code naming convention instead. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:114: // reverse the map so now it is sorted by thread name. Nit: comments are supposed to be complete sentences - start with a capital letter and end with a period. This applies to some other comments as well.
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
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...
Patchset #2 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I have addressed all the comments in the new patch set. PTAL~ https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... File tools/win/ShowThreadNames/ReadMe.txt (right): https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ReadMe.txt:7: demo the SetThreadDescription API works, even without the debugger to be present. On 2017/02/23 01:57:26, brucedawson wrote: > One character too long... There are a few others that are one character too long > later on. Done. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ReadMe.txt:22: [Complie the code] On 2017/02/23 01:57:26, brucedawson wrote: > Typo Done. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ReadMe.txt:27: Unicode Character Set" in "Character Set" section. On 2017/02/23 01:57:26, brucedawson wrote: > This should be stored in the project file so this shouldn't be needed. It should > just work. Done. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ReadMe.txt:30: Run "ShowThreadNames.exe" either from the build direcotory or from Visual Studio. On 2017/02/23 01:57:26, brucedawson wrote: > directory Done. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:21: // The GetThreadDescription API is available since Windows 10, version 1607. On 2017/02/23 02:29:59, stanisc wrote: > Perhaps you should mention why you have to bind to the function this way rather > than just use the SDK. Done. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:62: if (!kernel32dll) { On 2017/02/23 01:57:26, brucedawson wrote: > I'd skip this error checking 'cause kernel32.dll will always be present. Done. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:76: HANDLE hThreadSnap = INVALID_HANDLE_VALUE; On 2017/02/23 02:29:59, stanisc wrote: > I'd suggest to not use Hungarian notation i.e. handle variable starting with h. > Use Chrome code naming convention instead. Changed to threadSnap. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:99: OpenThread(THREAD_ALL_ACCESS, FALSE, te32.th32ThreadID); On 2017/02/23 01:57:26, brucedawson wrote: > You can probably use THREAD_QUERY_LIMITED_INFORMATION or > THREAD_QUERY_INFORMATION Done. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:114: // reverse the map so now it is sorted by thread name. On 2017/02/23 02:29:59, stanisc wrote: > Nit: comments are supposed to be complete sentences - start with a capital > letter and end with a period. This applies to some other comments as well. Done. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:115: std::multimap<std::wstring, DWORD> name_id_map = flip_map(id_name_map); On 2017/02/23 01:57:26, brucedawson wrote: > If you're going to put it in a multimap<wstring,DWORD> you should just do that > directly instead of putting it in first in a multimap<DWORD,wstring>. > > Or, alternately, append the pairs to a std::vector and then sort it. For > performance critical code this would be better because of much better memory > access patterns, but for this it is highly irrelevant, so do whichever is > simpler. I will use multimap<wstring,DWORD> directly. https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNam... tools/win/ShowThreadNames/ShowThreadNames.cc:124: CloseHandle(hThreadSnap); On 2017/02/23 01:57:26, brucedawson wrote: > For tidiness consider closing the handle as soon as you're done with it. Done.
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
Patchset #2 (id:120001) has been deleted
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 checked by chengx@chromium.org to run a CQ dry run
Patchset #3 (id:160001) has been deleted
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.
lgtm with some nits https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:24: // in Chrome. Binding SetThreadDescription API in Chrome can only be done via You could just say that this API isn't yet available in the SDK that Chrome builds with. https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:61: HANDLE threadSnap = INVALID_HANDLE_VALUE; How about thread_snapshot? https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:82: if (te32.th32OwnerProcessID == dwOwnerPID) { Use chrome code naming convention for this as well (e.g. owner_pid) https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:89: std::wstring data_w(data); Could you use a more descriptive name such as thread_name or thread_description? Also there is no need to use _w suffix. https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:103: for (auto it = name_id_map.begin(); it != name_id_map.end(); ++it) { Nit: you could use C++ 11 foreach loop here - for (auto it : name_id_map)
https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:24: // The reason why this API is binded in this way rather than just using the binded -> bound https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:27: // the system dll, rather than SDK. "can only be done by GetProcAddress, rather than the import library." https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:32: unsigned int processId; Should use DWORD as type for consistency. https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:93: name_id_map.insert(std::make_pair(data_w, te32.th32ThreadID)); Don't forget to free the text with LocalFree, per the documentation.
lgtm with some minor nits. https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:137: } Put a line terminator on the last line.
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
The patchset sent to the CQ was uploaded after l-g-t-m from stanisc@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2713773002/#ps200001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have addressed all the feedback in the new patch set. Thanks for reviewing this CL! https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:24: // in Chrome. Binding SetThreadDescription API in Chrome can only be done via On 2017/02/23 19:19:44, stanisc wrote: > You could just say that this API isn't yet available in the SDK that Chrome > builds with. Done. https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:61: HANDLE threadSnap = INVALID_HANDLE_VALUE; On 2017/02/23 19:19:44, stanisc wrote: > How about thread_snapshot? Done. https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:82: if (te32.th32OwnerProcessID == dwOwnerPID) { On 2017/02/23 19:19:44, stanisc wrote: > Use chrome code naming convention for this as well (e.g. owner_pid) Done. https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:89: std::wstring data_w(data); On 2017/02/23 19:19:44, stanisc wrote: > Could you use a more descriptive name such as thread_name or thread_description? > Also there is no need to use _w suffix. Done. https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:103: for (auto it = name_id_map.begin(); it != name_id_map.end(); ++it) { On 2017/02/23 19:19:44, stanisc wrote: > Nit: you could use C++ 11 foreach loop here - for (auto it : name_id_map) Done. https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:24: // The reason why this API is binded in this way rather than just using the On 2017/02/23 19:33:11, brucedawson wrote: > binded -> bound Done. https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:27: // the system dll, rather than SDK. On 2017/02/23 19:33:12, brucedawson wrote: > "can only be done by GetProcAddress, rather than the import library." Done. https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:32: unsigned int processId; On 2017/02/23 19:33:11, brucedawson wrote: > Should use DWORD as type for consistency. Done. https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:93: name_id_map.insert(std::make_pair(data_w, te32.th32ThreadID)); On 2017/02/23 19:33:11, brucedawson wrote: > Don't forget to free the text with LocalFree, per the documentation. Done. https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNa... tools/win/ShowThreadNames/ShowThreadNames.cc:137: } On 2017/02/23 19:37:32, brucedawson wrote: > Put a line terminator on the last line. Done.
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1487882605555700,
"parent_rev": "daca12c6491bf7196c2fc07a1c10cc347341ad21", "commit_rev":
"b97c0922116dbeb62d81790daf475495a399d264"}
Message was sent while issue was closed.
Description was changed from ========== ShowThreadNames tool to get thread ID/name pairs in a Chrome process This tool is designed to test the usage of the SetThreadDescription WinAPI in Chrome. In Chrome, the SetThreadDescription API has been enabled to set thread names. However, since there is no tool support to retrieve thread names set by GetThreadDescription, we will still rely on SetNameInternal function in platform_thread_win.cc to set thread names. Despite this, we need a tool to demo the SetThreadDescription API works, even without the debugger to be present. This tool incorporates the GetThreadDescription API trying to get names of all threads in a process specified by its ID. If the thread names have been set by SetThreadDescription API call like in Chrome, all thread ID/name pairs are returned. BUG=684203 ========== to ========== ShowThreadNames tool to get thread ID/name pairs in a Chrome process This tool is designed to test the usage of the SetThreadDescription WinAPI in Chrome. In Chrome, the SetThreadDescription API has been enabled to set thread names. However, since there is no tool support to retrieve thread names set by GetThreadDescription, we will still rely on SetNameInternal function in platform_thread_win.cc to set thread names. Despite this, we need a tool to demo the SetThreadDescription API works, even without the debugger to be present. This tool incorporates the GetThreadDescription API trying to get names of all threads in a process specified by its ID. If the thread names have been set by SetThreadDescription API call like in Chrome, all thread ID/name pairs are returned. BUG=684203 Review-Url: https://codereview.chromium.org/2713773002 Cr-Commit-Position: refs/heads/master@{#452617} Committed: https://chromium.googlesource.com/chromium/src/+/b97c0922116dbeb62d81790daf47... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:200001) as https://chromium.googlesource.com/chromium/src/+/b97c0922116dbeb62d81790daf47... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
