Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15082004:
Adds chrome.memoryBenchmarking.heapProfilerDump for the browser process. (Closed)
|
Created:
7 years, 7 months ago by bulach Modified:
7 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, chromium-apps-reviews_chromium.org, Aaron Boodman Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdds chrome.memoryBenchmarking.heapProfilerDump for the browser process.
This function will be used in conjunction with --enable-memory-benchmarking
switch in order to obtain TCMalloc's heap dumps.
BUG=239836
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202087
Patch Set 1 : Patch #
Total comments: 11
Patch Set 2 : piman's comments #Patch Set 3 : palmer's comments #
Total comments: 3
Patch Set 4 : Removes return value #
Total comments: 17
Patch Set 5 : Comments #Patch Set 6 : Merged after dai's revert. #
Total comments: 4
Patch Set 7 : nit from piman #Patch Set 8 : Rebased #Messages
Total messages: 33 (0 generated)
dmikurube: as we chatted :) piman: this extends the patch you reviewed at https://chromiumcodereview.appspot.com/12211008, need OWNERS for content. palmer: OWNERS for the new IPC (which is only ever used when --enable-memory-benchmarking is enabled). thanks!
https://codereview.chromium.org/15082004/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/memory_benchmark_message_filter.cc (right): https://codereview.chromium.org/15082004/diff/2001/content/browser/renderer_h... content/browser/renderer_host/memory_benchmark_message_filter.cc:41: #if defined(USE_TCMALLOC) I think we need to make sure this can only happen when --enable-memory-benchmarking is set, e.g. by testing here, or by testing before adding the filter. https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... File content/common/memory_benchmark_messages.h (right): https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... content/common/memory_benchmark_messages.h:5: // IPC messages for device motion. nit: fix description. https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... content/common/memory_benchmark_messages.h:14: IPC_SYNC_MESSAGE_ROUTED0_1(MemoryBenchmarkHostMsg_HeapProfilerDump, This message doesn't need to be ROUTED, it could just be CONTROL. That way you don't need to find the right view in HeapProfilerDumpBrowser, you can directly go to RenderThreadImpl::current()->Send() https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... content/common/memory_benchmark_messages.h:15: std::string /* dump file name */) nit: use FilePath in IPCs.
https://codereview.chromium.org/15082004/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/memory_benchmark_message_filter.cc (right): https://codereview.chromium.org/15082004/diff/2001/content/browser/renderer_h... content/browser/renderer_host/memory_benchmark_message_filter.cc:41: #if defined(USE_TCMALLOC) > I think we need to make sure this can only happen when > --enable-memory-benchmarking is set, e.g. by testing here, or by testing before > adding the filter. Agreed. https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... File content/common/memory_benchmark_messages.h (right): https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... content/common/memory_benchmark_messages.h:15: std::string /* dump file name */) > nit: use FilePath in IPCs. It's more than a nit. :) An arbitrary file pathname write IPC, for memory benchmarking, cost us a Pwnium loss last year. The correct pattern is: client process asks browser process for a file descriptor/HANDLE to write to; browser chooses a pathname statically or otherwise safely; browser opens it with correct flags; browser sends FD back to client. (Options: The client can close the FD when done; or the client can send a message to the browser when it's time to close it and the browser actually calls close; or the browser can unilaterally close it at a time of its choosing.) Should this be guarded on #if defined(HAS_MEMORY_BENCHMARK) ? Part of the previous bug was that the IPC endpoint was present, even though no callers remained in a Release build.
thanks both! comments addressed, but I think I should clarify: this is sending file *name*, not actual FILE / fd, etc.. I changed the browser message filter to ensure it'll only kick in if the extra command line is passed (which is used only for benchmarks). but anyways: please keep all your eyes on it, I don't want to loose my sleep or make any of you concerned anytime. :) https://codereview.chromium.org/15082004/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/memory_benchmark_message_filter.cc (right): https://codereview.chromium.org/15082004/diff/2001/content/browser/renderer_h... content/browser/renderer_host/memory_benchmark_message_filter.cc:41: #if defined(USE_TCMALLOC) On 2013/05/10 18:13:52, piman wrote: > I think we need to make sure this can only happen when > --enable-memory-benchmarking is set, e.g. by testing here, or by testing before > adding the filter. good point! checking when adding the filter, so that this class assumes it has what it needs. https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... File content/common/memory_benchmark_messages.h (right): https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... content/common/memory_benchmark_messages.h:5: // IPC messages for device motion. On 2013/05/10 18:13:52, piman wrote: > nit: fix description. Done. https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... content/common/memory_benchmark_messages.h:14: IPC_SYNC_MESSAGE_ROUTED0_1(MemoryBenchmarkHostMsg_HeapProfilerDump, On 2013/05/10 18:13:52, piman wrote: > This message doesn't need to be ROUTED, it could just be CONTROL. That way you > don't need to find the right view in HeapProfilerDumpBrowser, you can directly > go to RenderThreadImpl::current()->Send() ah, right.. much simpler, thanks! https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... content/common/memory_benchmark_messages.h:15: std::string /* dump file name */) On 2013/05/10 18:13:52, piman wrote: > nit: use FilePath in IPCs. Done.
https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... File content/common/memory_benchmark_messages.h (right): https://codereview.chromium.org/15082004/diff/2001/content/common/memory_benc... content/common/memory_benchmark_messages.h:15: std::string /* dump file name */) On 2013/05/10 18:42:17, Chromium Palmer wrote: > > nit: use FilePath in IPCs. > > It's more than a nit. :) An arbitrary file pathname write IPC, for memory > benchmarking, cost us a Pwnium loss last year. > > The correct pattern is: client process asks browser process for a file > descriptor/HANDLE to write to; browser chooses a pathname statically or > otherwise safely; browser opens it with correct flags; browser sends FD back to > client. > > (Options: The client can close the FD when done; or the client can send a > message to the browser when it's time to close it and the browser actually calls > close; or the browser can unilaterally close it at a time of its choosing.) > > Should this be guarded on #if defined(HAS_MEMORY_BENCHMARK) ? Part of the > previous bug was that the IPC endpoint was present, even though no callers > remained in a Release build. sorry, I didn't address this comment in patchset #2, let me address this now...
cool, I addressed palmer's comments in patchset #3, ptal. now there'll be no entry points if the flag is not passed, and the code won't be compiled for platforms that don't support it anyways, i.e., it's just android and linux with tcmalloc on.. again, thanks for the extra pair of eyes! :)
https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:73: ::HeapProfilerDumpWithFileName(reason.c_str(), So, the renderer asks the browser for a FilePath (not ideal), and then calls ::HeapProfilerDumpWithFileName with the pathname of the FilePath. In theory, that shouldn't work because the renderer should be sandboxed and unable to open files. (::HeapProfilerDumpWithFileName ultimately does try to directly open the pathname; see third_party/tcmalloc/chromium/src/heap-profiler.cc.) Also in theory, nothing strictly requires the renderer to open the FilePath that the browser gave it. If the renderer can open files at all, it can open whatever pathname it wants; the browser has given advice rather than set effective policy. Have you tested this and found that the renderer can in fact open files? Because that would be a bug... (Not your fault or problem, but ::HeapProfilerDumpWithFileName is incorrectly declared as taking an int, rather than a size_t, for |filename_buffer_length|. Ironically, in the implementation, that int gets cast back to a size_t...)
Thanks for working on it, bulach. It's basically looks good to me. Just supplemental information for palmer's comment: https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:73: ::HeapProfilerDumpWithFileName(reason.c_str(), On 2013/05/10 19:53:53, Chromium Palmer wrote: > So, the renderer asks the browser for a FilePath (not ideal), and then calls > ::HeapProfilerDumpWithFileName with the pathname of the FilePath. > > In theory, that shouldn't work because the renderer should be sandboxed and > unable to open files. (::HeapProfilerDumpWithFileName ultimately does try to > directly open the pathname; see > third_party/tcmalloc/chromium/src/heap-profiler.cc.) > Yes, this is used under a very limited situation, much detailed memory profiling with using tcmalloc's heap profiler. Deep Memory Profiler (http://dev.chromium.org/developers/deep-memory-profiler) is an example of this. It is used in some buildbots (the "chromium.endure" waterfall, and more in future) and in developer's environment with a command line option --no-sandbox. > Also in theory, nothing strictly requires the renderer to open the FilePath that > the browser gave it. If the renderer can open files at all, it can open whatever > pathname it wants; the browser has given advice rather than set effective > policy. > The reason why the filename is returned is to give the filename to the test runner. The person who ran the test would want the filename dumped by the browser process. (The filename is decided by tcmalloc's heap profiler... No other way to know the dumped filename.) > Have you tested this and found that the renderer can in fact open files? Because > that would be a bug... > > (Not your fault or problem, but ::HeapProfilerDumpWithFileName is incorrectly > declared as taking an int, rather than a size_t, for |filename_buffer_length|. > Ironically, in the implementation, that int gets cast back to a size_t...)
On 2013/05/10 20:41:30, Dai Mikurube wrote: > Thanks for working on it, bulach. It's basically looks good to me. Just > supplemental information for palmer's comment: > > https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... > File content/renderer/memory_benchmarking_extension.cc (right): > > https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... > content/renderer/memory_benchmarking_extension.cc:73: > ::HeapProfilerDumpWithFileName(reason.c_str(), > On 2013/05/10 19:53:53, Chromium Palmer wrote: > > So, the renderer asks the browser for a FilePath (not ideal), and then calls > > ::HeapProfilerDumpWithFileName with the pathname of the FilePath. > > > > In theory, that shouldn't work because the renderer should be sandboxed and > > unable to open files. (::HeapProfilerDumpWithFileName ultimately does try to > > directly open the pathname; see > > third_party/tcmalloc/chromium/src/heap-profiler.cc.) > > > Yes, this is used under a very limited situation, much detailed memory profiling > with using tcmalloc's heap profiler. Deep Memory Profiler > (http://dev.chromium.org/developers/deep-memory-profiler) is an example of this. > It is used in some buildbots (the "chromium.endure" waterfall, and more in > future) and in developer's environment with a command line option --no-sandbox. > > > Also in theory, nothing strictly requires the renderer to open the FilePath > that > > the browser gave it. If the renderer can open files at all, it can open > whatever > > pathname it wants; the browser has given advice rather than set effective > > policy. > > > The reason why the filename is returned is to give the filename to the test > runner. The person who ran the test would want the filename dumped by the > browser process. (The filename is decided by tcmalloc's heap profiler... No > other way to know the dumped filename.) > > > Have you tested this and found that the renderer can in fact open files? > Because > > that would be a bug... > > > > (Not your fault or problem, but ::HeapProfilerDumpWithFileName is incorrectly > > declared as taking an int, rather than a size_t, for |filename_buffer_length|. > > Ironically, in the implementation, that int gets cast back to a size_t...) thanks for the detailed explanation Dai! palmer / piman: apologies for the lack of context, let me try to fill this in :) The ultimate goal is to reduce overall memory usage, specially on android. the first step in that direction is to obtain information on where the memory is going. there are several layers being worked on here: - TCMalloc: produces these "memory dumps", per process.. TCMalloc itself is being ported to android by dai and myself, among several other patches, there's: https://codereview.chromium.org/14321006/ https://codereview.chromium.org/14845011/ (worth noting: at this point, we have no intention to use TCMalloc as the default allocator for android yet... the initial goal is just so we can get memory dumps in the same format as other platforms... in the future, we'll investigate if it speeds up chrome as it did for other platforms). - test harness: this patch is the first one.. we'll use Telemetry to drive a test case, with these special command line flags being passed to chrome.. the test case itself will then call "chrome.memoryBenchmarking.heapProfilerDump()" to dump the memory maps.. the reason for this patch is so that we can also trigger the browser memory map... - the test harness then grabs all "file names" for these memory dumps... once the test case finishes, the Telemetry harness will then collect these files, and feed then to DMP (Deep Memory Profiler)... the important thing here is that Telemetry already knows that it needs to do "adb pull" to fetch android files from the target device. for linux, it just needs to copy from whetever they are (there's no distinction between host and target). - DMP will then process all of these dumps and provide nice looking graphs :) next "profit" step is then identify and reduce memory usage... so, whilst this is crucial for our memory reduction efforts, none of this code should ever be called outside such harness. I hope that helps!
OK, so the reason this code works is that you *do* use --no-sandbox. Thinking about it, I realize I am not sure that --no-sandbox does anything on Android? The Android sandbox is based on android:isolatedProcess="true", so you'd need some glue to tell Android Java code to not spawn renderers that way. But I don't see any such glue on a quick skim through the code.
LGTM from my pov. The file path sent to the renderer isn't sent again to the browser in prod, so I don't see a security concern.
thanks both! palmer: on android, we're running on rooted devices, and then the test harness will create a 777 directory that the renderers can write to it... we're doing it manually, the patch for the harness (telemetry) is being worked on here: https://codereview.chromium.org/15093008/ (not ready for review, but just to give you a general idea..)
Sounds OK. LGTM. Thanks for explaining everything.
lgtm.
https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:71: if (reason == "browser") Ahh, sorry, I overlooked the important part. I forgot that the old HeapProfilerDump() has already had an argument "reason". Actually, I thought to add a new argument like "process_type". If keeping consistency with the old PyAuto API, the arguments would be: HeapProfilerDump(process_type, reason) It is not consistent with the current V8 API, but I think it's acceptable since no one is using the V8 API yet.
On 2013/05/14 20:15:08, Dai Mikurube wrote: > https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... > File content/renderer/memory_benchmarking_extension.cc (right): > > https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... > content/renderer/memory_benchmarking_extension.cc:71: if (reason == "browser") > Ahh, sorry, I overlooked the important part. I forgot that the old > HeapProfilerDump() has already had an argument "reason". > > Actually, I thought to add a new argument like "process_type". If keeping > consistency with the old PyAuto API, the arguments would be: > > HeapProfilerDump(process_type, reason) > > It is not consistent with the current V8 API, but I think it's acceptable since > no one is using the V8 API yet. I'm not sure what is the difference from the V8 API and the pyauto api, but while at it.. :) on the Telemetry side, it'll just fetch all dump files from the target directory... in this case, could we simplify the V8 API as to not even return the file path? it doesn't seem it's going to be used... wdyt?
On 2013/05/15 08:05:17, bulach wrote: > On 2013/05/14 20:15:08, Dai Mikurube wrote: > > > https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... > > File content/renderer/memory_benchmarking_extension.cc (right): > > > > > https://codereview.chromium.org/15082004/diff/20001/content/renderer/memory_b... > > content/renderer/memory_benchmarking_extension.cc:71: if (reason == "browser") > > Ahh, sorry, I overlooked the important part. I forgot that the old > > HeapProfilerDump() has already had an argument "reason". > > > > Actually, I thought to add a new argument like "process_type". If keeping > > consistency with the old PyAuto API, the arguments would be: > > > > HeapProfilerDump(process_type, reason) > > > > It is not consistent with the current V8 API, but I think it's acceptable > since > > no one is using the V8 API yet. > > I'm not sure what is the difference from the V8 API and the pyauto api, but > while at it.. :) Thanks ;) > on the Telemetry side, it'll just fetch all dump files from the target > directory... in this case, could we simplify the V8 API as to not even return > the file path? it doesn't seem it's going to be used... wdyt? Files actually may be dumped from multiple processes like below. h.12345.0001.heap h.12345.0001.buckets h.12345.0002.heap h.12345.0002.buckets h.12345.maps h.12360.0001.heap h.12360.0001.buckets h.12360.maps h.12389.0001.heap h.12389.0001.buckets h.12389.0002.heap h.12389.0002.buckets h.12389.maps Therefore, we cannot tell which are the files that we want. We cannot get pid in Telemetry.
sorry about the churn! I chatted with dai, and given the other Telemetry patch: https://codereview.chromium.org/15093008/ we decided that: - there's no need for chrome.memoryBenchmarking.heapProfilerDump to return the path. - telemetry itself will just fetch the entire heap dump directory - it will also have a "browser.pid" file so that we know which dumps are for the browser process and which ones are for other processes this means that this patch is now a bit simpler :) would you mind another look please?
Thanks for working on it. Some comments. https://codereview.chromium.org/15082004/diff/35001/content/browser/renderer_... File content/browser/renderer_host/memory_benchmark_message_filter.cc (right): https://codereview.chromium.org/15082004/diff/35001/content/browser/renderer_... content/browser/renderer_host/memory_benchmark_message_filter.cc:7: #include "base/files/file_path.h" This "file_path.h" may not be requried? https://codereview.chromium.org/15082004/diff/35001/content/browser/renderer_... File content/browser/renderer_host/memory_benchmark_message_filter.h (right): https://codereview.chromium.org/15082004/diff/35001/content/browser/renderer_... content/browser/renderer_host/memory_benchmark_message_filter.h:13: class FilePath; This FilePath can be removed? https://codereview.chromium.org/15082004/diff/35001/content/common/memory_ben... File content/common/memory_benchmark_messages.h (right): https://codereview.chromium.org/15082004/diff/35001/content/common/memory_ben... content/common/memory_benchmark_messages.h:10: #include "base/files/file_path.h" May not be required. https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:10: #include "third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h" This "heap-profiler.h" should be included only when using TCMalloc. https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:37: " function(reason, process) {" I wonder if we could order it as (process_type, reason). It may break compatibility with the existing V8 HeapProfilerDump(), but I think it's acceptable since no one else has used it yet. The reason of this order is to keep consistency with the old PyAuto API HeapProfilerDump(). For example, Chrome Endure is switching from PyAuto to Telemetry in https://codereview.chromium.org/14977013/. They're having a Python function HeapProfilerDump() which wraps V8's HeapProfilerDump(). If is has the same interface, it's easy to understand and port. What do you think? https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:90: #endif // defined(USE_TCMALLOC) && (defined(OS_LINUX) || defined(OS_ANDROID)) I think these functions (at least chrome.memoryBenchmarking.isHeapProfilerRunning, but hopefully both) should be defined even if TCMalloc is not used. isHeapProfilerRunning should always return false in case of no tcmalloc. https://codereview.chromium.org/15082004/diff/35001/content/renderer/render_t... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/15082004/diff/35001/content/renderer/render_t... content/renderer/render_thread_impl.cc:400: #if defined(USE_TCMALLOC) && (defined(OS_LINUX) || defined(OS_ANDROID)) Same as the comment in memory_benchmarking_extension.cc.
Sorry, one more comment. And in addition, we may want to revert changes in
tcmalloc's heap-profiler.{cc|h} of http://crrev.com/182659 in this patch or
another patch.
https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b...
File content/renderer/memory_benchmarking_extension.cc (right):
https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b...
content/renderer/memory_benchmarking_extension.cc:39: " return
HeapProfilerDump(reason, process);"
Same argument order for this HeapProfilerDump().
And, we may not need to return a value here.
thanks dai! all comments addressed, ptal. would you like to drover that other patch you mentioned? if nothing else is depending on it, I'd be happy to rebase this patch after you revert that. https://codereview.chromium.org/15082004/diff/35001/content/browser/renderer_... File content/browser/renderer_host/memory_benchmark_message_filter.cc (right): https://codereview.chromium.org/15082004/diff/35001/content/browser/renderer_... content/browser/renderer_host/memory_benchmark_message_filter.cc:7: #include "base/files/file_path.h" On 2013/05/15 10:24:04, Dai Mikurube wrote: > This "file_path.h" may not be requried? Done. https://codereview.chromium.org/15082004/diff/35001/content/browser/renderer_... File content/browser/renderer_host/memory_benchmark_message_filter.h (right): https://codereview.chromium.org/15082004/diff/35001/content/browser/renderer_... content/browser/renderer_host/memory_benchmark_message_filter.h:13: class FilePath; On 2013/05/15 10:24:04, Dai Mikurube wrote: > This FilePath can be removed? Done. https://codereview.chromium.org/15082004/diff/35001/content/common/memory_ben... File content/common/memory_benchmark_messages.h (right): https://codereview.chromium.org/15082004/diff/35001/content/common/memory_ben... content/common/memory_benchmark_messages.h:10: #include "base/files/file_path.h" On 2013/05/15 10:24:04, Dai Mikurube wrote: > May not be required. Done. https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:10: #include "third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h" On 2013/05/15 10:24:04, Dai Mikurube wrote: > This "heap-profiler.h" should be included only when using TCMalloc. Done. https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:37: " function(reason, process) {" On 2013/05/15 10:24:04, Dai Mikurube wrote: > I wonder if we could order it as (process_type, reason). It may break > compatibility with the existing V8 HeapProfilerDump(), but I think it's > acceptable since no one else has used it yet. > > The reason of this order is to keep consistency with the old PyAuto API > HeapProfilerDump(). For example, Chrome Endure is switching from PyAuto to > Telemetry in https://codereview.chromium.org/14977013/. They're having a Python > function HeapProfilerDump() which wraps V8's HeapProfilerDump(). If is has the > same interface, it's easy to understand and port. > > What do you think? sounds reasonable, done. https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:39: " return HeapProfilerDump(reason, process);" On 2013/05/15 10:36:55, Dai Mikurube wrote: > Same argument order for this HeapProfilerDump(). > And, we may not need to return a value here. Done. https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:90: #endif // defined(USE_TCMALLOC) && (defined(OS_LINUX) || defined(OS_ANDROID)) On 2013/05/15 10:24:04, Dai Mikurube wrote: > I think these functions (at least > chrome.memoryBenchmarking.isHeapProfilerRunning, but hopefully both) should be > defined even if TCMalloc is not used. isHeapProfilerRunning should always > return false in case of no tcmalloc. hmm.. see the comment from palmer, I suppose if we're not going to use, it's preferred to not have these functions at all (less end-points, etc..). the very few clients that actually need it can test (chrome && chrome.memoryBenchmarking)... wdyt? https://codereview.chromium.org/15082004/diff/35001/content/renderer/render_t... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/15082004/diff/35001/content/renderer/render_t... content/renderer/render_thread_impl.cc:400: #if defined(USE_TCMALLOC) && (defined(OS_LINUX) || defined(OS_ANDROID)) On 2013/05/15 10:24:04, Dai Mikurube wrote: > Same as the comment in memory_benchmarking_extension.cc. same response :)
LGTM! :) And, I'll be reverting r182659 in https://codereview.chromium.org/14773032/. (tree closed) https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/35001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:90: #endif // defined(USE_TCMALLOC) && (defined(OS_LINUX) || defined(OS_ANDROID)) On 2013/05/15 10:58:29, bulach wrote: > On 2013/05/15 10:24:04, Dai Mikurube wrote: > > I think these functions (at least > > chrome.memoryBenchmarking.isHeapProfilerRunning, but hopefully both) should be > > defined even if TCMalloc is not used. isHeapProfilerRunning should always > > return false in case of no tcmalloc. > > hmm.. see the comment from palmer, I suppose if we're not going to use, it's > preferred to not have these functions at all (less end-points, etc..). > > the very few clients that actually need it can test (chrome && > chrome.memoryBenchmarking)... wdyt? Hmm, makes sense. Ok. :)
fyi: revert is landed at http://crrev.com/200250
thanks dai! merged with your revert. I'll wait for a final look from palmer / piman before landing.
> I'll wait for a final look from palmer / piman before landing. Thanks! Looking now.
LGTM+nit https://codereview.chromium.org/15082004/diff/59001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/59001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:62: } nit: you inlined this function into HeapProfilerDump. Either keep it and call it, or remove it?
https://codereview.chromium.org/15082004/diff/59001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/59001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:59: static void HeapProfilerDumpBrowser(const std::string& reason) { What is the range of possible |reason|s? Normally I'd want to use an enum, and stringify it in a trustworthy place, only where needed.
Supplemental information for palmer: https://codereview.chromium.org/15082004/diff/59001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/59001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:59: static void HeapProfilerDumpBrowser(const std::string& reason) { On 2013/05/15 20:27:50, Chromium Palmer wrote: > What is the range of possible |reason|s? Normally I'd want to use an enum, and > stringify it in a trustworthy place, only where needed. It can be any string, and it has no effects on any behavior. This |reason| is just a message for tcmalloc'c HeapProfilerDump and it's only logged. It is used like: > HeapPofilerDump("Dumping for checking memory consumption of func()."); (See https://code.google.com/p/gperftools/source/browse/trunk/src/gperftools/heap-...)
https://codereview.chromium.org/15082004/diff/59001/content/renderer/memory_b... File content/renderer/memory_benchmarking_extension.cc (right): https://codereview.chromium.org/15082004/diff/59001/content/renderer/memory_b... content/renderer/memory_benchmarking_extension.cc:62: } On 2013/05/15 19:18:54, piman wrote: > nit: you inlined this function into HeapProfilerDump. Either keep it and call > it, or remove it? ops, sorry, I forgot to remove. done.
palmer: ping? :)
On 2013/05/21 08:25:25, bulach wrote: > palmer: ping? :) Oops, sorry. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/15082004/74001
Message was sent while issue was closed.
Change committed as 202087 |
