Make Task Manager sort more meaningful

Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG=616897, 54909, 239611, 606985, 527455
TESTS=unit_tests, browser_tests

Committed: https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802
Cr-Commit-Position: refs/heads/master@{#401328}

[ncarter (slow), 2016-05-31 18:45:31]

Description was changed from

==========
Task Manager sort improvements:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Subtasks (i.e. subframes) are grouped to be after their parents.
    - Sorting of non-subtasks is done by process type, followed
      by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.

BUG=
==========

to

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Subtasks (i.e. subframes) are grouped to be after their parents.
    - Sorting of non-subtasks is done by process type, followed
      by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.

BUG=
==========

[ncarter (slow), 2016-06-01 21:41:43]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Subtasks (i.e. subframes) are grouped to be after their parents.
    - Sorting of non-subtasks is done by process type, followed
      by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.

BUG=
==========

to

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - Subtasks (i.e. subframes) are grouped to be after their parents.
    - Sorting of non-subtasks is done by process type, followed
      by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.

BUG=
==========

[ncarter (slow), 2016-06-01 21:51:58]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - Subtasks (i.e. subframes) are grouped to be after their parents.
    - Sorting of non-subtasks is done by process type, followed
      by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.

BUG=
==========

to

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using a vector of Tasks (TaskGroups
      are almost always very small, and vectors are faster in
      practice than maps for small containers), which it
      exposes.

BUG=
TESTS=unit_tests, browser_tests
==========

[ncarter (slow), 2016-06-01 22:37:54]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using a vector of Tasks (TaskGroups
      are almost always very small, and vectors are faster in
      practice than maps for small containers), which it
      exposes.

BUG=
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.

BUG=
TESTS=unit_tests, browser_tests
==========

[ncarter (slow), 2016-06-02 21:18:11]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.

BUG=
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

[ncarter (slow), 2016-06-02 21:18:34]

nick@chromium.org changed reviewers:
+ afakhry@chromium.org

[ncarter (slow), 2016-06-02 21:18:50]

Ahmed, please review!

[ncarter (slow), 2016-06-02 21:19:44]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

[ncarter (slow), 2016-06-03 17:26:05]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process.
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

[afakhry, 2016-06-03 17:53:42]

Here are some preliminary comments. Overall, I like this CL. I also LOVE the
tests you added.
I just need some more time to dive deeply into the complicated algorithm in
GetTaskIdsList(). There're half a dozen containers involved in this algorithm
and lots of things going on. I admire its cleverness, but I need to apply this
patch locally and attempt to answer the following questions:
- Can we reduce the number of containers / operations?
- How long does it take to finish? Does it affect our UI? (Probably profile or
trace it).

Thanks for this change!

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:298:
std::unordered_set<TaskGroup*> visited_groups;
Do you need to #include <unordered_set>?

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:316: for (Task*
task : current_group_tasks) {
Nit: Remove curly braces.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:316: for (Task*
task : current_group_tasks) {
const auto& ?

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:327:
std::vector<const Task*>& children = children_it->second;
This children vector hides the outer children unordered_map. It's confusing. Can
you please rename it?

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:349: for (Task*
task : group->tasks()) {
Nit: the curly braces.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:349: for (Task*
task : group->tasks()) {
Also const auto& ?

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl_unittest.cc:171:
FakeTask* cycle3 = AddTask(601, Task::SANDBOX_HELPER, "Cycle 3", -1);
Same group but different PIDs?

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/ui/task...
File chrome/browser/ui/task_manager/task_manager_table_model.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/ui/task...
chrome/browser/ui/task_manager/task_manager_table_model.cc:425: return
ValueCompare(row1, row2);
Be prepared for a storm of regression bugs saying "sorting by tasks doesn't sort
alphabetically". Have you talked to UX about this?

[afakhry, 2016-06-06 18:11:59]

I took another look at your algorithm and I liked it. I also did some
measurements and determined that it's quite fast (even though I had only 15 tabs
and few more apps and extensions running).

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:259: return
std::make_tuple(!!a->GetParentTask(), a->GetType(), a->GetTabId(),
First, using a tuple is brilliant.

Second, how about adding a bool Task::HasParentTask() which encapsulates "return
!!GetParentTask();"?

The code here and below in several places will be nicer to look at.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:271:
std::unordered_map<const Task*, std::vector<const Task*>> children;
Optional: Tracking the children here using a priority queue saves you the sort
at line 328 and the reverse insert in line 329. Unfortunately, I had to get rid
of your awesome lamda for an old-fashioned functor :D. So:

struct Comparator {
  bool operator() (const Task* a, const Task* b) {
    return std::make_tuple(!!a->GetParentTask(), 
                           a->GetType(), 
                           a->GetTabId(),
                           a->task_id()) <
           std::make_tuple(!!b->GetParentTask(), 
                           b->GetType(), 
                           b->GetTabId(),
                           b->task_id());
   }
} comparator;

  - Then here at line 271:

using ChildrenQueue = std::priority_queue<const Task*,
                                          std::vector<const Task*>,
                                          Comparator>;
std::unordered_map<const Task*, ChildrenQueue> children;

  - line 283 becomes:

children[task->GetParentTask()].push(task);

  - finally lines 326~330 become something like:

if (children_it == children.end())
  continue;
while (!children_it->second.empty()) {
  tasks_to_visit.push_back(children_it->second.top());
  children_it->second.pop();
}


I measured both timings and they're similar. Up to you.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:302:
CHECK(!tasks_to_visit.empty());
I think a DCHECK() is enough here.

[ncarter (slow), 2016-06-08 21:37:50]

I'll come back to this patch tomorrow, just responding to a couple comments.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:260: a->task_id())
<
FYI using tuples for lexicographic comparison is a common C++11 idiom. I think
it's essentially why we have std::tie, and is listed as the example usage
here:http://en.cppreference.com/w/cpp/utility/tuple/tie.

std::tie is better than make_tuple, but it doesn't work here, since the args
aren't lvalues. I wonder if I can use std::tie after adding HasParentTask() as
you suggest.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:271:
std::unordered_map<const Task*, std::vector<const Task*>> children;
On 2016/06/06 18:11:59, afakhry wrote:
> Optional: Tracking the children here using a priority queue saves you the sort
> at line 328 and the reverse insert in line 329. Unfortunately, I had to get
rid
> of your awesome lamda for an old-fashioned functor :D. So:
> 
> struct Comparator {
>   bool operator() (const Task* a, const Task* b) {
>     return std::make_tuple(!!a->GetParentTask(), 
>                            a->GetType(), 
>                            a->GetTabId(),
>                            a->task_id()) <
>            std::make_tuple(!!b->GetParentTask(), 
>                            b->GetType(), 
>                            b->GetTabId(),
>                            b->task_id());
>    }
> } comparator;
> 
>   - Then here at line 271:
> 
> using ChildrenQueue = std::priority_queue<const Task*,
>                                           std::vector<const Task*>,
>                                           Comparator>;
> std::unordered_map<const Task*, ChildrenQueue> children;
> 
>   - line 283 becomes:
> 
> children[task->GetParentTask()].push(task);
> 
>   - finally lines 326~330 become something like:
> 
> if (children_it == children.end())
>   continue;
> while (!children_it->second.empty()) {
>   tasks_to_visit.push_back(children_it->second.top());
>   children_it->second.pop();
> }
> 
> 
> I measured both timings and they're similar. Up to you.

Interesting idea. They're essentially the same under the hood; it's just that we
use heapsort vs an explicit std::sort. I'll take a look at this with fresh eyes
tomorrow. [Note to self: before touching this, make sure the tests actually
fails if we omit the existing std::sort.]

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:327:
std::vector<const Task*>& children = children_it->second;
On 2016/06/03 17:53:42, afakhry wrote:
> This children vector hides the outer children unordered_map. It's confusing.
Can
> you please rename it?

YUCK. Thanks for pointing this out, it was an accident -- it was an artifact of
some last-minute renaming (originally the outer |children| was called |parents|,
but I thought calling it |children| read better.). I'm actually surprised this
collision didn't cause a compile error or functional bug!

[afakhry, 2016-06-09 13:33:53]

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:260: a->task_id())
<
On 2016/06/08 21:37:50, ncarter wrote:
> FYI using tuples for lexicographic comparison is a common C++11 idiom. I think
> it's essentially why we have std::tie, and is listed as the example usage
> here:http://en.cppreference.com/w/cpp/utility/tuple/tie.
> 
> std::tie is better than make_tuple, but it doesn't work here, since the args
> aren't lvalues. I wonder if I can use std::tie after adding HasParentTask() as
> you suggest.

Thanks for sharing! I liked the std::tie example.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:271:
std::unordered_map<const Task*, std::vector<const Task*>> children;
On 2016/06/08 21:37:50, ncarter wrote:
> On 2016/06/06 18:11:59, afakhry wrote:
> > Optional: Tracking the children here using a priority queue saves you the
sort
> > at line 328 and the reverse insert in line 329. Unfortunately, I had to get
> rid
> > of your awesome lamda for an old-fashioned functor :D. So:
> > 
> > struct Comparator {
> >   bool operator() (const Task* a, const Task* b) {
> >     return std::make_tuple(!!a->GetParentTask(), 
> >                            a->GetType(), 
> >                            a->GetTabId(),
> >                            a->task_id()) <
> >            std::make_tuple(!!b->GetParentTask(), 
> >                            b->GetType(), 
> >                            b->GetTabId(),
> >                            b->task_id());
> >    }
> > } comparator;
> > 
> >   - Then here at line 271:
> > 
> > using ChildrenQueue = std::priority_queue<const Task*,
> >                                           std::vector<const Task*>,
> >                                           Comparator>;
> > std::unordered_map<const Task*, ChildrenQueue> children;
> > 
> >   - line 283 becomes:
> > 
> > children[task->GetParentTask()].push(task);
> > 
> >   - finally lines 326~330 become something like:
> > 
> > if (children_it == children.end())
> >   continue;
> > while (!children_it->second.empty()) {
> >   tasks_to_visit.push_back(children_it->second.top());
> >   children_it->second.pop();
> > }
> > 
> > 
> > I measured both timings and they're similar. Up to you.
> 
> Interesting idea. They're essentially the same under the hood; it's just that
we
> use heapsort vs an explicit std::sort. I'll take a look at this with fresh
eyes
> tomorrow. [Note to self: before touching this, make sure the tests actually
> fails if we omit the existing std::sort.]

Yes, they're definitely the same algorithm. The suggested one aims to improve
readability.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:327:
std::vector<const Task*>& children = children_it->second;
On 2016/06/08 21:37:50, ncarter wrote:
> On 2016/06/03 17:53:42, afakhry wrote:
> > This children vector hides the outer children unordered_map. It's confusing.
> Can
> > you please rename it?
> 
> YUCK. Thanks for pointing this out, it was an accident -- it was an artifact
of
> some last-minute renaming (originally the outer |children| was called
|parents|,
> but I thought calling it |children| read better.). I'm actually surprised this
> collision didn't cause a compile error or functional bug!

I think you got lucky as you were only accessing |children| defined in the
inner-most scope and the compiler assumed you want to use it as well. If you had
wanted to user the outer one, that would have been a disaster! :D

[ncarter (slow), 2016-06-20 17:38:45]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Sorting by the "Tasks" column now arranges rows in model order, rather
   than alphabetically.
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This model order still has a UI problem where it's not visible
after the user sorts one of the columns. A follow-on CL will
address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

[ncarter (slow), 2016-06-20 17:39:32]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This model order still has a UI problem where it's not visible
after the user sorts one of the columns. A follow-on CL will
address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by one
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

[ncarter (slow), 2016-06-20 17:39:49]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by one
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

[ncarter (slow), 2016-06-20 17:40:00]

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:259: return
std::make_tuple(!!a->GetParentTask(), a->GetType(), a->GetTabId(),
On 2016/06/06 18:11:59, afakhry wrote:
> First, using a tuple is brilliant.
> 
> Second, how about adding a bool Task::HasParentTask() which encapsulates
"return
> !!GetParentTask();"?
> 
> The code here and below in several places will be nicer to look at.

Done.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:271:
std::unordered_map<const Task*, std::vector<const Task*>> children;
On 2016/06/09 13:33:53, afakhry wrote:
> On 2016/06/08 21:37:50, ncarter wrote:
> > On 2016/06/06 18:11:59, afakhry wrote:
> > > Optional: Tracking the children here using a priority queue saves you the
> sort
> > > at line 328 and the reverse insert in line 329. Unfortunately, I had to
get
> > rid
> > > of your awesome lamda for an old-fashioned functor :D. So:
> > > 
> > > struct Comparator {
> > >   bool operator() (const Task* a, const Task* b) {
> > >     return std::make_tuple(!!a->GetParentTask(), 
> > >                            a->GetType(), 
> > >                            a->GetTabId(),
> > >                            a->task_id()) <
> > >            std::make_tuple(!!b->GetParentTask(), 
> > >                            b->GetType(), 
> > >                            b->GetTabId(),
> > >                            b->task_id());
> > >    }
> > > } comparator;
> > > 
> > >   - Then here at line 271:
> > > 
> > > using ChildrenQueue = std::priority_queue<const Task*,
> > >                                           std::vector<const Task*>,
> > >                                           Comparator>;
> > > std::unordered_map<const Task*, ChildrenQueue> children;
> > > 
> > >   - line 283 becomes:
> > > 
> > > children[task->GetParentTask()].push(task);
> > > 
> > >   - finally lines 326~330 become something like:
> > > 
> > > if (children_it == children.end())
> > >   continue;
> > > while (!children_it->second.empty()) {
> > >   tasks_to_visit.push_back(children_it->second.top());
> > >   children_it->second.pop();
> > > }
> > > 
> > > 
> > > I measured both timings and they're similar. Up to you.
> > 
> > Interesting idea. They're essentially the same under the hood; it's just
that
> we
> > use heapsort vs an explicit std::sort. I'll take a look at this with fresh
> eyes
> > tomorrow. [Note to self: before touching this, make sure the tests actually
> > fails if we omit the existing std::sort.]
> 
> Yes, they're definitely the same algorithm. The suggested one aims to improve
> readability.

Thanks for this suggestion. My overall sense is that sorting in retrospect
captures the essence of what I'm trying to do a little better, since there are
no "pop min" operations during insertion.

I've made some other changes that hopefully make the directionality of the
sorting a little clearer, including using base::Reversed() for the |parent|
iteration.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:298:
std::unordered_set<TaskGroup*> visited_groups;
On 2016/06/03 17:53:42, afakhry wrote:
> Do you need to #include <unordered_set>?

Done.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:302:
CHECK(!tasks_to_visit.empty());
On 2016/06/06 18:11:59, afakhry wrote:
> I think a DCHECK() is enough here.

Done.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:316: for (Task*
task : current_group_tasks) {
On 2016/06/03 17:53:42, afakhry wrote:
> Nit: Remove curly braces.

Done.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:316: for (Task*
task : current_group_tasks) {
On 2016/06/03 17:53:42, afakhry wrote:
> const auto& ?

I prefer Task* here, since it makes it clearer to the reader what's being
iterated over. Unless there's a functional reason for this -- does "const auto&"
produce better code?

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:327:
std::vector<const Task*>& children = children_it->second;
On 2016/06/09 13:33:53, afakhry wrote:
> On 2016/06/08 21:37:50, ncarter wrote:
> > On 2016/06/03 17:53:42, afakhry wrote:
> > > This children vector hides the outer children unordered_map. It's
confusing.
> > Can
> > > you please rename it?
> > 
> > YUCK. Thanks for pointing this out, it was an accident -- it was an artifact
> of
> > some last-minute renaming (originally the outer |children| was called
> |parents|,
> > but I thought calling it |children| read better.). I'm actually surprised
this
> > collision didn't cause a compile error or functional bug!
> 
> I think you got lucky as you were only accessing |children| defined in the
> inner-most scope and the compiler assumed you want to use it as well. If you
had
> wanted to user the outer one, that would have been a disaster! :D

Done.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:349: for (Task*
task : group->tasks()) {
On 2016/06/03 17:53:42, afakhry wrote:
> Also const auto& ?

I prefer 'Task*' to 'const auto&' here; it's is fewer characters and makes it
clear what we're iterating over.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:349: for (Task*
task : group->tasks()) {
On 2016/06/03 17:53:42, afakhry wrote:
> Nit: the curly braces.

Done.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl_unittest.cc:171:
FakeTask* cycle3 = AddTask(601, Task::SANDBOX_HELPER, "Cycle 3", -1);
On 2016/06/03 17:53:42, afakhry wrote:
> Same group but different PIDs?

Good catch! Fixed.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/ui/task...
File chrome/browser/ui/task_manager/task_manager_table_model.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/ui/task...
chrome/browser/ui/task_manager/task_manager_table_model.cc:425: return
ValueCompare(row1, row2);
On 2016/06/03 17:53:42, afakhry wrote:
> Be prepared for a storm of regression bugs saying "sorting by tasks doesn't
sort
> alphabetically". Have you talked to UX about this?

I'll start a thread with UI folks. We definitely have a few options for ways to
expose the logical sorting.

In the mean time, I've removed this part of the diff (I think the tests will
still pass). We can expose this to the UI as a separate CL.

[afakhry, 2016-06-22 13:49:46]

lgtm. Thanks for this CL!

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:271:
std::unordered_map<const Task*, std::vector<const Task*>> children;
On 2016/06/20 17:39:59, ncarter wrote:
> On 2016/06/09 13:33:53, afakhry wrote:
> > On 2016/06/08 21:37:50, ncarter wrote:
> > > On 2016/06/06 18:11:59, afakhry wrote:
> > > > Optional: Tracking the children here using a priority queue saves you
the
> > sort
> > > > at line 328 and the reverse insert in line 329. Unfortunately, I had to
> get
> > > rid
> > > > of your awesome lamda for an old-fashioned functor :D. So:
> > > > 
> > > > struct Comparator {
> > > >   bool operator() (const Task* a, const Task* b) {
> > > >     return std::make_tuple(!!a->GetParentTask(), 
> > > >                            a->GetType(), 
> > > >                            a->GetTabId(),
> > > >                            a->task_id()) <
> > > >            std::make_tuple(!!b->GetParentTask(), 
> > > >                            b->GetType(), 
> > > >                            b->GetTabId(),
> > > >                            b->task_id());
> > > >    }
> > > > } comparator;
> > > > 
> > > >   - Then here at line 271:
> > > > 
> > > > using ChildrenQueue = std::priority_queue<const Task*,
> > > >                                           std::vector<const Task*>,
> > > >                                           Comparator>;
> > > > std::unordered_map<const Task*, ChildrenQueue> children;
> > > > 
> > > >   - line 283 becomes:
> > > > 
> > > > children[task->GetParentTask()].push(task);
> > > > 
> > > >   - finally lines 326~330 become something like:
> > > > 
> > > > if (children_it == children.end())
> > > >   continue;
> > > > while (!children_it->second.empty()) {
> > > >   tasks_to_visit.push_back(children_it->second.top());
> > > >   children_it->second.pop();
> > > > }
> > > > 
> > > > 
> > > > I measured both timings and they're similar. Up to you.
> > > 
> > > Interesting idea. They're essentially the same under the hood; it's just
> that
> > we
> > > use heapsort vs an explicit std::sort. I'll take a look at this with fresh
> > eyes
> > > tomorrow. [Note to self: before touching this, make sure the tests
actually
> > > fails if we omit the existing std::sort.]
> > 
> > Yes, they're definitely the same algorithm. The suggested one aims to
improve
> > readability.
> 
> Thanks for this suggestion. My overall sense is that sorting in retrospect
> captures the essence of what I'm trying to do a little better, since there are
> no "pop min" operations during insertion.
> 
> I've made some other changes that hopefully make the directionality of the
> sorting a little clearer, including using base::Reversed() for the |parent|
> iteration.

Acknowledged.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:316: for (Task*
task : current_group_tasks) {
On 2016/06/20 17:40:00, ncarter wrote:
> On 2016/06/03 17:53:42, afakhry wrote:
> > const auto& ?
> 
> I prefer Task* here, since it makes it clearer to the reader what's being
> iterated over. Unless there's a functional reason for this -- does "const
auto&"
> produce better code?

It may or may not. const auto& is generally preferred for read-only access of
the elements in the container.

In our case above, using Task* will access an element by-value, i.e. copies a
Task pointer. This is likely not a big deal; it's just an address.

const auto&, will access the element by-const-ref. Which one performs better for
a good-old pointer? We can never tell until we look at the disassembly of this
code, and see what the compiler is doing. Hopefully the compiler can optimize
these things.

See:
http://stackoverflow.com/questions/15927033/what-is-the-correct-way-of-using-...

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:349: for (Task*
task : group->tasks()) {
On 2016/06/20 17:39:59, ncarter wrote:
> On 2016/06/03 17:53:42, afakhry wrote:
> > Also const auto& ?
> 
> I prefer 'Task*' to 'const auto&' here; it's is fewer characters and makes it
> clear what we're iterating over.

Acknowledged.

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/ui/task...
File chrome/browser/ui/task_manager/task_manager_table_model.cc (right):

https://codereview.chromium.org/2028753002/diff/210001/chrome/browser/ui/task...
chrome/browser/ui/task_manager/task_manager_table_model.cc:425: return
ValueCompare(row1, row2);
On 2016/06/20 17:40:00, ncarter wrote:
> On 2016/06/03 17:53:42, afakhry wrote:
> > Be prepared for a storm of regression bugs saying "sorting by tasks doesn't
> sort
> > alphabetically". Have you talked to UX about this?
> 
> I'll start a thread with UI folks. We definitely have a few options for ways
to
> expose the logical sorting.
> 
> In the mean time, I've removed this part of the diff (I think the tests will
> still pass). We can expose this to the UI as a separate CL.

Acknowledged.

[ncarter (slow), 2016-06-22 16:15:19]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-06-22 16:15:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028753002/270001

[commit-bot: I haz the power, 2016-06-22 17:26:18]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

[commit-bot: I haz the power, 2016-06-22 17:26:19]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2016-06-22 17:30:18]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests
==========

to

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests

Committed: https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802
Cr-Commit-Position: refs/heads/master@{#401328}
==========

[commit-bot: I haz the power, 2016-06-22 17:30:19]

Patchset 15 (id:??) landed as
https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802
Cr-Commit-Position: refs/heads/master@{#401328}

[Charlie Reis, 2016-06-23 00:27:45]

Description was changed from

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests

Committed: https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802
Cr-Commit-Position: refs/heads/master@{#401328}
==========

to

==========
Make Task Manager sort more meaningful:
 - Model order is now more meaningful:
    - Browser process is always first.
    - Gpu process is always second.
    - More generally, sorting of non-subtasks is done by process
      type, followed by tab id, and finally task ID.
       - Using tab ID puts "Tab:" rows rows in tab creation order, and
         provides preserves the row's position in the list even when
         a tab navigates cross-process (though not across prerendering)
       - Sorting by process type places the browser process first,
         the gpu process second, etc, in a way that's predictable.
         (sorting by task ID or pid resulted in an unpredictable
         ordering).
    - Subtasks (i.e. subframes) appear right after their parents.
    - Using task_id as a tiebreaker within a group of related tasks
      gives a predictable ordering, since this is effectively
      task-creation order.
    - Groups are kept on consecutive rows, as before. Within a group,
      rows are ordered by the same sort comparator that is used
      to sort between groups.
 - To effect the above sorting changes, the following tweaks to
   the TaskManager private interfaces are needed:
    - Expose a GetParentTask() method on Task.
    - TaskGroup switches to using an unsorted vector of Tasks.
    - Expose a Task ctor that supports overriding the pid, for
      use in unittests.

This does not fix the UI problem where there's no way for
the user to get back to the model order after sorting by any
of the columns. A follow-on CL will need to address that.

BUG=616897,54909,239611,606985,527455
TESTS=unit_tests, browser_tests

Committed: https://crrev.com/0088a36baa443168daf851f3ccf95a2ad7090802
Cr-Commit-Position: refs/heads/master@{#401328}
==========