base::FindThreadID does an unreasonable number of allocations |
||
Issue descriptionThis is called in the browser process on behalf of the renderer. It seems to be setting the renderer's I/O thread priority. The summary: Opens the directory /proc/XXX/task - Iterates through all files in that directory (base/linux_util.cc GetTasksForProcess) - Converts the names to numbers (strtoul) and puts to result into a vector. - Iterates through the vector and generates names /proc/XXX/task/YYY/status - Reads each one of those into a string. - Parses that file by converting each line into a pair of strings split on a colon and puts them into a vector - Iterates through the vector looking for "NSpid" - Splits that result on a tab to get a pair of integers - Returns the second one. At the very least, the parsing should be optimized to avoid allocating so much.
,
Apr 7 2017
,
Apr 7 2017
I think we should start by making the first step of checking for the existence of directory /proc/XXX/task optimal as that's the only part that will execute on all devices. The rest is only executed on the Chromebook Plus with big.LITTLE support. We should optimize that part too but the priority of that might be slightly less as it's limited to a single device today.
,
Apr 7 2017
I'm not sure I follow about /proc/XXX/task checking. I have those directories on my desktop Linux box.
,
Apr 7 2017
On my desktop Linux, all of this code runs, but the check for "NSpid" is never successful.
,
Apr 7 2017
Given that this seems to be called a nontrivial amount of time and the IDs are relatively stable, a cache of IDs would be a good thing to explore (it could hang off of the RenderProcessHost). But the re-usability of IDs makes this ahrd to do easily. In some cases the renderer might be able to generate a unique key for a thread that won't get re-used. If this isn't practical, at least we can use the last known "external" task ID for the thread and verify that the renderer-internal one still matches. If that check fails, fall back to iterating through all tasks like we do now. Ideas for improving the parsing code: Do the work as we iterate through the tasks for the process so we can stop iterating when we find a match rather than going through all items in /proc/XXX/task directory. Not super high impact, but easy to do if the GetTasksForProcess was changed to have an iterator-like interface instead (which will make future code along these lines easier to write efficiently too). Search for "\nNSpid" in the file string without converting the entire thing to a vector of string pairs.
,
May 12 2017
Any updates on this? We should fix this soon.
,
May 13 2017
https://codereview.chromium.org/2882583003 should provide some improvements but still keep the code easy to read.
,
May 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/293c72b48fb321d33d6b06147807de9e4000924e commit 293c72b48fb321d33d6b06147807de9e4000924e Author: reveman <reveman@chromium.org> Date: Tue May 23 22:04:23 2017 base: Avoid unnecessary allocations in base::FindThreadID. Improve parsing by using StringTokenizer. BUG=708765 Review-Url: https://codereview.chromium.org/2882583003 Cr-Commit-Position: refs/heads/master@{#474077} [modify] https://crrev.com/293c72b48fb321d33d6b06147807de9e4000924e/base/linux_util.cc
,
May 24 2017
Chrome crashes in base::FindThreadID() with 293c72b48fb321d33d6b06147807de9e4000924e [2513:2533:0524/102232.401231:FATAL:linux_util.cc(212)] Check failed: split_value_str.size() >= 3u (2 vs. 3) #0 0x2b4989f7b24c base::debug::StackTrace::StackTrace() #1 0x2b4989f9f931 logging::LogMessage::~LogMessage() #2 0x2b4989f9d6fc base::FindThreadID() #3 0x2b498c3ea767 content::RenderMessageFilter::SetThreadPriorityOnFileThread() #4 0x2b4989f66719 _ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv #5 0x2b4989f7bbe4 base::debug::TaskAnnotator::RunTask() #6 0x2b4989fac0f9 base::MessageLoop::RunTask() #7 0x2b4989fac3bb base::MessageLoop::DeferOrRunPendingTask() #8 0x2b4989fac7bd base::MessageLoop::DoWork() #9 0x2b4989fae0e9 base::MessagePumpDefault::Run() #10 0x2b4989fabd7b base::MessageLoop::Run() #11 0x2b4989fe14ea base::RunLoop::Run() #12 0x2b498a01ed97 base::Thread::Run() #13 0x2b498c0fb7c8 content::BrowserThreadImpl::FileUserBlockingThreadRun() #14 0x2b498c0fbaaf content::BrowserThreadImpl::Run() #15 0x2b498a01f29d base::Thread::ThreadMain() #16 0x2b498a016f9f base::(anonymous namespace)::ThreadFunc() #17 0x2b4989cb6184 start_thread #18 0x2b4996042bed clone
,
May 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7ba6d9f9c06b206a69593c141daf2164f3ca5322 commit 7ba6d9f9c06b206a69593c141daf2164f3ca5322 Author: reveman <reveman@chromium.org> Date: Wed May 24 15:36:45 2017 Revert of base: Avoid unnecessary allocations in base::FindThreadID. (patchset #2 id:20001 of https://codereview.chromium.org/2882583003/ ) Reason for revert: Causing crash: [FATAL:linux_util.cc(212)] Check failed: split_value_str.size() >= 3u (2 vs. 3) Original issue's description: > base: Avoid unnecessary allocations in base::FindThreadID. > > Improve parsing by using StringTokenizer. > > BUG=708765 > > Review-Url: https://codereview.chromium.org/2882583003 > Cr-Commit-Position: refs/heads/master@{#474077} > Committed: https://chromium.googlesource.com/chromium/src/+/293c72b48fb321d33d6b06147807de9e4000924e TBR=brettw@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=708765 Review-Url: https://codereview.chromium.org/2900323002 Cr-Commit-Position: refs/heads/master@{#474297} [modify] https://crrev.com/7ba6d9f9c06b206a69593c141daf2164f3ca5322/base/linux_util.cc
,
Aug 30 2017
Ping on this. I still see this getting called on my Linux desktop. I see it ~3 times for each renderer started. On my system, this function never succeeds as "NSpid" is never found in the proc files, but it does a bunch of work anyway. I notice some effort was made to optimize the parsing but it was reverted. Can you please put that back, and also see if we can avoid calling this altogether when the NSpid fields won't be present?
,
Sep 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e5aece57cbbf13af34d28a5e352e904f23a9146e commit e5aece57cbbf13af34d28a5e352e904f23a9146e Author: reveman <reveman@chromium.org> Date: Mon Sep 04 07:18:33 2017 Re-land: base: Avoid unnecessary allocations in base::FindThreadID. Improve parsing by using StringTokenizer. BUG=708765 Review-Url: https://chromiumcodereview.appspot.com/2882583003 Cr-Commit-Position: refs/heads/master@{#499448} [modify] https://crrev.com/e5aece57cbbf13af34d28a5e352e904f23a9146e/base/linux_util.cc
,
Sep 4 2017
Re-landed the patch after fixing the DCHECK that caused it to be reverted. DCHECK assumed that it would only be called for PID namespace sandboxed processes. There's probably more optimizations that can be made to this code but I'm not sure it's worth optimizing for NSpid not existing as all 4.1 and later kernels will have NSpid. I looked at eliminating the vector and strtoul but I'm not sure that's worth much. I also looked a reversing the order we iterate over tasks in the hope that that would increase the hit rate but it doesn't seem like that's the case. |
||
►
Sign in to add a comment |
||
Comment 1 by brettw@chromium.org
, Apr 5 2017