Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1058253003:
Adding V8 api to get memory statistics of spaces in V8::Heap. (Closed)
|
Created:
5 years, 8 months ago by ssid Modified:
5 years, 8 months ago Reviewers:
Primiano Tucci (use gerrit), Hannes Payer (out of office), jochen (gone - plz use gerrit), picksi1, rmcilroy Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAdding V8 api to get memory statistics of spaces in V8::Heap.
This is first step towards adding V8 heap statistics to the memory
tracing infrastructure. For being able to get useful memory number into
the memory dump, v8 needs to provide an external api needs to obtain
more information about the heap. So, this Cl extends the api to give
information about the memory allocated and used in the spaces.
BUG=466141, 476013
LOG=Y
Committed: https://crrev.com/281d30d758dc9060045ada1b3d326bfe3ef668dd
Cr-Commit-Position: refs/heads/master@{#27919}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Made changes. #
Total comments: 10
Patch Set 3 : Made changes. #Patch Set 4 : Added physical_size. #Patch Set 5 : Added pimpl class for using vector. #
Total comments: 1
Patch Set 6 : Adding new APIs for space statistics. #
Total comments: 16
Patch Set 7 : Fixing comments. #Patch Set 8 : Added base class functions. #Patch Set 9 : Fixing build. #
Total comments: 13
Patch Set 10 : Fixed comments. #
Total comments: 5
Patch Set 11 : Made changes. #
Total comments: 2
Patch Set 12 : Made changes. #
Total comments: 2
Patch Set 13 : Made changes. #Patch Set 14 : Fixing jochen's comments. #
Messages
Total messages: 45 (14 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
ssid@chromium.org changed reviewers: + rmcilroy@chromium.org
+rmcilroy@
https://codereview.chromium.org/1058253003/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/1/include/v8.h#newcode4896 include/v8.h:4896: SpaceStatistics lo_space_stats_; Maybe we want to make this a bit more opaque to avoid hard-coding the name / number of spaces. How about an array of SpaceStatistics which are named and the heap just fills in those that it knows about? https://codereview.chromium.org/1058253003/diff/1/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/1/src/api.cc#newcode6961 src/api.cc:6961: class HeapStatisticsHelper { I don't think we should have a seperate class just for this, and in any case it probably shouldn't be in api.cc. Could you move the filling-in of these stats to heap.cc (I'm not sure about how the public / internal dependencies would look though)?
Made changes, PTAL
ssid@chromium.org changed reviewers: + picksi@chromium.org
https://codereview.chromium.org/1058253003/diff/20001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/20001/include/v8.h#newcode21 include/v8.h:21: #include <vector> We probably don't want to pull in <vector> to be a requirement of V8's public API. Maybe just forward declare std::vector in the header, and only store a pointer to the vector in the HeapStatistics object (so that it only needs the forward declaration). https://codereview.chromium.org/1058253003/diff/20001/include/v8.h#newcode4860 include/v8.h:4860: SpaceStatistics(size_t size_of_objects, size_t committed_memory, Add a name field so we know what each space each SpaceStatitics represents. https://codereview.chromium.org/1058253003/diff/20001/include/v8.h#newcode4860 include/v8.h:4860: SpaceStatistics(size_t size_of_objects, size_t committed_memory, nit - each arg on a newline https://codereview.chromium.org/1058253003/diff/20001/include/v8.h#newcode4867 include/v8.h:4867: size_t available_memory_; I think we should make these more obvious names if they are part of the external API. Also, let's make this consistent with the total amounts below so: - CommittedMemory() -> space_size - SizeOfObjects() -> space_used_size - Available() -> space_available_size also lets add CommittedPhysicalMemory() -> physical_space_size to be consistent. https://codereview.chromium.org/1058253003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/20001/src/api.cc#newcode6978 src/api.cc:6978: heap->paged_space(space)->Size(), This should be SizeOfObjects() https://codereview.chromium.org/1058253003/diff/20001/src/api.cc#newcode6983 src/api.cc:6983: HeapStatistics::SpaceStatistics lo_space_stats( nit - newline above
Patchset #3 (id:40001) has been deleted
Thanks, It works passing pointer of vector. PTAL. https://codereview.chromium.org/1058253003/diff/20001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/20001/include/v8.h#newcode4860 include/v8.h:4860: SpaceStatistics(size_t size_of_objects, size_t committed_memory, On 2015/04/13 17:40:08, rmcilroy wrote: > nit - each arg on a newline Done. https://codereview.chromium.org/1058253003/diff/20001/include/v8.h#newcode4867 include/v8.h:4867: size_t available_memory_; On 2015/04/13 17:40:07, rmcilroy wrote: > I think we should make these more obvious names if they are part of the external > API. Also, let's make this consistent with the total amounts below so: > - CommittedMemory() -> space_size > - SizeOfObjects() -> space_used_size > - Available() -> space_available_size > > also lets add CommittedPhysicalMemory() -> physical_space_size to be consistent. Done. https://codereview.chromium.org/1058253003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/20001/src/api.cc#newcode6978 src/api.cc:6978: heap->paged_space(space)->Size(), On 2015/04/13 17:40:08, rmcilroy wrote: > This should be SizeOfObjects() Done. https://codereview.chromium.org/1058253003/diff/20001/src/api.cc#newcode6983 src/api.cc:6983: HeapStatistics::SpaceStatistics lo_space_stats( On 2015/04/13 17:40:08, rmcilroy wrote: > nit - newline above Done.
https://codereview.chromium.org/1058253003/diff/100001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/100001/src/api.cc#newcode5478 src/api.cc:5478: HeapStatistics::~HeapStatistics() { free(spaces_statistics_); } You are using 'new' and 'free' which is wrong - you need to use 'delete' if you are using 'new'. This is what asan is complaining about.
Hmm I am not a V8 owner but this still feels a bit hairy, how about not exposing
any vector, nor doing any dynamic memory allocation and keeping it simple (TM)
as follows:
Proposed change:
class Isolate:
// This is the existing API, which is not related to any particualr space,
and we leave it untouched.
void GetHeapStatistics(HeapStatistics* heap_statistics);
// Instead we add two new methods:
size_t GetNumberOfHeapSpaces()
void GetHeapSpaceStatistics(HeapSpaceStatistics* heap_space_statistics, size_t
space_index)
So, from caller viewpoint this will look as follows:
HeapSpaceStatistics hs;
const size_t num_spaces = isolate->GetNumberOfSHeapSpaces();
for (int i = 0; i < num_spaces; ++i) {
isolate->GetHeapSpaceStatistics(&hs, i);
printf("Statistics for the heap %s: %d....", hs->name(),
hs->allocated_bla())
}
....
Ross, WDYT?
On 2015/04/14 17:35:32, Primiano Tucci wrote:
> Hmm I am not a V8 owner but this still feels a bit hairy, how about not
exposing
> any vector, nor doing any dynamic memory allocation and keeping it simple (TM)
> as follows:
>
> Proposed change:
> class Isolate:
> // This is the existing API, which is not related to any particualr space,
> and we leave it untouched.
> void GetHeapStatistics(HeapStatistics* heap_statistics);
>
> // Instead we add two new methods:
> size_t GetNumberOfHeapSpaces()
> void GetHeapSpaceStatistics(HeapSpaceStatistics* heap_space_statistics,
size_t
> space_index)
>
>
> So, from caller viewpoint this will look as follows:
> HeapSpaceStatistics hs;
> const size_t num_spaces = isolate->GetNumberOfSHeapSpaces();
> for (int i = 0; i < num_spaces; ++i) {
> isolate->GetHeapSpaceStatistics(&hs, i);
> printf("Statistics for the heap %s: %d....", hs->name(),
> hs->allocated_bla())
> }
> ....
>
> Ross, WDYT?
I haven't looked too closely at the latest patchset yet (was just trying to hekp
ssid with the asan bug he mentioned before i left), but yes, I would prefer
something like this that doesn't involve allocation in the API.
Patchset #6 (id:120001) has been deleted
Added new APIs, I think this looks better.
Looks much cleaner this way, thanks. https://codereview.chromium.org/1058253003/diff/140001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/140001/include/v8.h#newcode5292 include/v8.h:5292: void GetHeapSpaceStatistics(HeapSpaceStatistics* space_statistics, int index); return a bool to inform caller of success or failure (e.g., space_statistics being NULL or index out of range). https://codereview.chromium.org/1058253003/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7009 src/api.cc:7009: int Isolate::NumberOfHeapSpaces() { return i::LAST_SPACE - i::FIRST_SPACE + 1; } nit - newline for body https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7015 src/api.cc:7015: i::Heap* heap = isolate->heap(); check for space_statistics being null. https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7015 src/api.cc:7015: i::Heap* heap = isolate->heap(); check for index out-of-bounds https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7019 src/api.cc:7019: space_statistics->space_used_size_ = heap->new_space()->Size(); SizeOfObjects https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7036 src/api.cc:7036: space_statistics->space_avalable_size_ = heap->lo_space()->Available(); Does the Spaces superclass have these methods defined? If so you could pull out the appropriate "Space" object and just do the code to fill in the space_statistics object once. If the methods aren't there on the superclass it might be worth doing this if all subclasses of the Space define the methods anyway. https://codereview.chromium.org/1058253003/diff/140001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1058253003/diff/140001/src/heap/heap.cc#newco... src/heap/heap.cc:473: return kLoSpaceName; add 'default' case with UNREACHABLE(); to ensure we always hit a valid index (and remove the default nullptr return below)
Replies inline. https://codereview.chromium.org/1058253003/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7019 src/api.cc:7019: space_statistics->space_used_size_ = heap->new_space()->Size(); On 2015/04/15 10:21:42, rmcilroy wrote: > SizeOfObjects PrintShortHeapStatistics() uses Size() and not SizeOfObjects() https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/heap.c... https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7036 src/api.cc:7036: space_statistics->space_avalable_size_ = heap->lo_space()->Available(); On 2015/04/15 10:21:42, rmcilroy wrote: > Does the Spaces superclass have these methods defined? If so you could pull out > the appropriate "Space" object and just do the code to fill in the > space_statistics object once. If the methods aren't there on the superclass it > might be worth doing this if all subclasses of the Space define the methods > anyway. The only line i can pull out of these three cases is GetSpaceName(x) Other all functions are different for the three. These are defined in the classes : PagedSpace, LargeObjectSpace and NewSpace.
Thanks. Made changes. PTAL https://codereview.chromium.org/1058253003/diff/140001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/140001/include/v8.h#newcode5292 include/v8.h:5292: void GetHeapSpaceStatistics(HeapSpaceStatistics* space_statistics, int index); On 2015/04/15 10:21:42, rmcilroy wrote: > return a bool to inform caller of success or failure (e.g., space_statistics > being NULL or index out of range). Done. https://codereview.chromium.org/1058253003/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7009 src/api.cc:7009: int Isolate::NumberOfHeapSpaces() { return i::LAST_SPACE - i::FIRST_SPACE + 1; } On 2015/04/15 10:21:42, rmcilroy wrote: > nit - newline for body Done. https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7015 src/api.cc:7015: i::Heap* heap = isolate->heap(); On 2015/04/15 10:21:42, rmcilroy wrote: > check for index out-of-bounds Done. https://codereview.chromium.org/1058253003/diff/140001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1058253003/diff/140001/src/heap/heap.cc#newco... src/heap/heap.cc:473: return kLoSpaceName; On 2015/04/15 10:21:42, rmcilroy wrote: > add 'default' case with UNREACHABLE(); to ensure we always hit a valid index > (and remove the default nullptr return below) Added UNREACHABLE and keeping return null since compiler gives error if i dont return at the end.
rmcilroy@chromium.org changed reviewers: + hpayer@chromium.org
https://codereview.chromium.org/1058253003/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7019 src/api.cc:7019: space_statistics->space_used_size_ = heap->new_space()->Size(); On 2015/04/15 10:29:20, ssid wrote: > On 2015/04/15 10:21:42, rmcilroy wrote: > > SizeOfObjects > > PrintShortHeapStatistics() uses Size() and not SizeOfObjects() > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/heap.c... SizeOfObjects() calls Size() for the newspace, so let's keep them consistent. https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7036 src/api.cc:7036: space_statistics->space_avalable_size_ = heap->lo_space()->Available(); On 2015/04/15 10:29:20, ssid wrote: > On 2015/04/15 10:21:42, rmcilroy wrote: > > Does the Spaces superclass have these methods defined? If so you could pull > out > > the appropriate "Space" object and just do the code to fill in the > > space_statistics object once. If the methods aren't there on the superclass it > > might be worth doing this if all subclasses of the Space define the methods > > anyway. > > The only line i can pull out of these three cases is GetSpaceName(x) > Other all functions are different for the three. These are defined in the > classes : PagedSpace, LargeObjectSpace and NewSpace. +hpayer As mentioned, you could you make these functions virtual abstract in Space and have the implementations in each of the spaces be overrides. Hannes: Are you aware of any reason that these methods are defined in the subclasses and aren't virtual abstract functions in Space which are overridden by the specific subclass? Would it cause any issue to make this change for code clarity?
https://codereview.chromium.org/1058253003/diff/140001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/140001/src/api.cc#newcode7036 src/api.cc:7036: space_statistics->space_avalable_size_ = heap->lo_space()->Available(); On 2015/04/15 12:17:40, rmcilroy wrote: > On 2015/04/15 10:29:20, ssid wrote: > > On 2015/04/15 10:21:42, rmcilroy wrote: > > > Does the Spaces superclass have these methods defined? If so you could pull > > out > > > the appropriate "Space" object and just do the code to fill in the > > > space_statistics object once. If the methods aren't there on the superclass > it > > > might be worth doing this if all subclasses of the Space define the methods > > > anyway. > > > > The only line i can pull out of these three cases is GetSpaceName(x) > > Other all functions are different for the three. These are defined in the > > classes : PagedSpace, LargeObjectSpace and NewSpace. > > +hpayer > > As mentioned, you could you make these functions virtual abstract in Space and > have the implementations in each of the spaces be overrides. > > Hannes: Are you aware of any reason that these methods are defined in the > subclasses and aren't virtual abstract functions in Space which are overridden > by the specific subclass? Would it cause any issue to make this change for code > clarity? Hmm, should be possible. I cannot think of any reason why that wouldn't work. Would definitely cleaner in that case.
Patchset #8 (id:180001) has been deleted
Thanks. Made changes. PTAL.
Looks pretty good. Once the nits below are addressed and the compile errors are fixed we can send this to the V8 API owners for a look. https://codereview.chromium.org/1058253003/diff/220001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/220001/include/v8.h#newcode4910 include/v8.h:4910: size_t space_avalable_size() { return space_avalable_size_; } Fix spelling typo (avalable -> available) throughout CL. https://codereview.chromium.org/1058253003/diff/220001/include/v8.h#newcode5291 include/v8.h:5291: * Get the memory usage of a space in the heap. nit - add 'Returns true on success'. https://codereview.chromium.org/1058253003/diff/220001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/220001/src/api.cc#newcode7019 src/api.cc:7019: if (index > i::LAST_SPACE || index < i::FIRST_SPACE || !space_statistics) nit - move the '!space_statistics' check out to a seperate if block (just for code clarity). https://codereview.chromium.org/1058253003/diff/220001/src/api.cc#newcode7023 src/api.cc:7023: space_statistics->space_size_ = heap->space(index)->CommittedMemory(); nit - factor out: Space space = heap->space(index); and use the 'space' local variable for these lookups to avoid having to call space(index) multiple times. https://codereview.chromium.org/1058253003/diff/220001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1058253003/diff/220001/src/heap/spaces.h#newc... src/heap/spaces.h:1741: virtual intptr_t Size() override { return accounting_stats_.Size(); } Only one of 'virtual' or 'override' [1], so in this case remove the virtuals where you are adding override. [1] https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance https://codereview.chromium.org/1058253003/diff/220001/src/heap/spaces.h#newc... src/heap/spaces.h:2177: virtual void Print() override; I don't think you intended to change this.
Looks pretty good. Once the nits below are addressed and the compile errors are fixed we can send this to the V8 API owners for a look.
Made changes. PTAL. https://codereview.chromium.org/1058253003/diff/220001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/220001/include/v8.h#newcode4910 include/v8.h:4910: size_t space_avalable_size() { return space_avalable_size_; } On 2015/04/16 13:49:05, rmcilroy wrote: > Fix spelling typo (avalable -> available) throughout CL. Done. https://codereview.chromium.org/1058253003/diff/220001/include/v8.h#newcode5291 include/v8.h:5291: * Get the memory usage of a space in the heap. On 2015/04/16 13:49:05, rmcilroy wrote: > nit - add 'Returns true on success'. Done. https://codereview.chromium.org/1058253003/diff/220001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/220001/src/api.cc#newcode7019 src/api.cc:7019: if (index > i::LAST_SPACE || index < i::FIRST_SPACE || !space_statistics) On 2015/04/16 13:49:05, rmcilroy wrote: > nit - move the '!space_statistics' check out to a seperate if block (just for > code clarity). Done. https://codereview.chromium.org/1058253003/diff/220001/src/api.cc#newcode7023 src/api.cc:7023: space_statistics->space_size_ = heap->space(index)->CommittedMemory(); On 2015/04/16 13:49:05, rmcilroy wrote: > nit - factor out: > Space space = heap->space(index); > and use the 'space' local variable for these lookups to avoid having to call > space(index) multiple times. Done. https://codereview.chromium.org/1058253003/diff/220001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1058253003/diff/220001/src/heap/spaces.h#newc... src/heap/spaces.h:1741: virtual intptr_t Size() override { return accounting_stats_.Size(); } On 2015/04/16 13:49:05, rmcilroy wrote: > Only one of 'virtual' or 'override' [1], so in this case remove the virtuals > where you are adding override. > > [1] https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Thanks, done. https://codereview.chromium.org/1058253003/diff/220001/src/heap/spaces.h#newc... src/heap/spaces.h:2177: virtual void Print() override; On 2015/04/16 13:49:05, rmcilroy wrote: > I don't think you intended to change this. This was needed to maintain consistency in the file, (build error) Note: If the C++ version is not 11, then this function will not have override declared, and virtual is removed.
Looks good with a couple more things. Let me know when you've moved this over to your V8 repo. https://codereview.chromium.org/1058253003/diff/220001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/220001/include/v8.h#newcode5291 include/v8.h:5291: * Get the memory usage of a space in the heap. On 2015/04/16 16:40:29, ssid wrote: > On 2015/04/16 13:49:05, rmcilroy wrote: > > nit - add 'Returns true on success'. > > Done. You seem to have missed this. https://codereview.chromium.org/1058253003/diff/240001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/240001/include/v8.h#newcode5293 include/v8.h:5293: * \param space_statistics the HeapSpaceStatistics object to fill in capitalize "The" https://codereview.chromium.org/1058253003/diff/240001/include/v8.h#newcode5295 include/v8.h:5295: * \param index ranges from 0 - number of spaces in the heap (given by "\param index The index of the space to get statistics from, which ranges from 0 to NumberOfHeapSpaces() - 1." https://codereview.chromium.org/1058253003/diff/240001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1058253003/diff/240001/src/api.cc#newcode7026 src/api.cc:7026: i::Space* space = heap->space(index); super nit - move up on line (just to keep all filling in of space_statistics on adjacent lines.
Done. https://codereview.chromium.org/1058253003/diff/240001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/240001/include/v8.h#newcode5293 include/v8.h:5293: * \param space_statistics the HeapSpaceStatistics object to fill in On 2015/04/16 16:53:36, rmcilroy wrote: > capitalize "The" Done. https://codereview.chromium.org/1058253003/diff/240001/include/v8.h#newcode5295 include/v8.h:5295: * \param index ranges from 0 - number of spaces in the heap (given by On 2015/04/16 16:53:36, rmcilroy wrote: > "\param index The index of the space to get statistics from, which ranges from 0 > to NumberOfHeapSpaces() - 1." Done.
https://codereview.chromium.org/1058253003/diff/260001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1058253003/diff/260001/src/heap/heap.cc#newco... src/heap/heap.cc:58: const char* kNewSpaceName = "new_space"; The constants are only used in GetSpaceName(int idx), why don't we just return the string in that function and get rid of the constants?
Danno: Could you give this an API OWNER's review? Thanks. https://codereview.chromium.org/1058253003/diff/260001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1058253003/diff/260001/src/heap/heap.cc#newco... src/heap/heap.cc:58: const char* kNewSpaceName = "new_space"; On 2015/04/17 09:07:25, Hannes Payer wrote: > The constants are only used in GetSpaceName(int idx), why don't we just return > the string in that function and get rid of the constants? +1
rmcilroy@chromium.org changed reviewers: + danno@chromium.org
Actually add danno@ this time... Danno: Could you give this an API OWNER's review? Thanks.
rmcilroy@chromium.org changed reviewers: + jochen@chromium.org - danno@chromium.org
-danno, +jochen (on danno's request) Jochen, could you give this an API OWNER review please.
lgtm with nits https://codereview.chromium.org/1058253003/diff/280001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1058253003/diff/280001/include/v8.h#newcode5288 include/v8.h:5288: int NumberOfHeapSpaces(); size_t https://codereview.chromium.org/1058253003/diff/280001/include/v8.h#newcode5299 include/v8.h:5299: bool GetHeapSpaceStatistics(HeapSpaceStatistics* space_statistics, int index); index should be size_t
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1058253003/#ps300001 (title: "Made changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1058253003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/3941)
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1058253003/#ps320001 (title: "Fixing jochen's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1058253003/320001
Message was sent while issue was closed.
Committed patchset #14 (id:320001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/281d30d758dc9060045ada1b3d326bfe3ef668dd Cr-Commit-Position: refs/heads/master@{#27919} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
