Refactoring: Remove an argument 'enabled' from onHeapProfilingEnabled

Refactoring: Remove an argument 'enabled' from onHeapProfilingEnabled

Assuming that once heap profiling is enabled we won't stop this during
the process, this CL removes the 'enabled' boolean argument from
onHeapProfilingEnabled of WebMemoryDumpProviders.

BUG=585063
TEST=n/a

[hajimehoshi, 2016-02-18 10:29:43]

Description was changed from

==========
Refactoring: Remove an argument 'enabled' from onHeapProfilingEnabled

Assuming that once heap profiling is enabled we won't stop this during
the process, this CL removes the 'enabled' boolean argument from
onHeapProfilingEnabled of WebMemoryDumpProviders.

BUG=585063
TEST=n/a
==========

to

==========
Refactoring: Remove an argument 'enabled' from onHeapProfilingEnabled

Assuming that once heap profiling is enabled we won't stop this during
the process, this CL removes the 'enabled' boolean argument from
onHeapProfilingEnabled of WebMemoryDumpProviders.

BUG=585063
TEST=n/a
==========

[hajimehoshi, 2016-02-18 10:29:43]

hajimehoshi@chromium.org changed reviewers:
+ haraken@chromium.org, kinuko@chromium.org, primiano@chromium.org,
tkent@chromium.org

[hajimehoshi, 2016-02-18 10:31:51]

PTAL

+primiano for base/trace_event
+kinuko for content/child
+haraken for platform
+tkent for public/platform

[haraken, 2016-02-18 11:19:31]

LGTM

[Primiano Tucci (use gerrit), 2016-02-18 11:42:47]

On 2016/02/18 11:19:31, haraken wrote:
> LGTM

Hmm as per discussion in crrev.com/1676453002 shouldn't we shoot at supporting
the tear-down path to cleanup stuff after tracing session is ended?
I think the use case we should shoot for is: you enable tracing with heap
profiler, create a bunch of AllocationRegister to hold metadata and everything.
Once the trace is ended we should cleanup that stuff, so we are back to normal.
Of course we will never support the case of enable -> disable -> re-enable, as I
explained in crrev.com/1676453002, because that will lose inmportant data.
But I think we should shoot at least at support the enable -> disable (only
once) case, to cleanup after ourselves.
What do you think?

[haraken, 2016-02-18 11:50:33]

On 2016/02/18 11:42:47, Primiano Tucci wrote:
> On 2016/02/18 11:19:31, haraken wrote:
> > LGTM
> 
> Hmm as per discussion in crrev.com/1676453002 shouldn't we shoot at supporting
> the tear-down path to cleanup stuff after tracing session is ended?
> I think the use case we should shoot for is: you enable tracing with heap
> profiler, create a bunch of AllocationRegister to hold metadata and
everything.
> Once the trace is ended we should cleanup that stuff, so we are back to
normal.
> Of course we will never support the case of enable -> disable -> re-enable, as
I
> explained in crrev.com/1676453002, because that will lose inmportant data.
> But I think we should shoot at least at support the enable -> disable (only
> once) case, to cleanup after ourselves.
> What do you think?

That sounds reasonable.

Then would it make more sense to prepare separate APIs?

- startHeapProfiling()
- stopHeapProfiling()

We can also add assertions to allow only the start => stop sequence.

[haraken, 2016-02-18 11:52:44]

On 2016/02/18 11:50:33, haraken wrote:
> On 2016/02/18 11:42:47, Primiano Tucci wrote:
> > On 2016/02/18 11:19:31, haraken wrote:
> > > LGTM
> > 
> > Hmm as per discussion in crrev.com/1676453002 shouldn't we shoot at
supporting
> > the tear-down path to cleanup stuff after tracing session is ended?
> > I think the use case we should shoot for is: you enable tracing with heap
> > profiler, create a bunch of AllocationRegister to hold metadata and
> everything.
> > Once the trace is ended we should cleanup that stuff, so we are back to
> normal.
> > Of course we will never support the case of enable -> disable -> re-enable,
as
> I
> > explained in crrev.com/1676453002, because that will lose inmportant data.
> > But I think we should shoot at least at support the enable -> disable (only
> > once) case, to cleanup after ourselves.
> > What do you think?
> 
> That sounds reasonable.
> 
> Then would it make more sense to prepare separate APIs?
> 
> - startHeapProfiling()
> - stopHeapProfiling()
> 
> We can also add assertions to allow only the start => stop sequence.

Hmm, btw what's the benefit of stopping the heap profiler? It will release some
memory but the user using the --enable-heap-profiling option wouldn't care about
the memory.

[Primiano Tucci (use gerrit), 2016-02-18 12:05:01]

> Then would it make more sense to prepare separate APIs?
> 
> - startHeapProfiling()
> - stopHeapProfiling()
Really up to you. In general my feeling is that the code that we put in
"OnHeapProfilingEnabled" is pretty small, a bunch of lines, so maybe we can keep
a single one just to keep code compact.
Don't have a strong opinion here really, up to you what you think is the most
readable choice.


> > We can also add assertions to allow only the start => stop sequence.
Yes we definitely should. 
I'm definitely going to add that in memory_dump_manager, but if you want to add
in the dump providers to make the code more readable and easy to reason feel
free to add it also there.

> 
> Hmm, btw what's the benefit of stopping the heap profiler? It will release
some
> memory but the user using the --enable-heap-profiling option wouldn't care
about
> the memory.

So the only real benefit I can think about is if in future in telemetry we want
to have some benchmark which first measures some memory and then performance (so
we have uninstalled all the hooks) without having to restart chrome to remove
the --enable-heap-profiling flags

[haraken, 2016-02-18 12:10:13]

On 2016/02/18 12:05:01, Primiano Tucci wrote:
> > Then would it make more sense to prepare separate APIs?
> > 
> > - startHeapProfiling()
> > - stopHeapProfiling()
> Really up to you. In general my feeling is that the code that we put in
> "OnHeapProfilingEnabled" is pretty small, a bunch of lines, so maybe we can
keep
> a single one just to keep code compact.
> Don't have a strong opinion here really, up to you what you think is the most
> readable choice.
> 
> 
> > > We can also add assertions to allow only the start => stop sequence.
> Yes we definitely should. 
> I'm definitely going to add that in memory_dump_manager, but if you want to
add
> in the dump providers to make the code more readable and easy to reason feel
> free to add it also there.
> 
> > 
> > Hmm, btw what's the benefit of stopping the heap profiler? It will release
> some
> > memory but the user using the --enable-heap-profiling option wouldn't care
> about
> > the memory.
> 
> So the only real benefit I can think about is if in future in telemetry we
want
> to have some benchmark which first measures some memory and then performance
(so
> we have uninstalled all the hooks) without having to restart chrome to remove
> the --enable-heap-profiling flags

Then I'd prefer landing this CL. We can revise the API when we really need that
ability and a telemetry API to stop the heap profiling.

[Primiano Tucci (use gerrit), 2016-02-18 12:34:36]

On 2016/02/18 12:10:13, haraken wrote:
> Then I'd prefer landing this CL. We can revise the API when we really need
that
> ability and a telemetry API to stop the heap profiling.

Hmm but we already have that, It's just StopTracing().
Essentially the only thing missing here to make chrome behave nicely is
memory_dump_manager calling OnHeapProfilerEnabled(false) (or whatever) when the
trace is stopped.
It's a small change, but I don't see a good reason to not try to clean-up our
stuff after we are done.
Unless this would cause some technical complication / maintenance problem I'm
not thinking about?

[haraken, 2016-02-18 12:46:51]

On 2016/02/18 12:34:36, Primiano Tucci wrote:
> On 2016/02/18 12:10:13, haraken wrote:
> > Then I'd prefer landing this CL. We can revise the API when we really need
> that
> > ability and a telemetry API to stop the heap profiling.
> 
> Hmm but we already have that, It's just StopTracing().
> Essentially the only thing missing here to make chrome behave nicely is
> memory_dump_manager calling OnHeapProfilerEnabled(false) (or whatever) when
the
> trace is stopped.
> It's a small change, but I don't see a good reason to not try to clean-up our
> stuff after we are done.
> Unless this would cause some technical complication / maintenance problem I'm
> not thinking about?

Makes sense. Let's close this CL.