Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1555553002:
[profiler] Implement POC Sampling Heap Profiler (Closed)
Description[profiler] Implement POC Sampling Heap Profiler
This implements a proof-of-concept sampling based heap profiler inspired by
tcmalloc's heap profiler [1] and Go's mprof/memprofile [2].
The basic idea is the sample allocations using a randomized Poisson process. At
any point in time we can cheaply request the set of live sample objects that
should be a representative sample of heap. Samples include stack-traces from the
allocation sites, making this an effective tool for memory leak debugging.
Unlike AllocationTracking, this is intended to be cheap and usable online in
production.
The proof-of-concept is only sampling new-space allocations at this point.
Support for sampling paged space and native allocations is anticipated in the
future.
[1] http://goog-perftools.sourceforge.net/doc/heap_profiler.html
[2] http://blog.golang.org/profiling-go-programs
Committed: https://crrev.com/e5a9947811db9c9e23557dbad27f8b8a349b3262
Cr-Commit-Position: refs/heads/master@{#33448}
Patch Set 1 #Patch Set 2 : cleanup samples when sampling heap profiler is stopped #
Total comments: 5
Patch Set 3 : switch from JSON to a C++ api #Patch Set 4 : Flesh out tests. Switch to a call-graph based API similar to cpu profiler. #Patch Set 5 : name strings cannot be null strings anymore #Patch Set 6 : fix warning + minor cleanup in AllocateNode #
Total comments: 2
Patch Set 7 : remove commented code #
Total comments: 18
Patch Set 8 : return profile as a transferred pointer - cannot move due to various c++11 issues #Patch Set 9 : address code review comments from alph@ #Patch Set 10 : increase sampling frequency in tests to reduce random chance failure likelihood #
Total comments: 39
Patch Set 11 : address round 2 comments from alph@ #
Total comments: 1
Patch Set 12 : remove unused variable #
Messages
Total messages: 47 (15 generated)
Description was changed from ========== [profiler] Implement POC Sampling Heap Profiler This implements a proof-of-concept sampling based heap profiler inspired by tcmalloc's heap profiler [1] and Go's mprof/memprofile [2]. The basic idea is the sample allocations using a randomized Poisson process. At any point in time we can cheaply request the set of live sample objects that should be a representative sample of heap. Samples include stack-traces from the allocation sites, making this an effective tool for memory leak debugging. Unlike AllocationTracking, this is intended to be cheap and usable online in production. The proof-of-concept is only sampling new-space allocations at this point. Support for sampling paged space and native allocations is anticipated in the future. [1] http://goog-perftools.sourceforge.net/doc/heap_profiler.html [2] http://blog.golang.org/profiling-go-programs ========== to ========== [profiler] Implement POC Sampling Heap Profiler This implements a proof-of-concept sampling based heap profiler inspired by tcmalloc's heap profiler [1] and Go's mprof/memprofile [2]. The basic idea is the sample allocations using a randomized Poisson process. At any point in time we can cheaply request the set of live sample objects that should be a representative sample of heap. Samples include stack-traces from the allocation sites, making this an effective tool for memory leak debugging. Unlike AllocationTracking, this is intended to be cheap and usable online in production. The proof-of-concept is only sampling new-space allocations at this point. Support for sampling paged space and native allocations is anticipated in the future. [1] http://goog-perftools.sourceforge.net/doc/heap_profiler.html [2] http://blog.golang.org/profiling-go-programs ==========
ofrobots@google.com changed reviewers: + hpayer@chromium.org, jochen@chromium.org, yangguo@chromium.org
On 2015/12/29 16:49:53, ofrobots wrote: > mailto:ofrobots@google.com changed reviewers: > + mailto:hpayer@chromium.org, mailto:jochen@chromium.org, mailto:yangguo@chromium.org This does need more tests, but I wanted to get an initial review on the API before I started adding more tests. PTAL. Thanks in advance.
ofrobots@google.com changed reviewers: + alph@chromium.org
https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:529: * currently live in the system. Each sampled allocation includes the stack Not questioning the effectiveness, just curious: can we really consider it a representative sample of live objects if we only observe allocation, but not death of objects? https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:560: void GetHeapSample(OutputStream* stream); I wonder whether it makes better sense to use the same format as the heap snapshot uses. You can still turn it into JSON in post-processing. That way, we could use it in devtools as well, as sort of light-weight heap snapshot that can be always-on whenever devtools is active.
https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:529: * currently live in the system. Each sampled allocation includes the stack On 2016/01/07 06:14:10, Yang wrote: > Not questioning the effectiveness, just curious: can we really consider it a > representative sample of live objects if we only observe allocation, but not > death of objects? We do observe death as well. We keep a weak reference the the sampled object. At any point in time the 'set of samples' is the set of objects we have sampled that is still alive. Do you think the comment here is misleading? I can fix it if that's the case. https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:560: void GetHeapSample(OutputStream* stream); On 2016/01/07 06:14:10, Yang wrote: > I wonder whether it makes better sense to use the same format as the heap > snapshot uses. You can still turn it into JSON in post-processing. That way, we > could use it in devtools as well, as sort of light-weight heap snapshot that can > be always-on whenever devtools is active. One of the difference is that heap-snapshot gives you the object graph, whereas the sampling heap profiler gives you list of allocations currently live along with the allocation stack trace. It does not give you the connections between the objects (which a sampling of the objects cannot do). I chose the present format (JSON string pushed to an OutputStream) to make it similar to AllocationTracker as conceptually the SamplingHeapProfiler is closer to AllocationTracker.
https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:529: * currently live in the system. Each sampled allocation includes the stack On 2016/01/07 at 06:49:30, ofrobots wrote: > On 2016/01/07 06:14:10, Yang wrote: > > Not questioning the effectiveness, just curious: can we really consider it a > > representative sample of live objects if we only observe allocation, but not > > death of objects? > > We do observe death as well. We keep a weak reference the the sampled object. At any point in time the 'set of samples' is the set of objects we have sampled that is still alive. Do you think the comment here is misleading? I can fix it if that's the case. note that it's not possible to create weak references to all objects: https://bugs.chromium.org/p/v8/issues/detail?id=3647
On 2016/01/07 10:29:34, jochen wrote: > https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h > File include/v8-profiler.h (right): > > https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h#n... > include/v8-profiler.h:529: * currently live in the system. Each sampled > allocation includes the stack > On 2016/01/07 at 06:49:30, ofrobots wrote: > > On 2016/01/07 06:14:10, Yang wrote: > > > Not questioning the effectiveness, just curious: can we really consider it a > > > representative sample of live objects if we only observe allocation, but not > > > death of objects? > > > > We do observe death as well. We keep a weak reference the the sampled object. > At any point in time the 'set of samples' is the set of objects we have sampled > that is still alive. Do you think the comment here is misleading? I can fix it > if that's the case. > > note that it's not possible to create weak references to all objects: > https://bugs.chromium.org/p/v8/issues/detail?id=3647 That's an interesting bug. Do you think this has a chance of getting fixed? Is that bug only possible from native code?
On 2016/01/07 at 14:04:47, ofrobots wrote: > On 2016/01/07 10:29:34, jochen wrote: > > https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h > > File include/v8-profiler.h (right): > > > > https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h#n... > > include/v8-profiler.h:529: * currently live in the system. Each sampled > > allocation includes the stack > > On 2016/01/07 at 06:49:30, ofrobots wrote: > > > On 2016/01/07 06:14:10, Yang wrote: > > > > Not questioning the effectiveness, just curious: can we really consider it a > > > > representative sample of live objects if we only observe allocation, but not > > > > death of objects? > > > > > > We do observe death as well. We keep a weak reference the the sampled object. > > At any point in time the 'set of samples' is the set of objects we have sampled > > that is still alive. Do you think the comment here is misleading? I can fix it > > if that's the case. > > > > note that it's not possible to create weak references to all objects: > > https://bugs.chromium.org/p/v8/issues/detail?id=3647 > > That's an interesting bug. Do you think this has a chance of getting fixed? Is that bug only possible from native code? it's only possible from native code (as JS code can't set weak references...) I guess it should be possible to fix this. Do you want to come up with a patch?
On 2016/01/07 06:49:30, ofrobots wrote: > I chose the present format (JSON string pushed to an OutputStream) to make it > similar to AllocationTracker as conceptually the SamplingHeapProfiler is closer > to AllocationTracker. Just my EUR 0.02 but as a potential (in-process) user of this API, I'd prefer avoiding the cost of deserializing data that V8 painstakingly serialized just a few cycles before. On the other hand, JSON strings make maintaining binary compatibility easier and there is something to be said for that.
On 2016/01/07 15:31:59, noordhuis wrote: > On 2016/01/07 06:49:30, ofrobots wrote: > > I chose the present format (JSON string pushed to an OutputStream) to make it > > similar to AllocationTracker as conceptually the SamplingHeapProfiler is > closer > > to AllocationTracker. > > Just my EUR 0.02 but as a potential (in-process) user of this API, I'd prefer > avoiding the cost of deserializing data that V8 painstakingly serialized just a > few cycles before. > > On the other hand, JSON strings make maintaining binary compatibility easier and > there is something to be said for that. Let me try implementing a C++ based interface and see how that looks. Ownership of data needs to be carefully considered and some (most?) copying is going to be unavoidable. This is likely going to cost a similar amount to what serialization costs today, but at least you'll be able to avoid the deserialization cost. If others have opinions one or way or the other, those would be good to know.
On 2016/01/07 14:06:03, jochen wrote: > On 2016/01/07 at 14:04:47, ofrobots wrote: > > On 2016/01/07 10:29:34, jochen wrote: > > > https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h > > > File include/v8-profiler.h (right): > > > > > > > https://codereview.chromium.org/1555553002/diff/20001/include/v8-profiler.h#n... > > > include/v8-profiler.h:529: * currently live in the system. Each sampled > > > allocation includes the stack > > > On 2016/01/07 at 06:49:30, ofrobots wrote: > > > > On 2016/01/07 06:14:10, Yang wrote: > > > > > Not questioning the effectiveness, just curious: can we really consider > it a > > > > > representative sample of live objects if we only observe allocation, but > not > > > > > death of objects? > > > > > > > > We do observe death as well. We keep a weak reference the the sampled > object. > > > At any point in time the 'set of samples' is the set of objects we have > sampled > > > that is still alive. Do you think the comment here is misleading? I can fix > it > > > if that's the case. > > > > > > note that it's not possible to create weak references to all objects: > > > https://bugs.chromium.org/p/v8/issues/detail?id=3647 > > > > That's an interesting bug. Do you think this has a chance of getting fixed? Is > that bug only possible from native code? > > it's only possible from native code (as JS code can't set weak references...) > > I guess it should be possible to fix this. Do you want to come up with a patch? I can try implementing a fix, albeit a bit later as I do not think this is on the critical path for me. On-heap allocations that I am sampling today aren't going to be hit with this issue.
This is ready for review.
I defer to Yang and Hannes
On 2016/01/15 14:39:22, jochen wrote: > I defer to Yang and Hannes I've been busy the whole day. But I have not forgotten about this!
LGTM from the GC side https://codereview.chromium.org/1555553002/diff/100001/test/cctest/test-heap-... File test/cctest/test-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/100001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2876: // static void PrintNode(const v8::AllocationProfile::Node* node, int indent) { You probably don't need that piece anymore.
LGTM as well. https://codereview.chromium.org/1555553002/diff/100001/test/cctest/test-heap-... File test/cctest/test-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/100001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2876: // static void PrintNode(const v8::AllocationProfile::Node* node, int indent) { On 2016/01/19 08:44:08, Hannes Payer wrote: > You probably don't need that piece anymore. Yup. Remove instead of comment out.
The CQ bit was checked by ofrobots@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555553002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555553002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/1...)
The CQ bit was checked by ofrobots@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555553002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555553002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)
https://codereview.chromium.org/1555553002/diff/120001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1555553002/diff/120001/include/v8-profiler.h#... include/v8-profiler.h:515: std::deque<Node> nodes; Can you move it to the implementation side so it's not exposed in the interface? https://codereview.chromium.org/1555553002/diff/120001/include/v8-profiler.h#... include/v8-profiler.h:659: * StartSamplingHeapProfiler was called. It is worth to describe the profile lifetime here. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:1: // Copyright 2015 the V8 project authors. All rights reserved. ditto https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:40: std::set<SampledAllocation*> empty{}; looks like a banned feature: http://chromium-cpp.appspot.com/ https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:71: // We sample with a Poisson process, with constant average sampling interval. Out of curiosity, why it is better to have a random sampling intervals rather than constant ones. E.g. CPU profilers usually use constant sampling interval. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:82: static_cast<intptr_t>(next)); looks like undefined behavior when next happens to be outside the intptr_t limits. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:178: Node({ToApiHandle<v8::String>(isolate_->factory()->InternalizeUtf8String( looks like a banned feature. http://chromium-cpp.appspot.com/ https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:1: // Copyright 2015 the V8 project authors. All rights reserved. Update copyright year. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:70: SamplingHeapProfiler* const shp_; could you please avoid abbreviations.
alph@ PTAL. https://codereview.chromium.org/1555553002/diff/120001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1555553002/diff/120001/include/v8-profiler.h#... include/v8-profiler.h:515: std::deque<Node> nodes; On 2016/01/19 23:43:57, alph wrote: > Can you move it to the implementation side so it's not exposed in the interface? Done. https://codereview.chromium.org/1555553002/diff/120001/include/v8-profiler.h#... include/v8-profiler.h:659: * StartSamplingHeapProfiler was called. On 2016/01/19 23:43:57, alph wrote: > It is worth to describe the profile lifetime here. Done. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:1: // Copyright 2015 the V8 project authors. All rights reserved. On 2016/01/19 23:43:57, alph wrote: > ditto This file was authored and published publicly (in the form of this CL) on Dec 29th 2015. The rule I am following is: "the year the file was created in full numeric form (e.g., 2010); don't change this if you edit, move, or copy the file." https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:40: std::set<SampledAllocation*> empty{}; On 2016/01/19 23:43:57, alph wrote: > looks like a banned feature: http://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:82: static_cast<intptr_t>(next)); On 2016/01/19 23:43:57, alph wrote: > looks like undefined behavior when next happens to be outside the intptr_t > limits. Done. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:178: Node({ToApiHandle<v8::String>(isolate_->factory()->InternalizeUtf8String( On 2016/01/19 23:43:57, alph wrote: > looks like a banned feature. http://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:1: // Copyright 2015 the V8 project authors. All rights reserved. On 2016/01/19 23:43:57, alph wrote: > Update copyright year. Likewise, this file was authored and published publicly in 2015. https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:70: SamplingHeapProfiler* const shp_; On 2016/01/19 23:43:57, alph wrote: > could you please avoid abbreviations. Done.
https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:71: // We sample with a Poisson process, with constant average sampling interval. On 2016/01/19 23:43:57, alph wrote: > Out of curiosity, why it is better to have a random sampling intervals rather > than constant ones. E.g. CPU profilers usually use constant sampling interval. The random sample interval makes sure that our observations don't bias towards any application phases. For example imagine a program that allocates, in a loop, 512 bytes of Point objects followed by an Array. If we sampled with an interval of 512 bytes, we will only observe the Array allocations and conclude that the program only allocates Arrays. This could affect the CPU profiler too, but is probably not that severe a problem because IO latencies add random-ish perturbations to thread-scheduling. Object allocation, on the other hand, typically can follow a very deterministic allocation pattern, and it is important that any periods in the application itself are not in phase with the sampling interval.
The CQ bit was checked by ofrobots@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555553002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555553002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
The CQ bit was checked by ofrobots@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555553002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555553002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
Thank you. Great work! Some more comments below. https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h#... include/v8-profiler.h:8: #include <string> not used? https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h#... include/v8-profiler.h:428: struct Allocation { Out of curiosity, does it need to be that fine grained, i.e. having both size and count? Moreover the count currently seems always be 1. CPU profiler does not hold per sample information, but aggregates right in place the time spend per stack. Why can't you do the same here? https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h#... include/v8-profiler.h:444: Node(Local<String> arg_name, Local<String> arg_script_name, I'd move it out of the interface, as the client is not supposed to create these nodes. https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h#... include/v8-profiler.h:452: children(), No need to explicitly initialize children and allocations. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/heap-prof... File src/profiler/heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/180001/src/profiler/heap-prof... src/profiler/heap-profiler.cc:8: #include "src/base/utils/random-number-generator.h" not used? https://codereview.chromium.org/1555553002/diff/180001/src/profiler/heap-prof... src/profiler/heap-profiler.cc:10: #include "src/frames-inl.h" ditto https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:84: : (next > INTPTR_MAX ? INTPTR_MAX : static_cast<intptr_t>(next)); I have a little concern here. The Poisson distribution works fine in a theoretical case where the stream is infinite. But in reality it could happen that we get a fairly large number for the next interval and we just end up having no samples collected for the profile. Though that's seems to only be possible when random returns zero, so the probability is quite low. But nevertheless, how about using some sane number instead of INTPTR_MAX? wdyt? https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:105: start_position_ = shared->start_position(); Can you always set start_position_? https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:135: switch (isolate->current_vm_state()) { Could you reuse StateToString here? https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/vm-state-in... https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:197: if (child->script_id == function_info->get_script_id() && Makes sense to use a map for children. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:234: kill extra line https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:238: FunctionInfo function_info("(root)", "", v8::UnboundScript::kNoScriptId, 0); Seems to be the last three parameters are never used. Kill them? https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:239: AllocateNode(*profile, scripts, &function_info); profile is an out parameter. Makes sense to pass it by pointer to emphasize that fact. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:11: #include "src/base/utils/random-number-generator.h" You can forward ref instead. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:13: #include "src/profiler/allocation-tracker.h" Is it used? https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... File test/cctest/test-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2903: heap_profiler->StartSamplingHeapProfiler(512 * 1024); This is a source for flakiness, as the interval is in fact a random number, so it may happen to collect no samples. Could you suppress randomness for the test or limit the top bound of the interval as I suggested above. Also provided that there will be just 2 samples on average, could that be they all fall into say (GC). https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2906: v8::AllocationProfile* profile = heap_profiler->GetAllocationProfile(); Please use SmartPointer wherever possible. https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2950: CHECK_GT(count_32kb, count_512kb); Again, this seems to be a source for the test flakiness.
https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h#... include/v8-profiler.h:8: #include <string> On 2016/01/20 23:13:01, alph wrote: > not used? Done. https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h#... include/v8-profiler.h:428: struct Allocation { On 2016/01/20 23:13:01, alph wrote: > Out of curiosity, does it need to be that fine grained, i.e. having both size > and count? Moreover the count currently seems always be 1. > > CPU profiler does not hold per sample information, but aggregates right in place > the time spend per stack. Why can't you do the same here? The goal is to have some flexibility in how much of aggregation want to do in the future. This design allows for us to provide fine grained or aggregated allocation information. This is what we plan do in the future: 1. Use a single bucket for objects for a given size. The count will stop being 1 in such a case. 2. Decide if we want to summarize objects of different sizes into the same bucket. This is probably going to controllable by the clients of the API. It is not clear that this should be the default though. For example if a particular allocation site allocates Buffer objects, one of size 10 the other of size 990, summarizing the data as '2 allocations with average size 500' loses information. This is not a concern for the CPU profiler because each sample at a particular point in the call-graph contains the same data; 'I happen to be here'. For allocation profiling we are gathering information more than that. The current API was chosen to allow the above to be possible. https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h#... include/v8-profiler.h:444: Node(Local<String> arg_name, Local<String> arg_script_name, On 2016/01/20 23:13:01, alph wrote: > I'd move it out of the interface, as the client is not supposed to create these > nodes. Done. https://codereview.chromium.org/1555553002/diff/180001/include/v8-profiler.h#... include/v8-profiler.h:452: children(), On 2016/01/20 23:13:01, alph wrote: > No need to explicitly initialize children and allocations. Acknowledged. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/heap-prof... File src/profiler/heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/180001/src/profiler/heap-prof... src/profiler/heap-profiler.cc:8: #include "src/base/utils/random-number-generator.h" On 2016/01/20 23:13:01, alph wrote: > not used? Done. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/heap-prof... src/profiler/heap-profiler.cc:10: #include "src/frames-inl.h" On 2016/01/20 23:13:01, alph wrote: > ditto Done. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:84: : (next > INTPTR_MAX ? INTPTR_MAX : static_cast<intptr_t>(next)); On 2016/01/20 23:13:01, alph wrote: > I have a little concern here. The Poisson distribution works fine in a > theoretical case where the stream is infinite. > > But in reality it could happen that we get a fairly large number for the next > interval and we just end up having no samples collected for the profile. > > Though that's seems to only be possible when random returns zero, so the > probability is quite low. But nevertheless, how about using some sane number > instead of INTPTR_MAX? wdyt? The probability of `next` being close to INTPTR_MAX is not just low, it is astronomically improbable given the poisson probability distribution. Regardless, I changed it to INT_MAX instead because INTPTR_MAX doesn't seem to be defined in android's stdint.h. INT_MAX is large enough, but this will break down if the clients of the profiler want to use a sample rate within an order of magnitude of INT_MAX. :/ https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:105: start_position_ = shared->start_position(); On 2016/01/20 23:13:01, alph wrote: > Can you always set start_position_? Done. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:135: switch (isolate->current_vm_state()) { On 2016/01/20 23:13:01, alph wrote: > Could you reuse StateToString here? > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/vm-state-in... The format returned by that isn't great for this purpose. The parentheses added here make sure the function names are distinguishable from real JS functions named 'GC' or 'COMPILER' etc. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:197: if (child->script_id == function_info->get_script_id() && On 2016/01/20 23:13:01, alph wrote: > Makes sense to use a map for children. I agree that this would be a nice optimization in the future. Do you think it should be done before this CL lands? https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:234: On 2016/01/20 23:13:01, alph wrote: > kill extra line Done. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:238: FunctionInfo function_info("(root)", "", v8::UnboundScript::kNoScriptId, 0); On 2016/01/20 23:13:01, alph wrote: > Seems to be the last three parameters are never used. Kill them? Done. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:239: AllocateNode(*profile, scripts, &function_info); On 2016/01/20 23:13:01, alph wrote: > profile is an out parameter. Makes sense to pass it by pointer to emphasize that > fact. Done. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:11: #include "src/base/utils/random-number-generator.h" On 2016/01/20 23:13:01, alph wrote: > You can forward ref instead. Done. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:13: #include "src/profiler/allocation-tracker.h" On 2016/01/20 23:13:01, alph wrote: > Is it used? Done. https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... File test/cctest/test-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2903: heap_profiler->StartSamplingHeapProfiler(512 * 1024); On 2016/01/20 23:13:01, alph wrote: > This is a source for flakiness, as the interval is in fact a random number, so > it may happen to collect no samples. Could you suppress randomness for the test > or limit the top bound of the interval as I suggested above. > > Also provided that there will be just 2 samples on average, could that be they > all fall into say (GC). I like the idea to suppress randomness some of the tests. I can pursue it, but hopefully in a follow-on CL? On 64-bit machines, we should see ~14-15 samples. On my machine, I did 100 runs, and I got exactly 15 samples on all 100 of those runs. For the 32KiB observer, I got 202 samples ~50% of the time and 203 samples otherwise. Also note that it is not expected that we will get any samples that fall into the '(GC)' category. GC doesn't allocate new objects, it moves objects across spaces. Those look like allocations from a given Space's point of view, but the profiler should deal with them correctly. In other words, yes, there would be some differences in exactly how many samples we capture, but this is fine for now? I can add tests that run with randomness suppressed in a follow-on CL. https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2906: v8::AllocationProfile* profile = heap_profiler->GetAllocationProfile(); On 2016/01/20 23:13:01, alph wrote: > Please use SmartPointer wherever possible. Done. https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2950: CHECK_GT(count_32kb, count_512kb); On 2016/01/20 23:13:01, alph wrote: > Again, this seems to be a source for the test flakiness. I am intentionally using a weak test here to avoid flakiness. I am expecting that this is fine for now, but in a follow-on CL I can suppress randomness for this type of tests.
Thanks a lot! lgtm https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:84: : (next > INTPTR_MAX ? INTPTR_MAX : static_cast<intptr_t>(next)); On 2016/01/21 03:03:28, ofrobots wrote: > On 2016/01/20 23:13:01, alph wrote: > > I have a little concern here. The Poisson distribution works fine in a > > theoretical case where the stream is infinite. > > > > But in reality it could happen that we get a fairly large number for the next > > interval and we just end up having no samples collected for the profile. > > > > Though that's seems to only be possible when random returns zero, so the > > probability is quite low. But nevertheless, how about using some sane number > > instead of INTPTR_MAX? wdyt? > > The probability of `next` being close to INTPTR_MAX is not just low, it is > astronomically improbable given the poisson probability distribution. > Regardless, I changed it to INT_MAX instead because INTPTR_MAX doesn't seem to > be defined in android's stdint.h. INT_MAX is large enough, but this will break > down if the clients of the profiler want to use a sample rate within an order of > magnitude of INT_MAX. :/ Well, the probability of getting a weird number is the probability of getting zero out of NextDouble which is 2^-52 or 10^-15, which is pretty low. The next possible value which is -log(2^-52) =~ 36 looks safe. https://codereview.chromium.org/1555553002/diff/180001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:197: if (child->script_id == function_info->get_script_id() && On 2016/01/21 03:03:28, ofrobots wrote: > On 2016/01/20 23:13:01, alph wrote: > > Makes sense to use a map for children. > > I agree that this would be a nice optimization in the future. Do you think it > should be done before this CL lands? It affect the API, so if you want to change it it's better to do it rather earlier than later. https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... File test/cctest/test-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/180001/test/cctest/test-heap-... test/cctest/test-heap-profiler.cc:2903: heap_profiler->StartSamplingHeapProfiler(512 * 1024); On 2016/01/21 03:03:28, ofrobots wrote: > On 2016/01/20 23:13:01, alph wrote: > > This is a source for flakiness, as the interval is in fact a random number, so > > it may happen to collect no samples. Could you suppress randomness for the > test > > or limit the top bound of the interval as I suggested above. > > > > Also provided that there will be just 2 samples on average, could that be they > > all fall into say (GC). > > I like the idea to suppress randomness some of the tests. I can pursue it, but > hopefully in a follow-on CL? > > On 64-bit machines, we should see ~14-15 samples. On my machine, I did 100 runs, > and I got exactly 15 samples on all 100 of those runs. For the 32KiB observer, > I got 202 samples ~50% of the time and 203 samples otherwise. > > Also note that it is not expected that we will get any samples that fall into > the '(GC)' category. GC doesn't allocate new objects, it moves objects across > spaces. Those look like allocations from a given Space's point of view, but the > profiler should deal with them correctly. > > In other words, yes, there would be some differences in exactly how many samples > we capture, but this is fine for now? I can add tests that run with randomness > suppressed in a follow-on CL. oh yes, it's 2 multiplies by sizeof(intptr_t) = 16. https://codereview.chromium.org/1555553002/diff/200001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1555553002/diff/200001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:166: Node node; kill it.
The CQ bit was checked by ofrobots@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org, yangguo@chromium.org, alph@chromium.org Link to the patchset: https://codereview.chromium.org/1555553002/#ps220001 (title: "remove unused variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555553002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555553002/220001
Message was sent while issue was closed.
Description was changed from ========== [profiler] Implement POC Sampling Heap Profiler This implements a proof-of-concept sampling based heap profiler inspired by tcmalloc's heap profiler [1] and Go's mprof/memprofile [2]. The basic idea is the sample allocations using a randomized Poisson process. At any point in time we can cheaply request the set of live sample objects that should be a representative sample of heap. Samples include stack-traces from the allocation sites, making this an effective tool for memory leak debugging. Unlike AllocationTracking, this is intended to be cheap and usable online in production. The proof-of-concept is only sampling new-space allocations at this point. Support for sampling paged space and native allocations is anticipated in the future. [1] http://goog-perftools.sourceforge.net/doc/heap_profiler.html [2] http://blog.golang.org/profiling-go-programs ========== to ========== [profiler] Implement POC Sampling Heap Profiler This implements a proof-of-concept sampling based heap profiler inspired by tcmalloc's heap profiler [1] and Go's mprof/memprofile [2]. The basic idea is the sample allocations using a randomized Poisson process. At any point in time we can cheaply request the set of live sample objects that should be a representative sample of heap. Samples include stack-traces from the allocation sites, making this an effective tool for memory leak debugging. Unlike AllocationTracking, this is intended to be cheap and usable online in production. The proof-of-concept is only sampling new-space allocations at this point. Support for sampling paged space and native allocations is anticipated in the future. [1] http://goog-perftools.sourceforge.net/doc/heap_profiler.html [2] http://blog.golang.org/profiling-go-programs ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [profiler] Implement POC Sampling Heap Profiler This implements a proof-of-concept sampling based heap profiler inspired by tcmalloc's heap profiler [1] and Go's mprof/memprofile [2]. The basic idea is the sample allocations using a randomized Poisson process. At any point in time we can cheaply request the set of live sample objects that should be a representative sample of heap. Samples include stack-traces from the allocation sites, making this an effective tool for memory leak debugging. Unlike AllocationTracking, this is intended to be cheap and usable online in production. The proof-of-concept is only sampling new-space allocations at this point. Support for sampling paged space and native allocations is anticipated in the future. [1] http://goog-perftools.sourceforge.net/doc/heap_profiler.html [2] http://blog.golang.org/profiling-go-programs ========== to ========== [profiler] Implement POC Sampling Heap Profiler This implements a proof-of-concept sampling based heap profiler inspired by tcmalloc's heap profiler [1] and Go's mprof/memprofile [2]. The basic idea is the sample allocations using a randomized Poisson process. At any point in time we can cheaply request the set of live sample objects that should be a representative sample of heap. Samples include stack-traces from the allocation sites, making this an effective tool for memory leak debugging. Unlike AllocationTracking, this is intended to be cheap and usable online in production. The proof-of-concept is only sampling new-space allocations at this point. Support for sampling paged space and native allocations is anticipated in the future. [1] http://goog-perftools.sourceforge.net/doc/heap_profiler.html [2] http://blog.golang.org/profiling-go-programs Committed: https://crrev.com/e5a9947811db9c9e23557dbad27f8b8a349b3262 Cr-Commit-Position: refs/heads/master@{#33448} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e5a9947811db9c9e23557dbad27f8b8a349b3262 Cr-Commit-Position: refs/heads/master@{#33448}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1615173002/ by ofrobots@google.com. The reason for reverting is: The random nature of the tests caused the following buildbot to fail: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20gcc%204.8/builds.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
