Closed Bug 1385953 Opened 3 years ago Closed 3 years ago

Figure out what to do with MemProfiler

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jandem, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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)
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)
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)
I take immense pleasure at deleting code.  :-)  I'll do it.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
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)
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+
https://hg.mozilla.org/mozilla-central/rev/b7bd19d28e16
Status: NEW → RESOLVED
Closed: 3 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.