Add a benchmarking_extension API to call TCMalloc's HeapProfilerDump().

Add a benchmarking_extension API to call TCMalloc's HeapProfilerDump().


BUG=chromium-os:38652
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181185

[Dai Mikurube (NOT FULLTIME), 2013-02-05 07:42:08]

Hi Nat and Dennis,

This adds a benchmarking_extension API to support HeapProfilerDump() from
Telemetry tests.  (I think we still need more work in tools/perf/perf_tools.)

It works with 'chrome --enable-benchmarking --enable-stats-table --no-sandbox'
and chrome.benchmarking.heapProfilerDump('reason') in DevTools.


Dennis, do you have any bug to migrate Chrome Endure from PyAuto to Telemetry?

[dennis_jeffrey, 2013-02-06 01:19:43]

This looks good to me at a high level, but I'll defer to the other reviewer(s)
for final approval since I'm not very familiar with this area.  Please CC me too
on the corresponding changes you'll need to make (if any) on the Telemetry side
to get this working with Telemetry.

My tracking issue to convert the Chrome Endure endurance tests from pyauto to
Telemetry is here: https://code.google.com/p/chromium-os/issues/detail?id=38652

I'm currently working on that.

[Dai Mikurube (NOT FULLTIME), 2013-02-06 01:23:35]

On 2013/02/06 01:19:43, dennis_jeffrey wrote:
> This looks good to me at a high level, but I'll defer to the other reviewer(s)
> for final approval since I'm not very familiar with this area.  Please CC me
too
> on the corresponding changes you'll need to make (if any) on the Telemetry
side
> to get this working with Telemetry.
> 
> My tracking issue to convert the Chrome Endure endurance tests from pyauto to
> Telemetry is here:
https://code.google.com/p/chromium-os/issues/detail?id=38652
> 
> I'm currently working on that.

Thanks.  I added the BUG to this issue.  I think I finally ask OWNER's review to
jochen.

I'll Cc you in succeeding changes around it.

[nduca, 2013-02-06 07:26:42]

https://codereview.chromium.org/12211008/diff/1/chrome/renderer/benchmarking_...
File chrome/renderer/benchmarking_extension.cc (right):

https://codereview.chromium.org/12211008/diff/1/chrome/renderer/benchmarking_...
chrome/renderer/benchmarking_extension.cc:176: #if !defined(NO_TCMALLOC) &&
(defined(OS_LINUX) || defined(OS_CHROMEOS))
Awesome. Now, we've been trying to split apart this monolithic extension [so
that is easy to reason about the side effects of different patches]. While these
changes aren't themselves controversial, I think it'd be in keeping with that
general desire to pull this out to its own extension, maybe we can call it the
memory-benchmarking extension? We can then put other memory related extensions
as we find need for them.

https://codereview.chromium.org/11801024/ has an example of danhn doing this.

[Dai Mikurube (NOT FULLTIME), 2013-02-06 10:51:15]

Thanks, Nat.

https://codereview.chromium.org/12211008/diff/1/chrome/renderer/benchmarking_...
File chrome/renderer/benchmarking_extension.cc (right):

https://codereview.chromium.org/12211008/diff/1/chrome/renderer/benchmarking_...
chrome/renderer/benchmarking_extension.cc:176: #if !defined(NO_TCMALLOC) &&
(defined(OS_LINUX) || defined(OS_CHROMEOS))
On 2013/02/06 07:26:42, nduca wrote:
> Awesome. Now, we've been trying to split apart this monolithic extension [so
> that is easy to reason about the side effects of different patches]. While
these
> changes aren't themselves controversial, I think it'd be in keeping with that
> general desire to pull this out to its own extension, maybe we can call it the
> memory-benchmarking extension? We can then put other memory related extensions
> as we find need for them.
> 
> https://codereview.chromium.org/11801024/ has an example of danhn doing this.

Hmm, do you mean we should have new memory_benchmarking_extension.{cc|h} instead
of adding this function to benchmarking_extension.cc?

It's ok to do that.  Are all files to be changed are the following?

1) chrome/chrome_renderer.gypi ... add memory_benchmarking_extension
2) chrome/renderer/chrome_content_renderer_client.cc ... register
3) chrome/renderer/memory_benchmarking_extension.h
4) chrome/renderer/memory_benchmarking_extension.cc

[nduca, 2013-02-06 10:54:37]

That sounds about right!

[Dai Mikurube (NOT FULLTIME), 2013-02-06 11:18:15]

Updated the patch.  Checked it's working with --enable-benchmarking. :)

[Dai Mikurube (NOT FULLTIME), 2013-02-06 11:25:43]

Added jochen for OWNER review.

Jochen, could you take a look when Nat says ok?

[nduca, 2013-02-06 11:32:15]

Use your own flag.... --enable-memory-benchmarking

[nduca, 2013-02-06 11:32:39]

Oh, sorry, also, this is not really chrome specific, right? Seems more like a
content/renderer extension.

[Dai Mikurube (NOT FULLTIME), 2013-02-06 11:36:45]

On 2013/02/06 11:32:39, nduca wrote:
> Oh, sorry, also, this is not really chrome specific, right? Seems more like a
> content/renderer extension.

Should we have content/renderer/memory_benchmarking_extension ?

In that case, is the JS variable name still
chrome.(memory)benchmarking.heapProfilerDump() ?

[anagy4698, 2013-02-06 11:37:41]

 Enable-memória-benchmarking


2013/2/6 <dmikurube@chromium.org>

> On 2013/02/06 11:32:39, nduca wrote:
>
>> Oh, sorry, also, this is not really chrome specific, right? Seems more
>> like a
>> content/renderer extension.
>>
>
> Should we have content/renderer/memory_**benchmarking_extension ?
>
> In that case, is the JS variable name still
> chrome.(memory)benchmarking.**heapProfilerDump() ?
>
>
https://codereview.chromium.**org/12211008/<https://codereview.chromium.org/1...
>



-- 
anagy4698@gmail.com.

[jochen (gone - plz use gerrit), 2013-02-06 11:47:56]

On 2013/02/06 11:36:45, Dai Mikurube wrote:
> On 2013/02/06 11:32:39, nduca wrote:
> > Oh, sorry, also, this is not really chrome specific, right? Seems more like
a
> > content/renderer extension.
> 
> Should we have content/renderer/memory_benchmarking_extension ?
> 
> In that case, is the JS variable name still
> chrome.(memory)benchmarking.heapProfilerDump() ?

yes

[jochen (gone - plz use gerrit), 2013-02-06 11:57:57]

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
File chrome/renderer/memory_benchmarking_extension.cc (right):

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:10: #if !defined(NO_TCMALLOC)
&& (defined(OS_LINUX) || defined(OS_CHROMEOS))
#if clauses should come after all includes with an empty line in between

also, why not windows as well?

I think if you don't have tcmalloc, you just shouldn't compile the extension in.

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:15: const char
kMemoryBenchmarkingExtensionName[] = "v8/MemoryBenchmarking";
should also go into an anonymous namespace

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:19: class
MemoryBenchmarkingWrapper : public v8::Extension {
this class and the static functions below should go into an anonymous namespace

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:22:
v8::Extension(kMemoryBenchmarkingExtensionName,
usually, we put the code in a .js file in the resources/ subdirectory and load
it via resource bundle

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:51: IsHeapProfilerRunning(const
v8::Arguments& args) {
i think you should wrap after IsHeapProfilerRunning( instead of before

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:61: char reason[256];
why not 

std::string reason("benchmarking_extension");
if (args.Length() > 0 && args[0].isString())
  reason = *v8::String::AsciiValue(args[0]);
::HeapProfilerDump(reason.c_str());

[Dai Mikurube (NOT FULLTIME), 2013-02-06 12:24:45]

Moved to content/.  We may want another OWNER. ;)

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
File chrome/renderer/memory_benchmarking_extension.cc (right):

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:10: #if !defined(NO_TCMALLOC)
&& (defined(OS_LINUX) || defined(OS_CHROMEOS))
On 2013/02/06 11:57:57, jochen wrote:
> #if clauses should come after all includes with an empty line in between
> 
Done.

> also, why not windows as well?
> 
Since Windows dump is not tested.  (Now, I don't have an environment to test
it.)

> I think if you don't have tcmalloc, you just shouldn't compile the extension
in.

I'd like to allow users to check it by
chrome.memoryBenchmarking.isHeapProfilerRunning.

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:15: const char
kMemoryBenchmarkingExtensionName[] = "v8/MemoryBenchmarking";
On 2013/02/06 11:57:57, jochen wrote:
> should also go into an anonymous namespace

Done.

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:19: class
MemoryBenchmarkingWrapper : public v8::Extension {
On 2013/02/06 11:57:57, jochen wrote:
> this class and the static functions below should go into an anonymous
namespace

In an anonymous namespace, how can I call MemoryBenchmarkingExtension::Get() to
register the extension?  (It's now registered in render_thread_impl.)

It's the same way with content/render/gpu/gpu_benchmarking_extension.

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:22:
v8::Extension(kMemoryBenchmarkingExtensionName,
On 2013/02/06 11:57:57, jochen wrote:
> usually, we put the code in a .js file in the resources/ subdirectory and load
> it via resource bundle

Almost all the benchmarking_extension does this.  What do you think, Nat?

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:51: IsHeapProfilerRunning(const
v8::Arguments& args) {
On 2013/02/06 11:57:57, jochen wrote:
> i think you should wrap after IsHeapProfilerRunning( instead of before

Done.

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:61: char reason[256];
On 2013/02/06 11:57:57, jochen wrote:
> why not 
> 
> std::string reason("benchmarking_extension");
> if (args.Length() > 0 && args[0].isString())
>   reason = *v8::String::AsciiValue(args[0]);
> ::HeapProfilerDump(reason.c_str());
Done.

I didn't know AsciiValue().  I did the same way with
chrome/renderer/benchmarking_extension.cc.

[nduca, 2013-02-06 12:31:12]

I think its okay to leave the js in the cpp since its not large, not heavily
modified. Its just tranpoline code basically.

[jochen (gone - plz use gerrit), 2013-02-06 12:31:41]

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
File chrome/renderer/memory_benchmarking_extension.cc (right):

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:19: class
MemoryBenchmarkingWrapper : public v8::Extension {
On 2013/02/06 12:24:45, Dai Mikurube wrote:
> On 2013/02/06 11:57:57, jochen wrote:
> > this class and the static functions below should go into an anonymous
> namespace
> 
> In an anonymous namespace, how can I call MemoryBenchmarkingExtension::Get()
to
> register the extension?  (It's now registered in render_thread_impl.)
> 
> It's the same way with content/render/gpu/gpu_benchmarking_extension.

I meant that the Wrapper should go into the anon namespace. You only need the
static MemoryBenchmarkingExtension::Get() outside of the namespace.

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:22:
v8::Extension(kMemoryBenchmarkingExtensionName,
On 2013/02/06 12:24:45, Dai Mikurube wrote:
> On 2013/02/06 11:57:57, jochen wrote:
> > usually, we put the code in a .js file in the resources/ subdirectory and
load
> > it via resource bundle
> 
> Almost all the benchmarking_extension does this.  What do you think, Nat?

I'd argue they do it wrong :)

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:24: "  chrome = {};"
this should be just 

chrome = chrome || {};
chrome.benchmarking = chrome.benchmarking || {};

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:41: if
(name->Equals(v8::String::New("IsHeapProfilerRunning"))) {
no { } for single-line bodies

[Dai Mikurube (NOT FULLTIME), 2013-02-06 12:38:01]

Thanks. :)

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
File chrome/renderer/memory_benchmarking_extension.cc (right):

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:19: class
MemoryBenchmarkingWrapper : public v8::Extension {
On 2013/02/06 12:31:41, jochen wrote:
> On 2013/02/06 12:24:45, Dai Mikurube wrote:
> > On 2013/02/06 11:57:57, jochen wrote:
> > > this class and the static functions below should go into an anonymous
> > namespace
> > 
> > In an anonymous namespace, how can I call MemoryBenchmarkingExtension::Get()
> to
> > register the extension?  (It's now registered in render_thread_impl.)
> > 
> > It's the same way with content/render/gpu/gpu_benchmarking_extension.
> 
> I meant that the Wrapper should go into the anon namespace. You only need the
> static MemoryBenchmarkingExtension::Get() outside of the namespace. 

Makes sense.  Done.

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:24: "  chrome = {};"
On 2013/02/06 12:31:41, jochen wrote:
> this should be just 
> 
> chrome = chrome || {};
> chrome.benchmarking = chrome.benchmarking || {};

IIRC, they have different meaning in JS.  I'm not sure how these extensions
(including other extensions which do the same thing) should behave.

Wdyt, Nat?

https://codereview.chromium.org/12211008/diff/6001/chrome/renderer/memory_ben...
chrome/renderer/memory_benchmarking_extension.cc:41: if
(name->Equals(v8::String::New("IsHeapProfilerRunning"))) {
On 2013/02/06 12:31:41, jochen wrote:
> no { } for single-line bodies

Done.

[nduca, 2013-02-06 12:41:27]

Lets stick to chrome.benchmarking convention.

Wrt js debate, I understand the assertion. Your call whether to shave that yak.
Its a yak. They suck.

[nduca, 2013-02-06 12:54:39]

lgtm

[Dai Mikurube (NOT FULLTIME), 2013-02-06 12:59:32]

Added piman@ for content/ OWNER review.  piman, could you take a look at this?

I'm +1 for sticking to the existing chrome.benchmarking convention at this time.
 Please tell us if you have strong objection for that.

[piman, 2013-02-06 18:11:51]

LGTM+nit

https://codereview.chromium.org/12211008/diff/15008/content/renderer/memory_b...
File content/renderer/memory_benchmarking_extension.cc (right):

https://codereview.chromium.org/12211008/diff/15008/content/renderer/memory_b...
content/renderer/memory_benchmarking_extension.cc:10: #if !defined(NO_TCMALLOC)
&& (defined(OS_LINUX) || defined(OS_CHROMEOS))
nit: on chromeos=1 OS_LINUX is still defined so you don't need to explicitly
test for OS_CHROMEOS (here and below)

[Dai Mikurube (NOT FULLTIME), 2013-02-07 00:41:42]

Thanks, piman.  I'll be committing it.

https://codereview.chromium.org/12211008/diff/15008/content/renderer/memory_b...
File content/renderer/memory_benchmarking_extension.cc (right):

https://codereview.chromium.org/12211008/diff/15008/content/renderer/memory_b...
content/renderer/memory_benchmarking_extension.cc:10: #if !defined(NO_TCMALLOC)
&& (defined(OS_LINUX) || defined(OS_CHROMEOS))
On 2013/02/06 18:11:51, piman wrote:
> nit: on chromeos=1 OS_LINUX is still defined so you don't need to explicitly
> test for OS_CHROMEOS (here and below)

Removed.

[commit-bot: I haz the power, 2013-02-07 00:55:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/12211008/8002

[commit-bot: I haz the power, 2013-02-07 03:59:58]

Change committed as 181185