Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1113233002:
Adding api to get last gc object statistics for chrome://tracing. (Closed)
DescriptionAdding api to get last gc object statistics for chrome://tracing.
For dumping the number of objects and size of objects alive after the
last gc into chrome://tracing memory dumps, this CL adds new api to
get these number for each isolate.
Note: Chrome should be run with --js-flags='--track_gc_object_stats
--noincremental-marking' for object tracking.
BUG=476013
LOG=Y
Committed: https://crrev.com/66083dd05ff812f3402758340bfab18e4f75fa50
Cr-Commit-Position: refs/heads/master@{#28474}
Patch Set 1 #Patch Set 2 : Adding type name. #
Total comments: 9
Patch Set 3 : Fixing comments. #
Total comments: 18
Patch Set 4 : Addressing ulan's comments. #
Total comments: 1
Patch Set 5 : Fixing api name and rebase #
Total comments: 1
Patch Set 6 : Returning subtypes as different strings. #Patch Set 7 : Returning subtypes as different strings. #
Total comments: 4
Patch Set 8 : Fixing the code types. #
Total comments: 1
Patch Set 9 : Fixing the comment with NumberOfTrackedHeapObjectTypes. #
Messages
Total messages: 30 (7 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org, rmcilroy@chromium.org
Adding object count if track_gc_object_stats flag is set.
ssid@chromium.org changed reviewers: + picksi@chromium.org - rmcilroy@chromium.org
PTAL. thanks.
picksi@google.com changed reviewers: + picksi@google.com
The #define/switch Voodoo is a bit scary, but looks like a common pattern :) https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1021: return object_counts_last_time_[type]; Should we DCHECK the array bounds here for safety? e.g. type>=0 and type<object_types?
Makes sense to me % some comments. Let's add some V8 folk here? https://codereview.chromium.org/1113233002/diff/20001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1113233002/diff/20001/include/v8.h#newcode5222 include/v8.h:5222: size_t NumberOfHeapObjectsTypes(); This doesn't seem to be defined anywhere ? https://codereview.chromium.org/1113233002/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1113233002/diff/20001/src/api.cc#newcode6946 src/api.cc:6946: if (object_type == nullptr) { Is object type ever expected to be null? If so why does it imply that object count should be 0? Maybe add a comment https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1455: const char* GetObjectTypeName(size_t index); Should this be near the other ones you are adding?
comments inline. https://codereview.chromium.org/1113233002/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1113233002/diff/20001/src/api.cc#newcode6928 src/api.cc:6928: size_t Isolate::NumberOfHeapObjectsTypes() { primiano@ here is the definition. https://codereview.chromium.org/1113233002/diff/20001/src/api.cc#newcode6946 src/api.cc:6946: if (object_type == nullptr) { On 2015/05/05 14:17:53, Primiano Tucci wrote: > Is object type ever expected to be null? If so why does it imply that object > count should be 0? Maybe add a comment The object type can be null in some cases, there are 241 object types defined in multiple macros. but they are not continuous sequences. DCHECK here is to check if no object type exists that is counted but not named. will write a comment. https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1455: const char* GetObjectTypeName(size_t index); On 2015/05/05 14:17:53, Primiano Tucci wrote: > Should this be near the other ones you are adding? the previous function does the same kind of operations. that is why this function is added here. if the way in which the type index is handled changes, then these functions change together. Note: I am not sure if this function should be a part of Heap class. Maybe this should be in objects.h . Waiting for owner's comment on that.
ssid@chromium.org changed reviewers: + ulan@chromium.org
ulan@ : Please review the changes. Note: The function Heap::GetObjectTypeName is implemented in Heap since the OBJECT_STATS_COUNT is defined in here and the function Heap::CheckpointObjectStats does similar indexing. So, it will be easier to change these both together when rquired. But I also think objects.h is the correct place for this function. WDYT? https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1021: return object_counts_last_time_[type]; On 2015/05/05 14:01:55, picksi wrote: > Should we DCHECK the array bounds here for safety? e.g. type>=0 and > type<object_types? Done.
On 2015/05/05 14:48:01, ssid wrote: > ulan@ : Please review the changes. > > Note: > The function Heap::GetObjectTypeName is implemented in Heap since the > OBJECT_STATS_COUNT is defined in here and the function > Heap::CheckpointObjectStats does similar indexing. So, it will be easier to > change these both together when rquired. But I also think objects.h is the > correct place for this function. WDYT? > > https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h > File src/heap/heap.h (right): > > https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h#newcode... > src/heap/heap.h:1021: return object_counts_last_time_[type]; > On 2015/05/05 14:01:55, picksi wrote: > > Should we DCHECK the array bounds here for safety? e.g. type>=0 and > > type<object_types? > > Done. Thanks!
In description: > Note: Chrome should be run with --js-flags='--track_gc_object_stats' for object tracking. should be --js-flags='--track_gc_object_stats --noincremental-marking'. With incremental marking we will not correct stats. https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1113233002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1455: const char* GetObjectTypeName(size_t index); On 2015/05/05 14:26:16, ssid wrote: > On 2015/05/05 14:17:53, Primiano Tucci wrote: > > Should this be near the other ones you are adding? > > the previous function does the same kind of operations. that is why this > function is added here. if the way in which the type index is handled changes, > then these functions change together. > Note: I am not sure if this function should be a part of Heap class. Maybe this > should be in objects.h . Waiting for owner's comment on that. This place is correct. The "index" has meaning only in Heap because it is the index in the object-stat table. https://codereview.chromium.org/1113233002/diff/30005/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1113233002/diff/30005/include/v8.h#newcode4821 include/v8.h:4821: size_t object_size() { return object_size_; } You probably also want a notion of sub-type or secondary type. For example a single code object can have multiple HeapObjectStatistics: - one with type "CODE_TYPE", - one with type "CODE_TYPE_<code-kind>", - one with type "CODE_TYPE_<code-age". So HeapObjectStatistics can share objects. Sum of object_size() will not be the correct total as it double counts some objects. https://codereview.chromium.org/1113233002/diff/30005/include/v8.h#newcode5222 include/v8.h:5222: size_t NumberOfHeapObjectsTypes(); Strictly speaking this is not the number of heap object types. This is the number of heap object statistics that we track (which is more than the actual heap object types). Can we make it more explicit in the name that this is about statistics and not generic heap object types? https://codereview.chromium.org/1113233002/diff/30005/include/v8.h#newcode5233 include/v8.h:5233: bool GetLastGcObjectStatistics(HeapObjectStatistics* object_statistics, GetHeapObjectStatisticsAtLastGC(...) to be consistent with other GetHeap*Statistics functions. https://codereview.chromium.org/1113233002/diff/30005/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1113233002/diff/30005/src/api.cc#newcode6930 src/api.cc:6930: } Two new-lines between functions. Please run "git cl format" to fix style. https://codereview.chromium.org/1113233002/diff/30005/src/api.cc#newcode6935 src/api.cc:6935: return false; Either use {} or put return statements in the previous line. https://codereview.chromium.org/1113233002/diff/30005/src/api.cc#newcode6946 src/api.cc:6946: if (object_type == nullptr) { This case shouldn't be happening. Could you please post here which index has no type? https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.cc#newcod... src/heap/heap.cc:6445: if (index > OBJECT_STATS_COUNT) index >= OBJECT_STATS_COUNT https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.cc#newcod... src/heap/heap.cc:6449: #define ADJUST_LAST_TIME_OBJECT_COUNT(name) \ The name "ADJUST_LAST_TIME_OBJECT_COUNT" is confusing here. Maybe "COMPARE_AND_RETURN_NAME" or "CASE_NAME" ? https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.h#newcode... src/heap/heap.h:1020: size_t object_count_last_gc(size_t type) { s/type/index/ here and below
Made changes as discussed. PTAL. https://codereview.chromium.org/1113233002/diff/30005/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1113233002/diff/30005/include/v8.h#newcode4821 include/v8.h:4821: size_t object_size() { return object_size_; } On 2015/05/06 12:46:02, ulan wrote: > You probably also want a notion of sub-type or secondary type. For example a > single code object can have multiple HeapObjectStatistics: > - one with type "CODE_TYPE", > - one with type "CODE_TYPE_<code-kind>", > - one with type "CODE_TYPE_<code-age". > > So HeapObjectStatistics can share objects. Sum of object_size() will not be the > correct total as it double counts some objects. > > As discussed offline, the sub-types are just given by the way the object is named, so this object remains the same. https://codereview.chromium.org/1113233002/diff/30005/include/v8.h#newcode5222 include/v8.h:5222: size_t NumberOfHeapObjectsTypes(); On 2015/05/06 12:46:02, ulan wrote: > Strictly speaking this is not the number of heap object types. This is the > number of heap object statistics that we track (which is more than the actual > heap object types). Can we make it more explicit in the name that this is about > statistics and not generic heap object types? Done. https://codereview.chromium.org/1113233002/diff/30005/include/v8.h#newcode5233 include/v8.h:5233: bool GetLastGcObjectStatistics(HeapObjectStatistics* object_statistics, On 2015/05/06 12:46:02, ulan wrote: > GetHeapObjectStatisticsAtLastGC(...) to be consistent with other > GetHeap*Statistics functions. Done. https://codereview.chromium.org/1113233002/diff/30005/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1113233002/diff/30005/src/api.cc#newcode6930 src/api.cc:6930: } On 2015/05/06 12:46:02, ulan wrote: > Two new-lines between functions. Please run "git cl format" to fix style. Done. https://codereview.chromium.org/1113233002/diff/30005/src/api.cc#newcode6935 src/api.cc:6935: return false; On 2015/05/06 12:46:02, ulan wrote: > Either use {} or put return statements in the previous line. Done. https://codereview.chromium.org/1113233002/diff/30005/src/api.cc#newcode6946 src/api.cc:6946: if (object_type == nullptr) { On 2015/05/06 12:46:02, ulan wrote: > This case shouldn't be happening. Could you please post here which index has no > type? As discussed offline, there are string categories that are not tracked/named. So, this DCHECK helps to check if the objects are actually tracked. https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.cc#newcod... src/heap/heap.cc:6445: if (index > OBJECT_STATS_COUNT) On 2015/05/06 12:46:02, ulan wrote: > index >= OBJECT_STATS_COUNT Done. https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.cc#newcod... src/heap/heap.cc:6449: #define ADJUST_LAST_TIME_OBJECT_COUNT(name) \ On 2015/05/06 12:46:02, ulan wrote: > The name "ADJUST_LAST_TIME_OBJECT_COUNT" is confusing here. Maybe > "COMPARE_AND_RETURN_NAME" or "CASE_NAME" ? Done. https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1113233002/diff/30005/src/heap/heap.h#newcode... src/heap/heap.h:1020: size_t object_count_last_gc(size_t type) { On 2015/05/06 12:46:02, ulan wrote: > s/type/index/ here and below Done.
ulan@chromium.org changed reviewers: + hablich@chromium.org
lgtm if the comment below is addressed and if Michael approves. Michael, how does it look with the branch point? Is it ok to land API change now or should we wait until V8 branches? https://codereview.chromium.org/1113233002/diff/50001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1113233002/diff/50001/include/v8.h#newcode5222 include/v8.h:5222: size_t NumberOfTrackedGCObjectsTypes(); Let's name it NumberOfTrackedHeapObjectTypes();
https://codereview.chromium.org/1113233002/diff/70001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1113233002/diff/70001/include/v8.h#newcode4821 include/v8.h:4821: const char* object_type() { return object_type_; } I wonder here if the right thing to do is concatenating types with slashes (e.g. "CODE/CODE_AGE/IsExecuted"), or if we should expose them separately at an API level, something like const char* object_type(); const char* object_subtype(); // This will be "" for most of the entries, % code_age, kind and fixed_array. Ulan, WDYT?
On 2015/05/07 14:40:13, ulan wrote: > lgtm if the comment below is addressed and if Michael approves. > > Michael, how does it look with the branch point? Is it ok to land API change now > or should we wait until V8 branches? > > https://codereview.chromium.org/1113233002/diff/50001/include/v8.h > File include/v8.h (right): > > https://codereview.chromium.org/1113233002/diff/50001/include/v8.h#newcode5222 > include/v8.h:5222: size_t NumberOfTrackedGCObjectsTypes(); > Let's name it NumberOfTrackedHeapObjectTypes(); As far as I can see this only adds new stuff to the API. So a potential revert from our POV should be simple. The thing is, when are the consumers of the API going to use the new functionality? If they are start using the new functionality after we branch, there shouldn't be a problem. Is this correct?
On 2015/05/07 19:59:07, Hablich wrote: > On 2015/05/07 14:40:13, ulan wrote: > > lgtm if the comment below is addressed and if Michael approves. ulan@ I have changed the API to return instance type and instance sub type as different strings here. The previous version works for the case, but to change the structure in the display it gets difficult. For instance if we want to display code types and object types as different lists, it would require API changes. Also, it looks bad for the api to know how the consumer is going to parse the string. PTAL at the change, Thanks. > > > > Michael, how does it look with the branch point? Is it ok to land API change > now > > or should we wait until V8 branches? > > > > https://codereview.chromium.org/1113233002/diff/50001/include/v8.h > > File include/v8.h (right): > > > > https://codereview.chromium.org/1113233002/diff/50001/include/v8.h#newcode5222 > > include/v8.h:5222: size_t NumberOfTrackedGCObjectsTypes(); > > Let's name it NumberOfTrackedHeapObjectTypes(); > > As far as I can see this only adds new stuff to the API. So a potential revert > from our POV should be simple. The thing is, when are the consumers of the API > going to use the new functionality? If they are start using the new > functionality after we branch, there shouldn't be a problem. Is this correct? Can you tell me when is it possible to start using the API? I will be using the API in my next CL, by next week maybe.
> The thing is, when are the consumers of the API going to use the new functionality? As soon as they roll on master... unless there are specific concerns and you prefer to wait for the branch point (out of curiosity, why?)
On 2015/05/08 11:41:19, Primiano Tucci wrote: > > The thing is, when are the consumers of the API going to use the new > functionality? > As soon as they roll on master... unless there are specific concerns and you > prefer to wait for the branch point (out of curiosity, why?) Sure thing: The V8 M44 branch will be created before noon CEST on the 2015-05-13. On this date one of the latest Canaries will be selected to be the branch point. This means we will roll this version into Chromium in order to give them and Blink time to synchronize with the roll. Essentially this will be a revert to an older version! In order to make this synchronization as painless as possible for V8 and Blink please don't make changes which make the revert more difficult. If you still need to do it, please tell me, so I can make sure that we are dealing with that change properly on branch day.
> ulan@ I have changed the API to return instance type and instance sub type as different strings here. OK, I left two comments in code. Otherwise, looks good. https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc#newco... src/heap/heap.cc:6520: *object_type = "CODE_KIND"; \ object_type should be "CODE_TYPE" object_sub_type should be "CODE_KIND" and #name https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc#newco... src/heap/heap.cc:6535: *object_sub_type = #name; \ object_type should be "CODE_TYPE" object_sub_type should be "CODE_AGE" and #name
On 2015/05/11 08:06:52, ulan wrote: > > ulan@ I have changed the API to return instance type and instance sub type as > different strings here. > OK, I left two comments in code. Otherwise, looks good. > > https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc > File src/heap/heap.cc (right): > > https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc#newco... > src/heap/heap.cc:6520: *object_type = "CODE_KIND"; \ > object_type should be "CODE_TYPE" > object_sub_type should be "CODE_KIND" and #name > > https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc#newco... > src/heap/heap.cc:6535: *object_sub_type = #name; > \ > object_type should be "CODE_TYPE" > object_sub_type should be "CODE_AGE" and #name ... and please wait until M44 branch creation to land this.
Thanks made changes. https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc#newco... src/heap/heap.cc:6520: *object_type = "CODE_KIND"; \ On 2015/05/11 08:06:52, ulan wrote: > object_type should be "CODE_TYPE" > object_sub_type should be "CODE_KIND" and #name Done. https://codereview.chromium.org/1113233002/diff/110001/src/heap/heap.cc#newco... src/heap/heap.cc:6535: *object_sub_type = #name; \ On 2015/05/11 08:06:52, ulan wrote: > object_type should be "CODE_TYPE" > object_sub_type should be "CODE_AGE" and #name Done.
LGTM. Ulan, are you fine with the new dump type/subtype pattern? If so, can this get landed? Thanks!
LGTM https://codereview.chromium.org/1113233002/diff/130001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1113233002/diff/130001/include/v8.h#newcode5234 include/v8.h:5234: * which ranges from 0 to NumberOfHeapObjectsTypes() - 1. from 0 to NumberOfTrackedHeapObjectTypes() - 1
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, ulan@chromium.org Link to the patchset: https://codereview.chromium.org/1113233002/#ps150001 (title: "Fixing the comment with NumberOfTrackedHeapObjectTypes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1113233002/150001
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/66083dd05ff812f3402758340bfab18e4f75fa50 Cr-Commit-Position: refs/heads/master@{#28474} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
