Figure out what to do with MemProfiler

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Questions are:

(1) Is anyone actively using this?
(2) Can we make this code debug-only or Nightly-only?

Even if the overhead is small, it's hard to justify if the answer to (1) is No.
(Reporter)

Comment 1

2 years ago
kanru, do you know the status of this? Thanks!
Flags: needinfo?(kchen)
(In reply to Jan de Mooij [:jandem] from comment #0)
> Questions are:
> 
> (1) Is anyone actively using this?
> (2) Can we make this code debug-only or Nightly-only?
> 
> Even if the overhead is small, it's hard to justify if the answer to (1) is
> No.

The answer to (1) no currently and I'm not sure if it will change in the near future. That depends on how devtools prioritize it.

I think we can safely make it DEBUG only or behind a flag for now.

Greg could you comment whether we want to fix bug 1231794 and bug 1231787 or integrate the memory profiler directly into gecko profiler?
Flags: needinfo?(kchen) → needinfo?(gtatum)
(Reporter)

Comment 3

2 years ago
We should also consider just removing the code - it will always be available in VCS if we ever do need it, but until then it will likely bitrot anyway.
(In reply to Jan de Mooij [:jandem] from comment #3)
> We should also consider just removing the code - it will always be available
> in VCS if we ever do need it, but until then it will likely bitrot anyway.

Yeah, it can bitrot easily so I'm inclined to keep it behind DEBUG or simply remove it.
(In reply to Kan-Ru Chen 
[:kanru] (UTC+8) from comment #2)
> Greg could you comment whether we want to fix 
>     bug 1231794 Replace memory graph data source with post-processing memory
> profiler's alloc/free
>    and 
>     bug 1231787 Integrate memory profiler's gc malloc counter
> instrumentation with allocations view
>    or integrate the memory profiler directly into gecko profiler?

I'm not very opinionated on it. The previous engineers interested in doing that work are no longer part of the DevTools team. I know I'd like to get more memory information into the profiler to get parity with perf.html, but I haven't had any conversations with other people on the current perf team. The devtools are all appearing to use the DebuggerMemory as the data source in the memory and performance actors.

Markus, do you have any thoughts here?
Flags: needinfo?(gtatum) → needinfo?(mstange)
I don't know enough about it in order to have an opinion. I think I've used it maybe once in my life.

I'm not sure why that is. I think memory just isn't a problem for me usually, or maybe I'm just not aware of it being a problem.
Flags: needinfo?(mstange)
(Reporter)

Comment 7

2 years ago
Ehsan, this is the kind of bug that should be pretty easy for someone else to take on.

MemProfiler hooks into our object allocation/tenuring/etc code so it adds some overhead there. We should try to remove it.
Blocks: 1245279
Flags: needinfo?(ehsan)
(Assignee)

Comment 8

2 years ago
I take immense pleasure at deleting code.  :-)  I'll do it.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
(Assignee)

Comment 9

2 years ago
This feature isn't currently used or being planned to be used in the near
future and has some overhead that makes it hard to justify to keep around,
so it's better to remove it and revive it from VCS history if we need it
later.
Attachment #8895418 - Flags: review?(jdemooij)
(Reporter)

Comment 10

2 years ago
Comment on attachment 8895418 [details] [diff] [review]
Remove MemProfiler

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

More code than I expected. Thanks for doing this!
Attachment #8895418 - Flags: review?(jdemooij) → review+

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7bd19d28e16
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1401101
You need to log in before you can comment on or make changes to this bug.