BrowserThread::GetTaskRunnerForThread returns an inefficient proxy when it doesn't need to |
||
Issue descriptionSince people can request the FILE thread task runner before the FILE thread exists a proxy was created around the actual task runner when using the BrowserThread::GetTaskRunnerForThread API. https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?type=cs&sq=package:chromium&l=676 These proxies implement RunsTasksInCurrentSequence via BrowserThread::CurrentlyOn: https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?type=cs&sq=package:chromium&l=79 It turns out there's a global lock in the implementation of that function: https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?type=cs&sq=package:chromium&l=573 If, on the other hand, you grab the current sequenced task runner via SequencedTaskRunnerHandle::Get, you'll get the actual task runner, which implements RunsTasksInCurrentSequence as: https://cs.chromium.org/chromium/src/base/message_loop/message_loop_task_runner.cc?q=MessageLoopTaskRunner&dr=CSs&l=46 This uses a per sequence lock, which is likely more efficient. A couple suggestions: (1) Don't create proxies for threads that DO already exist (ie, the UI thread); rather, just get a reference to the existing task runner. (2) Make the proxies 'temporary'; once the underlying task runner actually exists, have them forward calls directly to it? (3) Make the MessageLoopTaskRunners use atomics for the thread ID? And if the message loop task runners can exist before the threads exist, why the proxies at all? Why not just directly create the message loop task runners? (This came up after profiling shows we spend half of the early startup ModuleDatabase processing time waiting for and acquiring the global browser thread lock.)
,
Mar 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ed29db5e854f3f4c08029fa6ca4088a52053045 commit 9ed29db5e854f3f4c08029fa6ca4088a52053045 Author: Gabriel Charette <gab@chromium.org> Date: Tue Mar 27 19:48:00 2018 Make BrowserThreadImpl lock-less Sampling profiler highlights high contention on this lock: crbug.com/821034 . Initializing/tear down was already single-threaded (tear down had to be moved from BrowserProcessSubThread::Cleanup() to ~BrowserProcessSubThread() to enforce this with a ThreadChecker however). Add dchecks to confirm/enforce this and remove the lock which is usuless on data structures that are now (as of series of changes over the last year) read-only after startup. Use atomics to protect reads across the RUNNING=>SHUTDOWN boundary. Bug: 821034 Change-Id: I7800048bff51ad79cb10ee89fd3a0a31534c393e Reviewed-on: https://chromium-review.googlesource.com/973556 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#546212} [modify] https://crrev.com/9ed29db5e854f3f4c08029fa6ca4088a52053045/content/browser/browser_process_sub_thread.cc [modify] https://crrev.com/9ed29db5e854f3f4c08029fa6ca4088a52053045/content/browser/browser_thread_impl.cc [modify] https://crrev.com/9ed29db5e854f3f4c08029fa6ca4088a52053045/content/browser/browser_thread_impl.h
,
Mar 28 2018
BrowserThread is lock-less now, I may remove the proxies altogether at some point (if we can somehow get to the point where no one grabs a proxy before the threads are initialized) but right now they're merely single hop proxies to the BrowserThread:: interface and shouldn't be inefficient anymore :) |
||
►
Sign in to add a comment |
||
Comment 1 by gab@chromium.org
, Aug 10 2017