New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 828149 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Reduce unnecessary IO thread hops by removing TaskManagerIoThreadHelper

Project Member Reported by chongz@chromium.org, Apr 2 2018

Issue description

Background:
With https://crrev.com/c/978607 |WebContentsEntry::ResourceLoadComplete()| will have data available on UI thread, and it's probably unnecessary to do the counting on IO thread.

See nick@'s comments:
https://crrev.com/c/978607/11/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc#265
```
Posting to the IO thread seems unnecessary here, since the data is ultimately going to be accumulated on the IO thread and sent back here to the UI thread, so that it can be looked up and delivered back to the WebContentsEntry! (via WebContentsEntry::GetTaskForFrame()). The accumulation behavior is intended just as an optimization to avoid too many PostTasks, when we were getting notified continuously on the IO thread.

Here, we already have easy access to the task manager Task/TaskGroup that needs to receive the data. We can just write it there directly.

Here's what we can do:

 - Remove TaskManagerIoThreadHelper altogether, and the static methods of TaskManagerInterface and TaskManagerImpl that exist to interact with it.
 - This method just becomes: 

void WebContentsEntry::ResourceLoadComplete(
    RenderFrameHost* render_frame_host,
    const content::mojom::ResourceLoadInfo& resource_load_info) {
  Task* task = GetTaskForFrame(render_frame_host);
  task->OnNetworkBytesRead(resource_load_info.encoded_data_received_length);
  task->OnNetworkBytesSent(resource_load_info.encoded_data_sent_length);
}

 - The cases in TaskManagerIoThreadHelperTest are potentially worth preserving, at least in spirit (I like how they mock out time), but we'll need to make them test work differently. The test harness recipe that might work would be something like:
 - Use the BrowserWithTestWindowTest framework (so that we can create [Test]WebContentses and [Test]RenderFrameHosts that don't actually spawn renderer processes.
 - Create a  MockWebContentsTaskManager (so that we can create task_manager Tasks without a TaskManagerImpl).
 - We deliver some events via direct calls to ResourceLoadComplete.
 - We manually do Refresh() on the Tasks to compute a rate.
 - Assertions could be made against Task::network_usage_rate()
```

 
Labels: Hotlist-DesktopUIToolingRequired Hotlist-DesktopUIChecked
***Mass UI Triage***

Sign in to add a comment