1385953 - Figure out what to do with MemProfiler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

kanru, do you know the status of this? Thanks!

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

(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?

Comment 3

[Jan de Mooij [:jandem]]

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.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

(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.

Comment 5

[Greg Tatum [:gregtatum]]

(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?

Comment 6

[Markus Stange [: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.

Comment 7

[Jan de Mooij [:jandem]]

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.

Comment 8

[(no longer active)]

I take immense pleasure at deleting code.  :-)  I'll do it.

Comment 9

[(no longer active)]

Attachment: Remove MemProfiler

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.

Comment 10

[Jan de Mooij [:jandem]]

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!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7bd19d28e16
Remove MemProfiler; r=jandem

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7bd19d28e16