Closed Bug 798019 Opened 7 years ago Closed 7 years ago

Sort processes in about:memory by resident size

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Details

Attachments

(1 file)

The main process still always comes first, although when we get input from a file, we don't have a concept of the main process, so there we sort strictly by resident.

This helps a lot when viewing merged about:memory dumps.
Assignee: nobody → justin.lebar+bug
Attached patch Patch, v1Splinter Review
Attachment #668148 - Flags: review?(n.nethercote)
Comment on attachment 668148 [details] [diff] [review]
Patch, v1

Review of attachment 668148 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +508,5 @@
> +    if ((a == gUnnamedProcessStr) != (b == gUnnamedProcessStr)) {
> +      if (a == gUnnamedProcessStr) {
> +        return -1;
> +      }
> +      return 1;

This is better:

  return (a == gUnnamedProcessStr) ? -1 : 1;

::: toolkit/components/aboutmemory/tests/test_aboutmemory.xul
@@ +182,5 @@
>    }
>   
> +  // We sort processes by their |resident| value (starting with the largest).
> +  // A process with no |resident| value is considered to have a |resident| of
> +  // negative infinity.

Zero should suffice! :)
Attachment #668148 - Flags: review?(n.nethercote) → review+
Comment on attachment 668148 [details] [diff] [review]
Patch, v1

Review of attachment 668148 [details] [diff] [review]:
-----------------------------------------------------------------

BTW, if you haven't already, make sure you run all the tests in toolkit/components/aboutmemory/tests.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +508,5 @@
> +    if ((a == gUnnamedProcessStr) != (b == gUnnamedProcessStr)) {
> +      if (a == gUnnamedProcessStr) {
> +        return -1;
> +      }
> +      return 1;

Actually, shouldn't it be impossible for both |a| and |b| to equal |gUnnamedProcessStr|?  Perhaps assert that and remove the outer condition?  Or if you don't like that, how about this:

  if (a == gUnnamedProcessStr || b == gUnnamedProcessStr) {
    return (a == gUnnamedProcessStr) ? -1 : 1;
  }

::: toolkit/components/aboutmemory/tests/test_aboutmemory.xul
@@ +182,5 @@
>    }
>   
> +  // We sort processes by their |resident| value (starting with the largest).
> +  // A process with no |resident| value is considered to have a |resident| of
> +  // negative infinity.

Zero should suffice! :)
> Actually, shouldn't it be impossible for both |a| and |b| to equal |gUnnamedProcessStr|?

Ah, I guess these object properties so they're all unique.  I hadn't thought about that!
https://hg.mozilla.org/mozilla-central/rev/823227b1ffe6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
jlebar:  looks like you forgot to include the mozilla-inbound links on this bug and the related about:memory bugs you recently worked on.
(In reply to Nicholas Nethercote [:njn] from comment #6)
> jlebar:  looks like you forgot to include the mozilla-inbound links on this
> bug and the related about:memory bugs you recently worked on.

With merges once a day, I don't feel like it's necessary.  If my patch gets backed out, the backout usually refers to the original cset.

Is it somehow messing you up that there aren't links to m-i?
It's not specifically messing me up, but it's very much standard operating procedure.  I like having the full paper trail.
(In reply to Nicholas Nethercote [:njn] from comment #8)
> It's not specifically messing me up, but it's very much standard operating
> procedure.  I like having the full paper trail.

You know I don't like doing work that doesn't have a tangible benefit, even if everyone else does it.  :)

But your opinion is duly noted, and I'll try to remember to put m-i comments in bugs you're on.
You need to log in before you can comment on or make changes to this bug.