Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1781213002:
Add malloc/new profiling on linux (Closed)
|
Created:
4 years, 9 months ago by sehr Modified:
4 years, 9 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd malloc/new profiling on linux
BUG=
R=jpp@chromium.org, kschimpf@google.com, stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=4c16ac0f4f184128060f3b8c3db519f1a6293dfa
Patch Set 1 #Patch Set 2 : Cleaned up API, added operator new[] #Patch Set 3 : Refactoring #
Total comments: 32
Patch Set 4 : Code review fixes. #
Total comments: 24
Patch Set 5 : More code reviews #
Total comments: 8
Messages
Total messages: 15 (4 generated)
Description was changed from ========== Add malloc/new profiling on linux BUG= ========== to ========== Add malloc/new profiling on linux BUG= ==========
sehr@chromium.org changed reviewers: + jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org
PTAL.
kschimpf@google.com changed reviewers: + kschimpf@google.com
lgtm https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:67: for (auto C : *Callers) { Should you use? for (auto &C : *Callers) {
https://codereview.chromium.org/1781213002/diff/40001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1781213002/diff/40001/Makefile.standalone#new... Makefile.standalone:155: ifdef LINUX_MALLOC_PROFILE Update OBJDIR, something like: OBJDIR := $(OBJDIR)+MalProf Otherwise "make" won't necessarily rebuild source files when you add LINUX_MALLOC_PROFILE to the command line. https://codereview.chromium.org/1781213002/diff/40001/Makefile.standalone#new... Makefile.standalone:156: CXX_DEFINES += -DALLOW_LINUX_MALLOC_PROFILE=1 Can you update BASE_CXX_DEFINES instead of CXX_DEFINES? https://codereview.chromium.org/1781213002/diff/40001/Makefile.standalone#new... Makefile.standalone:299: SRCS += LinuxMallocProfiling.cpp I actually think this source file should be added to SRCS unconditionally, and its entire contents should be wrapped in an #ifdef. This is because the LLVM build system does not use Makefile.standalone, instead it uses Makefile and CMakeLists.txt. Better yet, if it at least compiles on all platforms, use BuildDefs so the compiler normally compiles it but optimizes it away. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:17: #include <malloc.h> alphabetize these. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:25: std::map<void*, uint64_t> *Callers; Use std::unordered_map<>, no? Also, please use a typedef instead. using MapType = std::unordered_map<void *, uint64_t>; https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:30: void *InternalAllocator(size_t size, void *caller) { LLVM coding style would be: void *internalAllocator(size_t Size, void *Caller) { https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:33: auto SetI = Callers->find(caller); Can't you just simply: ++(*Callers)[caller]; ? https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:43: } // end anonymous namespace end of anonymous namespace https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:77: std::cout << std::endl; I believe LLVM coding style says use "\n" instead of std::endl. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:79: delete Callers; Our code base favors unique_ptr<> over explicit delete. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:83: } // end namespace Ice end of namespace Ice https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfiling.h File src/LinuxMallocProfiling.h (right): https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:19: static void Start(); name these start() and end() https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:23: } // end namespace Ice end of namespace Ice https://codereview.chromium.org/1781213002/diff/40001/src/main.cpp File src/main.cpp (right): https://codereview.chromium.org/1781213002/diff/40001/src/main.cpp#newcode52 src/main.cpp:52: int error = Ice::CLCompileServer(argc, argv).runAndReturnErrorCode(); s/error/Error/
https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:25: std::map<void*, uint64_t> *Callers; Probably should add a note somewhere that this is not thread-safe? Could even check the flags for threads=0.
PTAL. https://codereview.chromium.org/1781213002/diff/40001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1781213002/diff/40001/Makefile.standalone#new... Makefile.standalone:155: ifdef LINUX_MALLOC_PROFILE On 2016/03/12 00:12:57, stichnot wrote: > Update OBJDIR, something like: > > OBJDIR := $(OBJDIR)+MalProf > > Otherwise "make" won't necessarily rebuild source files when you add > LINUX_MALLOC_PROFILE to the command line. Done. https://codereview.chromium.org/1781213002/diff/40001/Makefile.standalone#new... Makefile.standalone:156: CXX_DEFINES += -DALLOW_LINUX_MALLOC_PROFILE=1 On 2016/03/12 00:12:58, stichnot wrote: > Can you update BASE_CXX_DEFINES instead of CXX_DEFINES? Done. https://codereview.chromium.org/1781213002/diff/40001/Makefile.standalone#new... Makefile.standalone:299: SRCS += LinuxMallocProfiling.cpp On 2016/03/12 00:12:57, stichnot wrote: > I actually think this source file should be added to SRCS unconditionally, and > its entire contents should be wrapped in an #ifdef. This is because the LLVM > build system does not use Makefile.standalone, instead it uses Makefile and > CMakeLists.txt. > > Better yet, if it at least compiles on all platforms, use BuildDefs so the > compiler normally compiles it but optimizes it away. I added the source unconditionally, and the bodies of the functions are empty unless the flag is defined. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:17: #include <malloc.h> On 2016/03/12 00:12:58, stichnot wrote: > alphabetize these. Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:25: std::map<void*, uint64_t> *Callers; On 2016/03/12 00:12:58, stichnot wrote: > Use std::unordered_map<>, no? > > Also, please use a typedef instead. > > using MapType = std::unordered_map<void *, uint64_t>; Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:25: std::map<void*, uint64_t> *Callers; On 2016/03/12 00:36:15, stichnot wrote: > Probably should add a note somewhere that this is not thread-safe? Could even > check the flags for threads=0. Added here and a warning comes out if a build including the code is run with threads!=0. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:30: void *InternalAllocator(size_t size, void *caller) { On 2016/03/12 00:12:58, stichnot wrote: > LLVM coding style would be: > > void *internalAllocator(size_t Size, void *Caller) { Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:33: auto SetI = Callers->find(caller); On 2016/03/12 00:12:58, stichnot wrote: > Can't you just simply: > > ++(*Callers)[caller]; > > ? Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:43: } // end anonymous namespace On 2016/03/12 00:12:58, stichnot wrote: > end of anonymous namespace Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:67: for (auto C : *Callers) { On 2016/03/11 23:25:11, Karl wrote: > Should you use? > for (auto &C : *Callers) { Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:77: std::cout << std::endl; On 2016/03/12 00:12:58, stichnot wrote: > I believe LLVM coding style says use "\n" instead of std::endl. Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:79: delete Callers; On 2016/03/12 00:12:58, stichnot wrote: > Our code base favors unique_ptr<> over explicit delete. The Callers map needs to be accessible to non-member functions, and unique_ptr would need to be a constructed object in static data, which is also unacceptable. Suggestions? https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:83: } // end namespace Ice On 2016/03/12 00:12:58, stichnot wrote: > end of namespace Ice Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfiling.h File src/LinuxMallocProfiling.h (right): https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:19: static void Start(); On 2016/03/12 00:12:58, stichnot wrote: > name these start() and end() Done. https://codereview.chromium.org/1781213002/diff/40001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:23: } // end namespace Ice On 2016/03/12 00:12:58, stichnot wrote: > end of namespace Ice Done. https://codereview.chromium.org/1781213002/diff/40001/src/main.cpp File src/main.cpp (right): https://codereview.chromium.org/1781213002/diff/40001/src/main.cpp#newcode52 src/main.cpp:52: int error = Ice::CLCompileServer(argc, argv).runAndReturnErrorCode(); On 2016/03/12 00:12:58, stichnot wrote: > s/error/Error/ Done.
lgtm https://codereview.chromium.org/1781213002/diff/60001/src/IceCompileServer.cpp File src/IceCompileServer.cpp (right): https://codereview.chromium.org/1781213002/diff/60001/src/IceCompileServer.cp... src/IceCompileServer.cpp:171: Ice::LinuxMallocProfiling MallocProfile(Flags.getNumTranslationThreads()); you don't need MallocProfile for anything but RAII, right? I would **suggest** renaming it to something less "visible" (I tend to name these RAII variables _) https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:32: // --threads=0 to enable profilin. profiling https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:46: void *operator new (size_t size) { can you add a comment explaining why this works? https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:75: for (auto &C : *Callers) { does const auto & work? https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfiling.h File src/LinuxMallocProfiling.h (right): https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:13: //===----------------------------------------------------------------------===// you're missing the #ifndef LINUXMALLOCPROFILING_H #define LINUXMALLOCPROFILING_H #endif // LINUXMALLOCPROFILING_H https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:24: explicit LinuxMallocProfiling(size_t NumThreads) { can you move the definitions to the .cpp file so that iostream is not transitively included in a bunch of places? (Note that there's no guard preventing it from being included.) https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:39: LinuxMallocProfiling(const LinuxMallocProfiling&) = delete; I honestly believe this is the proper place for deleting ctors and assignment operators, but the stupid LLVM coding convention requires these deletions as the first declarations in the class -- yeah, I am no a fan of the LLVM coding style... :P https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:41: static void start(); if you move the ctor/dtor implementation to the .cpp file you can simply inline start/stop in those methods... https://codereview.chromium.org/1781213002/diff/60001/src/main.cpp File src/main.cpp (right): https://codereview.chromium.org/1781213002/diff/60001/src/main.cpp#newcode19 src/main.cpp:19: #ifdef ALLOW_LINUX_MALLOC_PROFILE if you guard entire LinuxMallocProfiling.h file with #ifdef ALLOW_LINUX_MALLOC_PROFILE #endif // ALLOW_LINUX_MALLOC_PROFILE then you could simply #include it here.
https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:17: #ifdef ALLOW_LINUX_MALLOC_PROFILE I don't like all the #ifdef here. I think it would be better to do something like this: #ifdef ALLOW_LINUX_MALLOC_PROFILE ... full implementation ... #else // !ALLOW_LINUX_MALLOC_PROFILE ... minimal skeleton implementation #endif // !ALLOW_LINUX_MALLOC_PROFILE https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:20: #include <iostream> LLVM sez iostream is forbidden. http://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden Can you use GlobalContext::StrDump instead? https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfiling.h File src/LinuxMallocProfiling.h (right): https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:23: public: This doesn't look like normal clang-format indentation...
PTAL. https://codereview.chromium.org/1781213002/diff/60001/src/IceCompileServer.cpp File src/IceCompileServer.cpp (right): https://codereview.chromium.org/1781213002/diff/60001/src/IceCompileServer.cp... src/IceCompileServer.cpp:171: Ice::LinuxMallocProfiling MallocProfile(Flags.getNumTranslationThreads()); On 2016/03/15 15:19:06, John wrote: > you don't need MallocProfile for anything but RAII, right? I would **suggest** > renaming it to something less "visible" (I tend to name these RAII variables _) Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:17: #ifdef ALLOW_LINUX_MALLOC_PROFILE On 2016/03/15 15:36:50, stichnot wrote: > I don't like all the #ifdef here. > > I think it would be better to do something like this: > > #ifdef ALLOW_LINUX_MALLOC_PROFILE > ... full implementation ... > #else // !ALLOW_LINUX_MALLOC_PROFILE > ... minimal skeleton implementation > #endif // !ALLOW_LINUX_MALLOC_PROFILE Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:20: #include <iostream> On 2016/03/15 15:36:50, stichnot wrote: > LLVM sez iostream is forbidden. > > http://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden > > Can you use GlobalContext::StrDump instead? I plumbed through the Ostream that was already used in this file. PTAL. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:32: // --threads=0 to enable profilin. On 2016/03/15 15:19:06, John wrote: > profiling Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:46: void *operator new (size_t size) { On 2016/03/15 15:19:06, John wrote: > can you add a comment explaining why this works? Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:75: for (auto &C : *Callers) { On 2016/03/15 15:19:06, John wrote: > does > > const auto & > > work? Yes. Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfiling.h File src/LinuxMallocProfiling.h (right): https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:13: //===----------------------------------------------------------------------===// On 2016/03/15 15:19:06, John wrote: > you're missing the > > #ifndef LINUXMALLOCPROFILING_H > #define LINUXMALLOCPROFILING_H > > #endif // LINUXMALLOCPROFILING_H Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:23: public: On 2016/03/15 15:36:50, stichnot wrote: > This doesn't look like normal clang-format indentation... Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:24: explicit LinuxMallocProfiling(size_t NumThreads) { On 2016/03/15 15:19:06, John wrote: > can you move the definitions to the .cpp file so that iostream is not > transitively included in a bunch of places? (Note that there's no guard > preventing it from being included.) Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:39: LinuxMallocProfiling(const LinuxMallocProfiling&) = delete; On 2016/03/15 15:19:06, John wrote: > I honestly believe this is the proper place for deleting ctors and assignment > operators, but the stupid LLVM coding convention requires these deletions as the > first declarations in the class -- yeah, I am no a fan of the LLVM coding > style... :P Done. https://codereview.chromium.org/1781213002/diff/60001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:41: static void start(); On 2016/03/15 15:19:06, John wrote: > if you move the ctor/dtor implementation to the .cpp file you can simply inline > start/stop in those methods... Done. https://codereview.chromium.org/1781213002/diff/60001/src/main.cpp File src/main.cpp (right): https://codereview.chromium.org/1781213002/diff/60001/src/main.cpp#newcode19 src/main.cpp:19: #ifdef ALLOW_LINUX_MALLOC_PROFILE On 2016/03/15 15:19:07, John wrote: > if you guard entire LinuxMallocProfiling.h file with > > #ifdef ALLOW_LINUX_MALLOC_PROFILE > #endif // ALLOW_LINUX_MALLOC_PROFILE > > then you could simply #include it here. Actually, it should have been completely removed here... Done.
lgtm https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:17: #ifdef ALLOW_LINUX_MALLOC_PROFILE I would be happier with a blank line before and after each #ifdef/#else/#endif. https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:45: // new, new[], and malloc are all defined as weak symbols to allow them reflow comment to 80-col https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:70: << "Use --threads=0 to enable.\n"; Tiny nit: omit the last "<<" and take advantage of string literal concatenation. https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfiling.h File src/LinuxMallocProfiling.h (right): https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:19: #include "IceTypes.h" Can this be removed? I don't see anything in this header file that ought to depend on IceTypes.h.
Description was changed from ========== Add malloc/new profiling on linux BUG= ========== to ========== Add malloc/new profiling on linux BUG= R=jpp@chromium.org, kschimpf@google.com, stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 4c16ac0f4f184128060f3b8c3db519f1a6293dfa (presubmit successful).
Message was sent while issue was closed.
Thanks. Committed. https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... File src/LinuxMallocProfiling.cpp (right): https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:17: #ifdef ALLOW_LINUX_MALLOC_PROFILE On 2016/03/17 20:09:06, stichnot wrote: > I would be happier with a blank line before and after each #ifdef/#else/#endif. Done. https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:45: // new, new[], and malloc are all defined as weak symbols to allow them On 2016/03/17 20:09:05, stichnot wrote: > reflow comment to 80-col Done. https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.cpp:70: << "Use --threads=0 to enable.\n"; On 2016/03/17 20:09:06, stichnot wrote: > Tiny nit: omit the last "<<" and take advantage of string literal concatenation. Done. https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfiling.h File src/LinuxMallocProfiling.h (right): https://codereview.chromium.org/1781213002/diff/80001/src/LinuxMallocProfilin... src/LinuxMallocProfiling.h:19: #include "IceTypes.h" On 2016/03/17 20:09:06, stichnot wrote: > Can this be removed? I don't see anything in this header file that ought to > depend on IceTypes.h. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
