Get a filename of dumped heap profile in a V8 extension HeapProfilerDump().

Get a filename of dumped heap profile in a V8 extension HeapProfilerDump().


BUG=chromium-os:38652
TBR=darin@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182659

[Dai Mikurube (NOT FULLTIME), 2013-02-13 11:30:51]

Hi Jim and Nat,

It's
* adding a new TCMalloc heap-profiler function HeapProfilerDumpWithFilename()
* changing the v8 extension HeapProfilerDump() to return the filename where the
heap profile is dumped to

Note that it affects only heap profiling (only when an environment variable
HEAPPROFILE is specified).


This change is required because Telemetry (benchmarking framework) needs another
way to identify the dumped file.  We used pid to identify the dumped file, but
Telemetry doesn't provide pid.

We had another design choice to specify a file (name or descriptor) to be
dumped.  I didn't choose it since existing profiler scripts (both tcmalloc's
heap profiler and deep memory profiler) assume a fixed filename format.  The
file name should be decided by the heap profiler itself.


We may want to reconsider the function name HeapProfilerDumpWithFileName().  I
think about upstreaming this patch to the original tcmalloc.

[Dai Mikurube (NOT FULLTIME), 2013-02-13 11:34:08]

(fyi, another reason why I didn't choose giving a filename is that the heap
profiler manages a dump count by itself.  The count should be consistent.)

[Dai Mikurube (NOT FULLTIME), 2013-02-13 16:04:45]

Added a note.

https://codereview.chromium.org/12212157/diff/1/third_party/tcmalloc/chromium...
File third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h (right):

https://codereview.chromium.org/12212157/diff/1/third_party/tcmalloc/chromium...
third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h:99: int
filename_buffer_length);
FYI, |dumped_filename_buffer| is output of this function.  Reviewers may think
that it should be the last argument, but it follows the existing convention of
tcmalloc.  See the line 195 in heap-profile-table for example:
http://src.chromium.org/viewvc/chrome/trunk/src/third_party/tcmalloc/chromium...

We may want more comments on the arguments.  I'll add it later.

[Dai Mikurube (NOT FULLTIME), 2013-02-14 07:27:35]

Gentle ping?

Updated the comment as written in
https://codereview.chromium.org/12212157/diff/1/third_party/tcmalloc/chromium....

[nduca, 2013-02-14 07:31:36]

extension portion lgtm.

I know jar got nailed with a dozen different p0 reviews today, so he may be
still recovering. :)

[Dai Mikurube (NOT FULLTIME), 2013-02-14 08:43:26]

On 2013/02/14 07:31:36, nduca wrote:
> extension portion lgtm.
> 
> I know jar got nailed with a dozen different p0 reviews today, so he may be
> still recovering. :)

Thanks for looking, Nat.

Yeah, he's so busy.  I sometimes send a ping for reminding. :)

[jar (doing other things), 2013-02-14 23:30:09]

Small nit below.

LGTM

https://codereview.chromium.org/12212157/diff/4001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):

https://codereview.chromium.org/12212157/diff/4001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profiler.cc:277: int
filename_buffer_length) {
nit: int-->size_t

[Dai Mikurube (NOT FULLTIME), 2013-02-15 03:30:42]

Thanks, Jim!

I'll be checking the "Commit" box.

https://codereview.chromium.org/12212157/diff/4001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):

https://codereview.chromium.org/12212157/diff/4001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profiler.cc:277: int
filename_buffer_length) {
On 2013/02/14 23:30:09, jar wrote:
> nit: int-->size_t
Done.

Hmm, int and size_t are mixed for size-like values in this file. ;)

[commit-bot: I haz the power, 2013-02-15 03:34:20]

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

[commit-bot: I haz the power, 2013-02-15 03:34:27]

Presubmit check for 12212157-8001 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/third_party/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/third_party/tcmalloc/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    third_party
    content/renderer

Presubmit checks took 4.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Dai Mikurube (NOT FULLTIME), 2013-02-15 05:08:59]

+piman

I forgot the OWNERS again. ;)

piman@, could you take a look?  nduca@ has already LGTM'ed.

[piman, 2013-02-15 05:26:59]

lgtm

[Dai Mikurube (NOT FULLTIME), 2013-02-15 05:40:47]

On 2013/02/15 05:26:59, piman wrote:
> lgtm

Thanks!

[commit-bot: I haz the power, 2013-02-15 05:41:42]

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

[commit-bot: I haz the power, 2013-02-15 05:41:45]

Presubmit check for 12212157-8001 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/third_party/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/third_party/tcmalloc/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    third_party

Presubmit checks took 1.4s to calculate.

[Dai Mikurube (NOT FULLTIME), 2013-02-15 05:52:53]

+Darin,

Ugh, third_party/ recently has OWNERS.  Darin, could you take a look?  It's for
the memory profiling in Telemetry tests.


Also, I guess we may want to have additional OWNERS for third_party/tcmalloc. 
jar@ (and glider@?) may be the right persons to be included.  (I know well about
heap-profiler things, but not so much about others.)

[Dai Mikurube (NOT FULLTIME), 2013-02-15 05:59:22]

Sorry, I missed the announcement of thitd_party's OWNERS.  I added darin to TBR.
 I'll be committing it.

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/Ynu3dDWXRcc/dis...

[commit-bot: I haz the power, 2013-02-15 06:00:05]

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

[darin (slow to review), 2013-02-15 06:01:34]

LGTM

Yes, adding an OWNERS file for tcmalloc seems like a good idea.

[commit-bot: I haz the power, 2013-02-15 10:12:31]

Change committed as 182659