Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1697903002:
Sampling heap profiler data structure changes (Closed)
|
Created:
4 years, 10 months ago by mattloring Modified:
4 years, 10 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionSampling heap profiler data structure changes
Previously, the sampling heap profiler stored a list of samples and then
built a tree representation when the profile was queried by calling
GetAllocationProfile. This change reduces duplication by removing stacks
from all samples. Also, less information is stored in the tree
maintained by the profiler and remaining information (script name, line
no, etc) is resolved when a profile is requested.
BUG=
Committed: https://crrev.com/cdd55e2a3717723492d76f66810bf56b8de7f198
Cr-Commit-Position: refs/heads/master@{#34119}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Namespaces fixes and globals cleanup #Patch Set 3 : Remove unused name from FunctionInfo #Patch Set 4 : Cleaned up leaking samples and function infos #Patch Set 5 : Remove functioninfo #
Total comments: 12
Patch Set 6 : Make sample no-assign and address comments #Patch Set 7 : Construct each global once #Patch Set 8 : Store profile_root_ by value #
Total comments: 14
Patch Set 9 : Constify things #
Total comments: 6
Patch Set 10 : More efficient samples_ cleanup #
Messages
Total messages: 46 (16 generated)
Description was changed from ========== Sampling heap profiler data structure changes Previously, the sampling heap profiler stored a list of samples and then built a tree representation when the profile was queried by calling GetAllocationProfile. This change reduces duplication by removing stacks from all samples. Also, less information is stored in the tree maintained by the profiler and remaining information (script name, line no, etc) is resolved when a profile is requested. BUG= ========== to ========== Sampling heap profiler data structure changes Previously, the sampling heap profiler stored a list of samples and then built a tree representation when the profile was queried by calling GetAllocationProfile. This change reduces duplication by removing stacks from all samples. Also, less information is stored in the tree maintained by the profiler and remaining information (script name, line no, etc) is resolved when a profile is requested. BUG= ==========
mattloring@google.com changed reviewers: + ofrobots@google.com
https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.cc (left): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:70: } Are we still clearing the weak references we have been holding when the SamplingHeapProfiler is destructed? If not, this is going to leak memory in V8 because of the book-keeping needed by V8 to deal with weak handles. Also, weak callbacks will get called later when you don't expect them (as the SamplingHeapProfiler no longer exists) and may end up crashing. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:47: profile_root_ = new SamplingHeapProfiler::AllocationNode(&function_info); drop the SamplingHeapProfiler:: namespace prefix here. The compiler will resolve this correctly in this context. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:89: Sample* sample = new Sample({size, node, global}); Prefereable to avoid list initialization. Prefer either a constructor, or simply: Sample* sample = {...}; https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:136: const WeakCallbackInfo<SamplingHeapProfiler::Sample>& data) { You might be able to drop the SHP:: prefix in the argument list too. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:138: SamplingHeapProfiler::AllocationNode* node = sample->owner; Drop the SHP:: prefix. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:147: script_name_(""), Do we ever use this field any more? Drop it if you don't need it. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:162: for (SamplingHeapProfiler::AllocationNode* child : parent->children_) { drop the prefix. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:167: SamplingHeapProfiler::AllocationNode* child = Drop the SamplingHeapProfiler prefix. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:175: SamplingHeapProfiler::AllocationNode* node = profile_root_; drop the prefix. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:193: std::vector<v8::AllocationProfile::Allocation> allocations; In a follow-on CL, it would be good to make the V8 API change to make a AllocationProfile::Node hold a map from size to int instead of the vector of allocations as you are doing internally. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:195: auto non_const_scripts = const_cast<std::map<int, Script*>&>(scripts); You dropped the necessary comment explaining why these ugly const gymnastics are necessary. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:77: Global<Value>* global; It is more idiomatic to keep Globals (and Locals and Persistents) by value rather than pointer. I suspect the reason you are keeping a pointer is to get around issues of Global (and hence Sample) not being copy-able. That limitation is by design. Instead add a constructor so that you can initialize Global (as value) in place. It would also be good to explicitly mark Sample as not copyable (by using DISALLOW_COPY_AND_ASSIGN).
https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.cc (left): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:70: } On 2016/02/14 23:57:42, ofrobots wrote: > Are we still clearing the weak references we have been holding when the > SamplingHeapProfiler is destructed? If not, this is going to leak memory in V8 > because of the book-keeping needed by V8 to deal with weak handles. Also, weak > callbacks will get called later when you don't expect them (as the > SamplingHeapProfiler no longer exists) and may end up crashing. Globals should be cleaned up now. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:47: profile_root_ = new SamplingHeapProfiler::AllocationNode(&function_info); On 2016/02/14 23:57:42, ofrobots wrote: > drop the SamplingHeapProfiler:: namespace prefix here. The compiler will resolve > this correctly in this context. Done. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:89: Sample* sample = new Sample({size, node, global}); On 2016/02/14 23:57:42, ofrobots wrote: > Prefereable to avoid list initialization. Prefer either a constructor, or > simply: > Sample* sample = {...}; Doing it as {...} yields: error: excess elements in scalar initializer and writing a constructor runs into problems with the lack of copy constructor in Global. Is there a nice way to address either of these? https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:136: const WeakCallbackInfo<SamplingHeapProfiler::Sample>& data) { On 2016/02/14 23:57:42, ofrobots wrote: > You might be able to drop the SHP:: prefix in the argument list too. Done. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:138: SamplingHeapProfiler::AllocationNode* node = sample->owner; On 2016/02/14 23:57:42, ofrobots wrote: > Drop the SHP:: prefix. Done. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:162: for (SamplingHeapProfiler::AllocationNode* child : parent->children_) { On 2016/02/14 23:57:42, ofrobots wrote: > drop the prefix. Done. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:167: SamplingHeapProfiler::AllocationNode* child = On 2016/02/14 23:57:42, ofrobots wrote: > Drop the SamplingHeapProfiler prefix. Done. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:175: SamplingHeapProfiler::AllocationNode* node = profile_root_; On 2016/02/14 23:57:42, ofrobots wrote: > drop the prefix. Done. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:193: std::vector<v8::AllocationProfile::Allocation> allocations; On 2016/02/14 23:57:42, ofrobots wrote: > In a follow-on CL, it would be good to make the V8 API change to make a > AllocationProfile::Node hold a map from size to int instead of the vector of > allocations as you are doing internally. Yup, this change was in the original version of the CL but I think a follow-on is more appropriate. https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:77: Global<Value>* global; On 2016/02/14 23:57:42, ofrobots wrote: > It is more idiomatic to keep Globals (and Locals and Persistents) by value > rather than pointer. I suspect the reason you are keeping a pointer is to get > around issues of Global (and hence Sample) not being copy-able. That limitation > is by design. Instead add a constructor so that you can initialize Global (as > value) in place. It would also be good to explicitly mark Sample as not copyable > (by using DISALLOW_COPY_AND_ASSIGN). Globals are held by value now. How does DISALLOW_COPY_AND_ASSIGN work with structs? Where should the call be?
https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:147: script_name_(""), On 2016/02/14 23:57:42, ofrobots wrote: > Do we ever use this field any more? Drop it if you don't need it. Done.
mattloring@google.com changed reviewers: + hpayer@chromium.org, ulan@chromium.org
The CQ bit was checked by mattloring@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/1697903002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697903002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
The CQ bit was checked by mattloring@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/1697903002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697903002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mattloring@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/1697903002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697903002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:68: for (auto it = samples_.begin(); it != samples_.end(); ++it) { We can use shorter version of for to be consistent with other loops. https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:114: child->script_position_ == start_position) DCHECK that names match? https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:57: SamplingHeapProfiler* profiler; Disallow copy and assign. https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:62: explicit AllocationNode(const char* const name, int script_id, No need for explicit here. https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:93: v8::AllocationProfile::Node* GenerateProfile( Could you please add a comment describing pre/post conditions of this function? i.e. what is node, what is expected to be in scripts, what is returned.
https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:89: Sample* sample = new Sample({size, node, global}); On 2016/02/16 05:28:40, mattloring wrote: > On 2016/02/14 23:57:42, ofrobots wrote: > > Prefereable to avoid list initialization. Prefer either a constructor, or > > simply: > > Sample* sample = {...}; > > Doing it as {...} yields: error: excess elements in scalar initializer and > writing a constructor runs into problems with the lack of copy constructor in > Global. Is there a nice way to address either of these? You will need to use a constructor that accepts the necessary arguments to build a Global rather than passing a global. It is by design that Globals cannot be copied or assigned.
https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:68: for (auto it = samples_.begin(); it != samples_.end(); ++it) { On 2016/02/17 11:14:50, ulan wrote: > We can use shorter version of for to be consistent with other loops. Done. https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:114: child->script_position_ == start_position) On 2016/02/17 11:14:50, ulan wrote: > DCHECK that names match? Done. https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:57: SamplingHeapProfiler* profiler; On 2016/02/17 11:14:50, ulan wrote: > Disallow copy and assign. Done. https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:62: explicit AllocationNode(const char* const name, int script_id, On 2016/02/17 11:14:50, ulan wrote: > No need for explicit here. Done. https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:93: v8::AllocationProfile::Node* GenerateProfile( On 2016/02/17 11:14:50, ulan wrote: > Could you please add a comment describing pre/post conditions of this function? > i.e. what is node, what is expected to be in scripts, what is returned. Done.
https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:47: profile_root_ = The lifetime of this allocation seems to match the lifetime of SamplingHeapProfiler. You man want to keep profile_root_ as value rather than a pointer and avoid having to manually new/delete.
https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.cc:89: Sample* sample = new Sample({size, node, global}); On 2016/02/17 17:48:21, ofrobots wrote: > On 2016/02/16 05:28:40, mattloring wrote: > > On 2016/02/14 23:57:42, ofrobots wrote: > > > Prefereable to avoid list initialization. Prefer either a constructor, or > > > simply: > > > Sample* sample = {...}; > > > > Doing it as {...} yields: error: excess elements in scalar initializer and > > writing a constructor runs into problems with the lack of copy constructor in > > Global. Is there a nice way to address either of these? > > You will need to use a constructor that accepts the necessary arguments to build > a Global rather than passing a global. It is by design that Globals cannot be > copied or assigned. Done.
https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/80001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:47: profile_root_ = On 2016/02/17 17:54:29, ofrobots wrote: > The lifetime of this allocation seems to match the lifetime of > SamplingHeapProfiler. You man want to keep profile_root_ as value rather than a > pointer and avoid having to manually new/delete. Done.
https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (left): https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:81: global_.Reset(); // drop the reference. You missed this. You should drop the reference in the destructor for Sample. https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:62: size_t size; const size_t https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:63: AllocationNode* owner; AllocationNode* const owner; https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:65: SamplingHeapProfiler* profiler; SamplingHeapProfiler* const profiler; https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:87: int script_id_; const int script_id; https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:88: int script_position_; const int start_position_; https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:89: const char* name_; const char* const name_;
https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (left): https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:81: global_.Reset(); // drop the reference. On 2016/02/17 22:34:49, ofrobots wrote: > You missed this. You should drop the reference in the destructor for Sample. Done. https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:62: size_t size; On 2016/02/17 22:34:49, ofrobots wrote: > const size_t Done. https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:63: AllocationNode* owner; On 2016/02/17 22:34:49, ofrobots wrote: > AllocationNode* const owner; Done. https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:65: SamplingHeapProfiler* profiler; On 2016/02/17 22:34:49, ofrobots wrote: > SamplingHeapProfiler* const profiler; Done. https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:87: int script_id_; On 2016/02/17 22:34:49, ofrobots wrote: > const int script_id; Done. https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:88: int script_position_; On 2016/02/17 22:34:49, ofrobots wrote: > const int start_position_; Done. https://codereview.chromium.org/1697903002/diff/140001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:89: const char* name_; On 2016/02/17 22:34:49, ofrobots wrote: > const char* const name_; Done.
The CQ bit was checked by mattloring@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/1697903002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697903002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/3077)
https://codereview.chromium.org/1697903002/diff/160001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/160001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:70: samples_.clear(); nit: It is more efficient to do a swap with an empty set. See the code on the left. https://codereview.chromium.org/1697903002/diff/160001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:98: node->allocations_[sample->size]--; map::operator[] silently insert a new entry in the map if it doesn't already exist. Add a DCHECK before this point to ensure node->allocations_[sample->size] != 0. https://codereview.chromium.org/1697903002/diff/160001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:109: strcmp(child->name_, name) == 0) { I think the earlier comment from Ulan was to DCHECK that the names match. I don't think you should do strcmp here. This should indeed be a DCHECK. If you ever run into a situation where the script id and start position are the same but the name is different, there is something fundamentally wrong with our understanding of the universe. The best action would be to abort. Hence a DCHECK.
https://codereview.chromium.org/1697903002/diff/160001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1697903002/diff/160001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:70: samples_.clear(); On 2016/02/18 01:02:24, ofrobots wrote: > nit: It is more efficient to do a swap with an empty set. See the code on the > left. Done. https://codereview.chromium.org/1697903002/diff/160001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:98: node->allocations_[sample->size]--; On 2016/02/18 01:02:24, ofrobots wrote: > map::operator[] silently insert a new entry in the map if it doesn't already > exist. Add a DCHECK before this point to ensure node->allocations_[sample->size] > != 0. Done. https://codereview.chromium.org/1697903002/diff/160001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:109: strcmp(child->name_, name) == 0) { On 2016/02/18 01:02:24, ofrobots wrote: > I think the earlier comment from Ulan was to DCHECK that the names match. > > I don't think you should do strcmp here. This should indeed be a DCHECK. If you > ever run into a situation where the script id and start position are the same > but the name is different, there is something fundamentally wrong with our > understanding of the universe. The best action would be to abort. Hence a > DCHECK. Actually, it is possible for two scripts to have the same id but different names. Right now our custom entries for (JS), (GC), ... all have a script_id of kNoScriptId. The strcmp was added because we were incorrectly attributing ticks to the wrong node.
The CQ bit was checked by mattloring@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/1697903002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697903002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/02/18 02:01:12, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. lgtm.
lgtm
The CQ bit was checked by mattloring@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1697903002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697903002/180001
Message was sent while issue was closed.
Description was changed from ========== Sampling heap profiler data structure changes Previously, the sampling heap profiler stored a list of samples and then built a tree representation when the profile was queried by calling GetAllocationProfile. This change reduces duplication by removing stacks from all samples. Also, less information is stored in the tree maintained by the profiler and remaining information (script name, line no, etc) is resolved when a profile is requested. BUG= ========== to ========== Sampling heap profiler data structure changes Previously, the sampling heap profiler stored a list of samples and then built a tree representation when the profile was queried by calling GetAllocationProfile. This change reduces duplication by removing stacks from all samples. Also, less information is stored in the tree maintained by the profiler and remaining information (script name, line no, etc) is resolved when a profile is requested. BUG= ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Sampling heap profiler data structure changes Previously, the sampling heap profiler stored a list of samples and then built a tree representation when the profile was queried by calling GetAllocationProfile. This change reduces duplication by removing stacks from all samples. Also, less information is stored in the tree maintained by the profiler and remaining information (script name, line no, etc) is resolved when a profile is requested. BUG= ========== to ========== Sampling heap profiler data structure changes Previously, the sampling heap profiler stored a list of samples and then built a tree representation when the profile was queried by calling GetAllocationProfile. This change reduces duplication by removing stacks from all samples. Also, less information is stored in the tree maintained by the profiler and remaining information (script name, line no, etc) is resolved when a profile is requested. BUG= Committed: https://crrev.com/cdd55e2a3717723492d76f66810bf56b8de7f198 Cr-Commit-Position: refs/heads/master@{#34119} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cdd55e2a3717723492d76f66810bf56b8de7f198 Cr-Commit-Position: refs/heads/master@{#34119}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/1708363002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Speculative revert for cpu profiler crashes on chromebooks: https://build.chromium.org/p/client.v8/builders/V8%20Arm%20-%20debug/builds/549 https://build.chromium.org/p/client.v8/builders/V8%20Arm%20-%20debug/builds/550. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
