Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1375963007:
[tracing] Display the resident size of the discardable memory segments (Closed)
|
Created:
5 years, 2 months ago by ssid Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lock_discardable Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Display the resident size of the discardable memory segments
This CL displays the total size of the resident pages of the
discardable memory segments. The resident page is counted using mincore
system call in posix systems. It is not implemented in other platforms.
TBR=avi@chromium.org
BUG=528632
Committed: https://crrev.com/a3fc37f99203bfecf0b701aa7aaf4d985e289117
Cr-Commit-Position: refs/heads/master@{#353546}
Patch Set 1 #Patch Set 2 : Fix mac. #
Total comments: 6
Patch Set 3 : Fixes. #
Total comments: 1
Patch Set 4 : Change ssize_t to int64 #Patch Set 5 : Using suppotrted macro. #
Total comments: 20
Patch Set 6 : Refactoring into same function. #Patch Set 7 : Move the attribute to global dump. #
Total comments: 10
Patch Set 8 : Fixes. #
Total comments: 2
Patch Set 9 : memset. #Patch Set 10 : Nit. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 42 (14 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
I have not tested it on mac yet. But it works on linux and android and is available on mac. I will test once I find time, else I can ifdef out mac too. PTAL. Thanks
Makes sense to me, but we need to fix a bit the logic of the mincore loop to avoid using too much memory when dealing with large virtual address spaces https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:171: const size_t page_size = base::GetPageSize(); I think we need to make this a bit smarter and operate in chunks. THe main problem I see is that if you ask to CountResidentBytes(...,1GB) this will end up using 1GB/4096 = 256 M of memory just to do the count (as it uses 1 byte per page). I think what you want to do here is looping in chunks of ~32M (which has a peak use of 8k for the |vec|) https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:175: unsigned char vec[page_count + 1]; From [1]. "We do not allow variable-length arrays or alloca()." [1] https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable-Len... https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... base/trace_event/process_memory_dump.h:127: static ssize_t CountResidentBytes(void* start_address, size_t mapped_size); IIRC static methods should be declared before non statics. Move this up.
Patchset #3 (id:40001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #3 (id:60001) has been deleted
Made changes, PTAL thanks https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:171: const size_t page_size = base::GetPageSize(); On 2015/10/06 17:14:09, Primiano Tucci wrote: > I think we need to make this a bit smarter and operate in chunks. > THe main problem I see is that if you ask to CountResidentBytes(...,1GB) this > will end up using 1GB/4096 = 256 M of memory just to do the count (as it uses 1 > byte per page). > I think what you want to do here is looping in chunks of ~32M (which has a peak > use of 8k for the |vec|) Done. https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:175: unsigned char vec[page_count + 1]; On 2015/10/06 17:14:09, Primiano Tucci wrote: > From [1]. "We do not allow variable-length arrays or alloca()." > > [1] > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable-Len... Done. https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1375963007/diff/20001/base/trace_event/proces... base/trace_event/process_memory_dump.h:127: static ssize_t CountResidentBytes(void* start_address, size_t mapped_size); On 2015/10/06 17:14:09, Primiano Tucci wrote: > IIRC static methods should be declared before non statics. Move this up. Done.
ssid@chromium.org changed reviewers: + reveman@chromium.org
+reveman for the discardable manager file. PTAL thanks
https://codereview.chromium.org/1375963007/diff/80001/base/trace_event/proces... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1375963007/diff/80001/base/trace_event/proces... base/trace_event/process_memory_dump.h:47: // process. Returns -1 if counting is not implemented in the current platform. Can we have a IsCountResidentBytesSupported() function instead of using a negative return value for when this is not supported on a platform? Or maybe even better, #ifdef the function so it's only compiled and available on platforms where this is supported.
On 2015/10/07 09:40:04, reveman wrote: > https://codereview.chromium.org/1375963007/diff/80001/base/trace_event/proces... > File base/trace_event/process_memory_dump.h (right): > > https://codereview.chromium.org/1375963007/diff/80001/base/trace_event/proces... > base/trace_event/process_memory_dump.h:47: // process. Returns -1 if counting is > not implemented in the current platform. > Can we have a IsCountResidentBytesSupported() function instead of using a > negative return value for when this is not supported on a platform? Or maybe > even better, #ifdef the function so it's only compiled and available on > platforms where this is supported. Hm, primiano did not like the idea of having isSupported method. I don't like the idea of ifdef-ing the function itself since maybe there is a way to do it in windows as well. I have not researched it yet. It also creates problems like the clients of this method will have to ifdef while using it, creating multiple ifdefs all over. If returning -1 is an issue, maybe I can change the method to bool CountResident(void* address, size_t size, size_t* value) and value takes the output, and function returns success.
On 2015/10/07 at 09:48:05, ssid wrote: > On 2015/10/07 09:40:04, reveman wrote: > > https://codereview.chromium.org/1375963007/diff/80001/base/trace_event/proces... > > File base/trace_event/process_memory_dump.h (right): > > > > https://codereview.chromium.org/1375963007/diff/80001/base/trace_event/proces... > > base/trace_event/process_memory_dump.h:47: // process. Returns -1 if counting is > > not implemented in the current platform. > > Can we have a IsCountResidentBytesSupported() function instead of using a > > negative return value for when this is not supported on a platform? Or maybe > > even better, #ifdef the function so it's only compiled and available on > > platforms where this is supported. > > Hm, primiano did not like the idea of having isSupported method. I don't like the idea of ifdef-ing the function itself since maybe there is a way to do it in windows as well. I have not researched it yet. It also creates problems like the clients of this method will have to ifdef while using it, creating multiple ifdefs all over. I think ifdefs are preferred in base/. You'll find many cases where we do something similar. In general, compile time logic is always preferred over run-time logic. You can do something similar to what I did for DISCARDABLE_SHARED_MEMORY_SHRINKING if the #ifdef needed to know if this is supported is not trivial: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/discar... > If returning -1 is an issue, maybe I can change the method to bool CountResident(void* address, size_t size, size_t* value) and value takes the output, and function returns success.
+1 for ifdef if that can be known purely at compile time.
On 2015/10/07 10:13:03, Primiano Tucci wrote: > +1 for ifdef if that can be known purely at compile time. BTW I just tried this CL and this seems to show the resident size only for discardable in the browser process, not in the renderer. Is this WAI?
On 2015/10/07 10:29:36, Primiano Tucci wrote: > On 2015/10/07 10:13:03, Primiano Tucci wrote: > > +1 for ifdef if that can be known purely at compile time. > > BTW I just tried this CL and this seems to show the resident size only for > discardable in the browser process, not in the renderer. > Is this WAI? Also, before landing this I'd like to have a closer look to the data. I looked at the trace and the value that are reported here as resident seem to not match what I see in /proc/pid/smaps.
Also, how long do these mincore calls take? Should this happen only on heavy dumps?
Patchset #6 (id:160001) has been deleted
Patchset #5 (id:140001) has been deleted
With offline discussion, the resident size cannot be displayed in sandboxed processed since secomp-bpf does not allow the sys call. This does not happen on android yet, but will cause problems in future when it is enabled. This means that only browse process can get the resident size information. This is ok because the browser knows about all the memory segments allocated across all processes. Also, mincore displays the resident memory even if it is touched by other processes. The trace viewer can be modified to show the resident_size in renderer also, if possible. I have added a new define which says if the resident size counting is supported. The maximum time taken for calls on normal segment is 0.3ms. But when there are large images of 300mb, it goes up to 5ms. So, adding resident size only on heavy dumps. Thanks, sid.
lgtm with nits https://codereview.chromium.org/1375963007/diff/180001/content/common/host_di... File content/common/host_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/1375963007/diff/180001/content/common/host_di... content/common/host_discardable_shared_memory_manager.cc:191: int64 resident_size = nit: size_t as that's now the return type for CountResidentBytes https://codereview.chromium.org/1375963007/diff/180001/content/common/host_di... content/common/host_discardable_shared_memory_manager.cc:194: if (resident_size >= 0) { nit: s/resident_size >= 0/resident_size/ after switching to size_t above
https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:25: #if defined(COUNT_RESIDENT_BYTES_SUPPORTED) I'd probably just put them in one function. This is not called by anything else. Having only one function and one ifdef guard would make it easier to move in future. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:56: size_t ProcessMemoryDump::CountResidentBytes(void* start_address, you have to decide here if you want to accept only page-aligned ranges (in which case add a DCHECK) or arbitrary ranges and do the slightly more complicated math to deal with partial pages (you have to approximate them, if a partial page is resident, you can only assume that all its partial content is resident) The latter would be probably a bit more useful, but not stricty needed today. SO you might just go with aligned, but add checks and add a comment. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:59: // kPageChunkSize / pageSize. add a small comment explaining why you are splitting in chunks (memory usage, one byte per page, yada yada) https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:64: total_resident_size += GetResidentSizeOfSegment( this would be a bit clearer if you add an extra variable: const size_t chunk_size = std::min(kMaxChunkSize, mapped_size - offset) https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:65: static_cast<char*>(start_address) + offset, I think this should be reinterpret_cast Not clear to me why you take a void* here, take a char* up in GetResidentSizeOfSegmen and use uintptr_t to do the math (which is right). Again, not a problem if you move everything in this function. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.h:51: // Returns the total bytes resident for a memory segment, with given s/a memory segment/a virtual address range/ https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:162: char memory1[100] = {0}; if you decide to support only page aligned ranges, lines 162-166 don't have any sense anymore. If you decide to support arbitrary ranges, you have to make a smarter check (like ranges [2048, 12288] where there is a non resident hole in the middle) https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:183: ASSERT_GE(res3, kVeryLargeMemorySize); ?? How does this pass? Nothing is making those pages ditry, hence resident? If this passes it does by accident.
Patchset #6 (id:200001) has been deleted
Made changes, Thanks, see replies inline. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:25: #if defined(COUNT_RESIDENT_BYTES_SUPPORTED) On 2015/10/07 13:59:51, Primiano Tucci wrote: > I'd probably just put them in one function. This is not called by anything else. > Having only one function and one ifdef guard would make it easier to move in > future. Done. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:56: size_t ProcessMemoryDump::CountResidentBytes(void* start_address, On 2015/10/07 13:59:51, Primiano Tucci wrote: > you have to decide here if you want to accept only page-aligned ranges (in which > case add a DCHECK) or arbitrary ranges and do the slightly more complicated math > to deal with partial pages (you have to approximate them, if a partial page is > resident, you can only assume that all its partial content is resident) > > The latter would be probably a bit more useful, but not stricty needed today. SO > you might just go with aligned, but add checks and add a comment. Yeah, I did not want to add more complications to this. That is why I avoided implementing non-aligned page calculations. If required in future then it can be added. I already added a dcheck for this reason in the other function, moved it here now. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:59: // kPageChunkSize / pageSize. On 2015/10/07 13:59:51, Primiano Tucci wrote: > add a small comment explaining why you are splitting in chunks (memory usage, > one byte per page, yada yada) Done. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:64: total_resident_size += GetResidentSizeOfSegment( On 2015/10/07 13:59:51, Primiano Tucci wrote: > this would be a bit clearer if you add an extra variable: > const size_t chunk_size = std::min(kMaxChunkSize, mapped_size - offset) Done. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:65: static_cast<char*>(start_address) + offset, On 2015/10/07 13:59:51, Primiano Tucci wrote: > I think this should be reinterpret_cast > Not clear to me why you take a void* here, take a char* up in > GetResidentSizeOfSegmen and use uintptr_t to do the math (which is right). > Again, not a problem if you move everything in this function. I had to do reinterpret_cast twice ot get back a void*. Thought casting char* is fine since it still is one byte and i can do the math. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump.h:51: // Returns the total bytes resident for a memory segment, with given On 2015/10/07 13:59:51, Primiano Tucci wrote: > s/a memory segment/a virtual address range/ Done. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:162: char memory1[100] = {0}; On 2015/10/07 13:59:51, Primiano Tucci wrote: > if you decide to support only page aligned ranges, lines 162-166 don't have any > sense anymore. > If you decide to support arbitrary ranges, you have to make a smarter check > (like ranges [2048, 12288] where there is a non resident hole in the middle) Hm, I added this check only to verify if it shows resident for stack pages. The memory allocation is not really needed. https://codereview.chromium.org/1375963007/diff/180001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:183: ASSERT_GE(res3, kVeryLargeMemorySize); On 2015/10/07 13:59:51, Primiano Tucci wrote: > ?? How does this pass? Nothing is making those pages ditry, hence resident? > If this passes it does by accident. new char[kVeryLargeMemorySize]() has an extra () at the end which means that the pages are initialized with 0. That is why the pages are dirty. https://codereview.chromium.org/1375963007/diff/180001/content/common/host_di... File content/common/host_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/1375963007/diff/180001/content/common/host_di... content/common/host_discardable_shared_memory_manager.cc:191: int64 resident_size = On 2015/10/07 13:41:44, reveman wrote: > nit: size_t as that's now the return type for CountResidentBytes Done. https://codereview.chromium.org/1375963007/diff/180001/content/common/host_di... content/common/host_discardable_shared_memory_manager.cc:194: if (resident_size >= 0) { On 2015/10/07 13:41:44, reveman wrote: > nit: s/resident_size >= 0/resident_size/ after switching to size_t above 0 is still reported. removing if.
I have changed the discardable attribute to be added in global dump instead of segment dump, after the change in trace viewer (https://chromiumcodereview.appspot.com/1395583002/), attributes of global dump will be propagated to both the dumps sharing it.
https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:38: const size_t kMemoryChunkSize = 32 * 1024 * 1024; s/kMemoryChunkSize/kMaxChunkSize/ https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:49: int res = mincore(chunk_start, chunk_size, static_cast<char*>(vec.get())); static_cast<char*>(vec.get(): no need to cast, this should be just: &vec.get()[0] https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:162: char memory1[100] = {0}; This is not page aligned. this should hit your dcheck right? remove lines 161-166 https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:169: scoped_ptr<char[]> memory2(new char[5 * page_size]()); Please use just base::AlignedAlloc which makes this more readable https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:174: ASSERT_GE(res2, 5 * page_size); why not ASSERT_EQ?
Thanks PTAL https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:38: const size_t kMemoryChunkSize = 32 * 1024 * 1024; On 2015/10/08 17:34:36, Primiano Tucci wrote: > s/kMemoryChunkSize/kMaxChunkSize/ Done. https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:49: int res = mincore(chunk_start, chunk_size, static_cast<char*>(vec.get())); On 2015/10/08 17:34:36, Primiano Tucci wrote: > static_cast<char*>(vec.get(): > no need to cast, this should be just: &vec.get()[0] Done. https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:162: char memory1[100] = {0}; On 2015/10/08 17:34:36, Primiano Tucci wrote: > This is not page aligned. this should hit your dcheck right? remove lines > 161-166 That is why I am getting the page start address in the next lines.. I removed this test now. https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:169: scoped_ptr<char[]> memory2(new char[5 * page_size]()); On 2015/10/08 17:34:36, Primiano Tucci wrote: > Please use just base::AlignedAlloc which makes this more readable Done. https://codereview.chromium.org/1375963007/diff/240001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:174: ASSERT_GE(res2, 5 * page_size); On 2015/10/08 17:34:36, Primiano Tucci wrote: > why not ASSERT_EQ? Initially it was not page aligned. So, it can return total resident as more than allocated since i passed page start. made it eq now.
LGTM thanks https://codereview.chromium.org/1375963007/diff/260001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1375963007/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:165: size_t res1 = I think you want to memset these.
https://codereview.chromium.org/1375963007/diff/260001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1375963007/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:165: size_t res1 = On 2015/10/09 09:48:51, Primiano Tucci wrote: > I think you want to memset these. Yess. done
ssid@chromium.org changed reviewers: + avi@chromium.org
TBRing avi@ for tracing related change in discardable manager. Approved by reveman@.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1375963007/#ps300001 (title: "Nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375963007/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375963007/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by ssid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375963007/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375963007/300001
lgtm stampity stamp
On 2015/10/12 16:11:28, ssid wrote: > TBRing avi@ for tracing related change in discardable manager. As a general principle TBR should be just for mechanical refactorings / trivial changes (e.g., adding lines to gyp/gn files). This was a minor and probably non controversial change, but IMHO doesn't strictly fall into the bucket of "refactoring changes that are mechanical or global in nature"*. If you really cannot wait for an OWNER review look for another one or ping them. * See TBR guidelines in http://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Reviewe...
Message was sent while issue was closed.
Committed patchset #10 (id:300001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a3fc37f99203bfecf0b701aa7aaf4d985e289117 Cr-Commit-Position: refs/heads/master@{#353546}
Message was sent while issue was closed.
Patchset #11 (id:320001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
