Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(363)

Issue 10411047: Type profiler by intercepting 'new' and 'delete' expressions. (Closed)

Created:
8 years, 7 months ago by Dai Mikurube (NOT FULLTIME)
Modified:
8 years, 2 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

Type profiler by intercepting 'new' and 'delete' expressions. It stores mapping between object's starting addresses and their allocated types when a build option 'clang_type_profiler=1' is specified. It enables information like "an object at 0x37f3c88 is an instance of std::string." Nothing is changed when the option is not specified. It depends on a modified version of the LLVM/Clang compiler introduced at deps/third_party/llvm-allocated-type. BUG=123758 TEST=build with clang_type_profiler=1 and run type_profiler_unittests and type_profiler_map_unittests manually. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158752

Patch Set 1 #

Patch Set 2 : refined. #

Patch Set 3 : Fixed? #

Patch Set 4 : loadable_module #

Patch Set 5 : Add logging in operator new. #

Patch Set 6 : Rebased. #

Patch Set 7 : Include <typeinfo> and declare operator new in Chromium's side. #

Patch Set 8 : refined. #

Patch Set 9 : Rebased and added a license comment. #

Patch Set 10 : Add delete overload and some additional anti-warning flags for the latest Clang trunk. #

Patch Set 11 : Store allocated types in AddressMap of TCMalloc. #

Patch Set 12 : Print allocated sizes per every types. #

Patch Set 13 : Renamed some files. #

Patch Set 14 : rebased, and updated. #

Patch Set 15 : Updated LookupAllocatedType. #

Patch Set 16 : Support operator delete[]. #

Total comments: 1

Patch Set 17 : first support for dumping allocated type #

Patch Set 18 : Use -fintercept-allocation-functions. #

Patch Set 19 : another summary for comparison #

Patch Set 20 : suspend type logging between fork and exec. #

Patch Set 21 : separate memory for allocated-type out of dmprof. #

Patch Set 22 : Separated out dumper. #

Patch Set 23 : modified gyp(i) #

Patch Set 24 : remove some comments #

Total comments: 74

Patch Set 25 : reflected the comments. #

Patch Set 26 : upload again #

Total comments: 4

Patch Set 27 : reflected the comments #

Patch Set 28 : stop interceptors in an entire child process #

Total comments: 19

Patch Set 29 : reflected the comments #17-#19. #

Total comments: 22

Patch Set 30 : reflected comments #21,22 #

Total comments: 72

Patch Set 31 : reflected the comment #24. #

Patch Set 32 : updated #

Patch Set 33 : commented otu #

Patch Set 34 : use RAW_DCHECK in allocated_type_map.cc. #

Patch Set 35 : refined include path. #

Patch Set 36 : fixed dependencies in gyp #

Total comments: 5

Patch Set 37 : fix gyp #

Total comments: 7

Patch Set 38 : reflected maruel's comment. Ready for review. #

Total comments: 30

Patch Set 39 : reflected Jim's comments. #

Total comments: 5

Patch Set 40 : reflected jar's comments, and added tests for allocated_type_profiler and AllocatedTypeMap. #

Total comments: 2

Patch Set 41 : removed "allocated" from all names. #

Patch Set 42 : fixed comments. #

Total comments: 34

Patch Set 43 : reflected the comments. #

Total comments: 15

Patch Set 44 : reflected #

Patch Set 45 : fixed #include position #

Total comments: 2

Patch Set 46 : stopped NULL checks #

Patch Set 47 : modified a comment. #

Patch Set 48 : fix IsAvailable #

Total comments: 25

Patch Set 49 : fixed for jar's comments #67 #

Total comments: 6

Patch Set 50 : removed automatic including "type_profiler.h" and fixed for jar's comments. #

Total comments: 12

Patch Set 51 : fixed for jar's comments #72 #

Total comments: 20

Patch Set 52 : fixed for Ryan's comments #78 #

Total comments: 18

Patch Set 53 : removed unnecessary friend #

Total comments: 8

Patch Set 54 : moved/added/deleted #if and #include #

Patch Set 55 : fixed for Ryan's comments #83 #

Patch Set 56 : nit fix #

Patch Set 57 : nit fix 2 #

Total comments: 4

Patch Set 58 : fixed for Ryan's comments #86. #

Patch Set 59 : rebased #

Patch Set 60 : rebased #

Total comments: 8

Patch Set 61 : fixed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+830 lines, -6 lines) Patch
M base/allocator/allocator.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 3 chunks +111 lines, -0 lines 0 comments Download
A base/allocator/type_profiler.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 1 chunk +40 lines, -0 lines 0 comments Download
A base/allocator/type_profiler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 1 chunk +63 lines, -0 lines 0 comments Download
A base/allocator/type_profiler_control.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 1 chunk +31 lines, -0 lines 0 comments Download
A base/allocator/type_profiler_control.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 1 chunk +38 lines, -0 lines 0 comments Download
A base/allocator/type_profiler_map_unittests.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 1 chunk +99 lines, -0 lines 0 comments Download
A base/allocator/type_profiler_tcmalloc.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 1 chunk +29 lines, -0 lines 0 comments Download
A base/allocator/type_profiler_tcmalloc.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 1 chunk +37 lines, -0 lines 0 comments Download
A base/allocator/type_profiler_unittests.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 1 chunk +189 lines, -0 lines 0 comments Download
M base/base.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 2 chunks +4 lines, -0 lines 0 comments Download
M base/process_util_posix.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 3 chunks +11 lines, -0 lines 0 comments Download
M build/common.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 4 chunks +25 lines, -0 lines 0 comments Download
M content/app/content_main_runner.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 2 chunks +13 lines, -2 lines 0 comments Download
A third_party/tcmalloc/chromium/src/gperftools/type_profiler_map.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +20 lines, -0 lines 0 comments Download
A third_party/tcmalloc/chromium/src/type_profiler_map.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +112 lines, -0 lines 0 comments Download
M tools/deep_memory_profiler/dmprof View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 1 chunk +2 lines, -1 line 0 comments Download
M tools/deep_memory_profiler/policy.l0.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 1 chunk +2 lines, -1 line 0 comments Download
M tools/deep_memory_profiler/policy.l1.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 1 chunk +2 lines, -1 line 0 comments Download
M tools/deep_memory_profiler/policy.l2.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 99 (0 generated)
Dai Mikurube (NOT FULLTIME)
http://codereview.chromium.org/10411047/diff/35001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/10411047/diff/35001/build/common.gypi#newcode2693 build/common.gypi:2693: Add -Wno-unused-private-field here.
8 years, 6 months ago (2012-06-15 19:54:34 UTC) #1
Dai Mikurube (NOT FULLTIME)
Hi, Could you please take a look at this change? (Or, could you forward it ...
8 years, 4 months ago (2012-08-02 07:43:54 UTC) #2
Alexander Potapenko
http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h File base/allocator/allocated_type.h (right): http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h#newcode12 base/allocator/allocated_type.h:12: void* __op_delete_intercept__(void*, size_t, const std::type_info &); If you need ...
8 years, 4 months ago (2012-08-02 14:57:12 UTC) #3
Nico
http://codereview.chromium.org/10411047/diff/71001/base/process_util_posix.cc File base/process_util_posix.cc (right): http://codereview.chromium.org/10411047/diff/71001/base/process_util_posix.cc#newcode650 base/process_util_posix.cc:650: #endif On 2012/08/02 07:43:55, Dai Mikurube wrote: > These ...
8 years, 4 months ago (2012-08-02 15:35:03 UTC) #4
Dai Mikurube (NOT FULLTIME)
Hi Alexander, Nico, Thank you for the comments. http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h File base/allocator/allocated_type.h (right): http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h#newcode12 base/allocator/allocated_type.h:12: void* ...
8 years, 4 months ago (2012-08-02 16:14:35 UTC) #5
Nico
http://codereview.chromium.org/10411047/diff/71001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/10411047/diff/71001/build/common.gypi#newcode1617 build/common.gypi:1617: '<!(realpath <(DEPTH)/base/allocator/allocator.gyp)' + ':allocated_type_tcmalloc#target', On 2012/08/02 16:14:35, Dai Mikurube ...
8 years, 4 months ago (2012-08-02 16:18:49 UTC) #6
Dai Mikurube (NOT FULLTIME)
http://codereview.chromium.org/10411047/diff/71001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/10411047/diff/71001/build/common.gypi#newcode1617 build/common.gypi:1617: '<!(realpath <(DEPTH)/base/allocator/allocator.gyp)' + ':allocated_type_tcmalloc#target', On 2012/08/02 16:18:49, Nico wrote: ...
8 years, 4 months ago (2012-08-02 16:54:39 UTC) #7
jar (doing other things)
http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h File base/allocator/allocated_type.h (right): http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h#newcode11 base/allocator/allocated_type.h:11: void* __op_new_intercept__(void*, size_t, const std::type_info&); nit: Declarations should have ...
8 years, 4 months ago (2012-08-02 23:38:30 UTC) #8
Alexander Potapenko
http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h File base/allocator/allocated_type.h (right): http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h#newcode5 base/allocator/allocated_type.h:5: #ifndef BASE_ALLOCATOR_ALLOCATED_TYPE_INTERCEPT_H_ Please either rename the file or the ...
8 years, 4 months ago (2012-08-03 09:57:07 UTC) #9
Dai Mikurube (NOT FULLTIME)
Thank you, all. Replied and updated the patch. http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h File base/allocator/allocated_type.h (right): http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type.h#newcode11 base/allocator/allocated_type.h:11: void* ...
8 years, 4 months ago (2012-08-03 10:01:49 UTC) #10
Dai Mikurube (NOT FULLTIME)
Thanks for your comments, Alexander. We passed each other. I'll do them later. (maybe next ...
8 years, 4 months ago (2012-08-03 10:04:01 UTC) #11
Dai Mikurube (NOT FULLTIME)
I was uploading a wrong patch... Uploaded as #26.
8 years, 4 months ago (2012-08-03 16:51:53 UTC) #12
jar (doing other things)
http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type_tcmalloc.cc File base/allocator/allocated_type_tcmalloc.cc (right): http://codereview.chromium.org/10411047/diff/71001/base/allocator/allocated_type_tcmalloc.cc#newcode11 base/allocator/allocated_type_tcmalloc.cc:11: volatile Atomic32 do_not_intercept = 0; Please use a positive ...
8 years, 4 months ago (2012-08-04 01:13:47 UTC) #13
Dai Mikurube (NOT FULLTIME)
Thanks for the comments. The uploaded patchset doesn't reflect all comments yet, but at least ...
8 years, 4 months ago (2012-08-07 10:39:18 UTC) #14
Dai Mikurube (NOT FULLTIME)
Finally, LaunchProcess and GetAppOutputInternal stops interceptors for an entire child process. Renamed the function name ...
8 years, 4 months ago (2012-08-07 11:30:42 UTC) #15
Dai Mikurube (NOT FULLTIME)
http://codereview.chromium.org/10411047/diff/78015/base/process_util_posix.cc File base/process_util_posix.cc (right): http://codereview.chromium.org/10411047/diff/78015/base/process_util_posix.cc#newcode649 base/process_util_posix.cc:649: #endif Note: I kept #ifdef here since the file ...
8 years, 4 months ago (2012-08-07 11:35:55 UTC) #16
Alexander Potapenko
http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type.h File base/allocator/allocated_type.h (right): http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type.h#newcode21 base/allocator/allocated_type.h:21: const std::type_info & type); Please fix the space. http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type_ignore.cc ...
8 years, 4 months ago (2012-08-07 11:56:56 UTC) #17
Alexander Potapenko
http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type.h File base/allocator/allocated_type.h (right): http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type.h#newcode20 base/allocator/allocated_type.h:20: size_t size, BTW why do you need |size| and ...
8 years, 4 months ago (2012-08-07 12:05:21 UTC) #18
jar (doing other things)
http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type_tcmalloc.cc File base/allocator/allocated_type_tcmalloc.cc (right): http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type_tcmalloc.cc#newcode25 base/allocator/allocated_type_tcmalloc.cc:25: if (base::subtle::NoBarrier_Load(&g_enable_intercept) != 0) { You are using atomics ...
8 years, 4 months ago (2012-08-07 17:07:34 UTC) #19
Dai Mikurube (NOT FULLTIME)
Thank you for the comments, Alexander and Jim. Updated the patch. http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type.h File base/allocator/allocated_type.h (right): ...
8 years, 4 months ago (2012-08-08 06:16:22 UTC) #20
Alexander Potapenko
Two nits. The change looks good in general. http://codereview.chromium.org/10411047/diff/72024/base/allocator/allocated_type_ignore.cc File base/allocator/allocated_type_ignore.cc (right): http://codereview.chromium.org/10411047/diff/72024/base/allocator/allocated_type_ignore.cc#newcode6 base/allocator/allocated_type_ignore.cc:6: #include ...
8 years, 4 months ago (2012-08-08 11:27:14 UTC) #21
jar (doing other things)
http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type_tcmalloc.cc File base/allocator/allocated_type_tcmalloc.cc (right): http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type_tcmalloc.cc#newcode25 base/allocator/allocated_type_tcmalloc.cc:25: if (base::subtle::NoBarrier_Load(&g_enable_intercept) != 0) { Please cite a reference, ...
8 years, 4 months ago (2012-08-08 23:21:03 UTC) #22
Dai Mikurube (NOT FULLTIME)
Thank you for reviewing. Updated the patch. http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type_tcmalloc.cc File base/allocator/allocated_type_tcmalloc.cc (right): http://codereview.chromium.org/10411047/diff/78015/base/allocator/allocated_type_tcmalloc.cc#newcode25 base/allocator/allocated_type_tcmalloc.cc:25: if (base::subtle::NoBarrier_Load(&g_enable_intercept) ...
8 years, 4 months ago (2012-08-09 10:23:53 UTC) #23
Ryan Sleevi
drive by review; This is mostly style nits, but I am concerned about the GYP ...
8 years, 4 months ago (2012-08-10 04:16:09 UTC) #24
Dai Mikurube (NOT FULLTIME)
Thanks for the comments, Ryan. I was wondering if someone could help about the dirty ...
8 years, 4 months ago (2012-08-10 07:30:03 UTC) #25
Alexander Potapenko
Regarding the _toolset: I don't know if there are any docs, but you should probably ...
8 years, 4 months ago (2012-08-10 08:43:19 UTC) #26
Ryan Sleevi
+cc jamesr, for point #1 in this "summary" Hi Dai, I took a second pass ...
8 years, 4 months ago (2012-08-10 09:38:50 UTC) #27
Dai Mikurube (NOT FULLTIME)
Mainly I'll reply and show my patch update next Monday, but I added one comment. ...
8 years, 4 months ago (2012-08-10 15:51:42 UTC) #28
Dai Mikurube (NOT FULLTIME)
Thank you for the comments. Ok, I summarize my requirements : 1) In compiling *ALL* ...
8 years, 4 months ago (2012-08-13 08:32:14 UTC) #29
Dai Mikurube (NOT FULLTIME)
http://codereview.chromium.org/10411047/diff/69004/third_party/tcmalloc/chromium/src/allocated_type_map.cc File third_party/tcmalloc/chromium/src/allocated_type_map.cc (right): http://codereview.chromium.org/10411047/diff/69004/third_party/tcmalloc/chromium/src/allocated_type_map.cc#newcode51 third_party/tcmalloc/chromium/src/allocated_type_map.cc:51: DCHECK_NE(allocated_type_map, NULL); Sorry, I overlooked it. I'll handle it ...
8 years, 4 months ago (2012-08-13 12:57:41 UTC) #30
Dai Mikurube (NOT FULLTIME)
Updated the patch. Replaced DCHECK_NE with RAW_DCHECK. http://codereview.chromium.org/10411047/diff/69004/third_party/tcmalloc/chromium/src/allocated_type_map.cc File third_party/tcmalloc/chromium/src/allocated_type_map.cc (right): http://codereview.chromium.org/10411047/diff/69004/third_party/tcmalloc/chromium/src/allocated_type_map.cc#newcode51 third_party/tcmalloc/chromium/src/allocated_type_map.cc:51: DCHECK_NE(allocated_type_map, NULL); ...
8 years, 4 months ago (2012-08-14 05:30:48 UTC) #31
Dai Mikurube (NOT FULLTIME)
This change got ready for review again. Could you PTAL this? (Though common.gypi (line 1613-1637) ...
8 years, 4 months ago (2012-08-14 08:23:44 UTC) #32
Ryan Sleevi
On 2012/08/13 08:32:14, Dai Mikurube wrote: > Thank you for the comments. > > Ok, ...
8 years, 4 months ago (2012-08-14 21:21:04 UTC) #33
Dai Mikurube (NOT FULLTIME)
Thank you, Ryan. I learned many things about gyp. :) First of all, I could ...
8 years, 4 months ago (2012-08-15 09:12:04 UTC) #34
Nico
On Wed, Aug 15, 2012 at 2:12 AM, <dmikurube@chromium.org> wrote: > Thank you, Ryan. I ...
8 years, 4 months ago (2012-08-15 17:23:49 UTC) #35
Ryan Sleevi
On Wed, Aug 15, 2012 at 2:12 AM, <dmikurube@chromium.org> wrote: > Thank you, Ryan. I ...
8 years, 4 months ago (2012-08-15 17:59:49 UTC) #36
Alexander Potapenko
Style nit. Looks like more experienced reviewers have been pulled into the review, so I'm ...
8 years, 4 months ago (2012-08-17 11:53:46 UTC) #37
M-A Ruel
http://codereview.chromium.org/10411047/diff/71025/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/71025/base/allocator/allocated_type_profiler.cc#newcode30 base/allocator/allocated_type_profiler.cc:30: return base::allocated_type::g_new_intercept(ptr, size, type); Please consider using a thread ...
8 years, 4 months ago (2012-08-19 02:23:26 UTC) #38
Dai Mikurube (NOT FULLTIME)
Thanks for your comments, Nico, Ryan, Alexander and Marc. Updated the patch set, and replied ...
8 years, 4 months ago (2012-08-20 09:43:56 UTC) #39
M-A Ruel
http://codereview.chromium.org/10411047/diff/71025/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/71025/base/allocator/allocated_type_profiler.cc#newcode30 base/allocator/allocated_type_profiler.cc:30: return base::allocated_type::g_new_intercept(ptr, size, type); On 2012/08/20 09:43:56, Dai Mikurube ...
8 years, 4 months ago (2012-08-20 18:09:04 UTC) #40
Dai Mikurube (NOT FULLTIME)
On 2012/08/20 18:09:04, Marc-Antoine Ruel wrote: > http://codereview.chromium.org/10411047/diff/71025/base/allocator/allocated_type_profiler.cc > File base/allocator/allocated_type_profiler.cc (right): > > http://codereview.chromium.org/10411047/diff/71025/base/allocator/allocated_type_profiler.cc#newcode30 ...
8 years, 4 months ago (2012-08-21 01:02:13 UTC) #41
Dai Mikurube (NOT FULLTIME)
This change is ready for review (maybe good) again except for Marc's point. Could you ...
8 years, 4 months ago (2012-08-21 04:53:53 UTC) #42
Dai Mikurube (NOT FULLTIME)
Added John (jam@) to reviewers for content/app/ change. John, could you take a look at ...
8 years, 4 months ago (2012-08-21 09:01:09 UTC) #43
jam
On 2012/08/21 09:01:09, Dai Mikurube wrote: > Added John (jam@) to reviewers for content/app/ change. ...
8 years, 4 months ago (2012-08-21 16:07:30 UTC) #44
Ryan Sleevi
GYP changes are massively cleaner, so they LGTM. Thanks for addressing the concerns.
8 years, 4 months ago (2012-08-21 19:07:25 UTC) #45
Dai Mikurube (NOT FULLTIME)
John, On 2012/08/21 16:07:30, John Abd-El-Malek wrote: > On 2012/08/21 09:01:09, Dai Mikurube wrote: > ...
8 years, 4 months ago (2012-08-21 22:58:42 UTC) #46
Dai Mikurube (NOT FULLTIME)
In addition, it's s similar way to existing base/allocator/allocator_extension, and its SetGetStatsFunction and SetReleaseFreeMemoryFunction.
8 years, 4 months ago (2012-08-21 23:06:13 UTC) #47
jamesr
On 2012/08/21 23:06:13, Dai Mikurube wrote: > In addition, it's s similar way to existing ...
8 years, 4 months ago (2012-08-21 23:43:23 UTC) #48
Dai Mikurube (NOT FULLTIME)
On 2012/08/21 23:43:23, jamesr wrote: > On 2012/08/21 23:06:13, Dai Mikurube wrote: > > In ...
8 years, 4 months ago (2012-08-22 00:21:12 UTC) #49
M-A Ruel
lgtm with nit http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc#newcode29 base/allocator/allocated_type_profiler.cc:29: if (base::allocated_type::g_new_intercept) { As I said ...
8 years, 4 months ago (2012-08-22 13:38:17 UTC) #50
Ryan Sleevi
On 2012/08/22 00:21:12, Dai Mikurube wrote: > On 2012/08/21 23:43:23, jamesr wrote: > > On ...
8 years, 4 months ago (2012-08-22 22:42:14 UTC) #51
Dai Mikurube (NOT FULLTIME)
Thanks Marc. Updated the patch. Now, ready for review again. I wonder if I could ...
8 years, 4 months ago (2012-08-23 10:30:49 UTC) #52
jar (doing other things)
http://codereview.chromium.org/10411047/diff/98002/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/98002/base/allocator/allocated_type_profiler.cc#newcode26 base/allocator/allocated_type_profiler.cc:26: void* __op_new_intercept__(void* ptr, nit: functions should appear in the ...
8 years, 4 months ago (2012-08-23 19:55:25 UTC) #53
Dai Mikurube (NOT FULLTIME)
Thanks Jim. Updated the patch set. http://codereview.chromium.org/10411047/diff/98002/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/98002/base/allocator/allocated_type_profiler.cc#newcode26 base/allocator/allocated_type_profiler.cc:26: void* __op_new_intercept__(void* ptr, ...
8 years, 4 months ago (2012-08-24 02:47:26 UTC) #54
jar (doing other things)
http://codereview.chromium.org/10411047/diff/98002/base/allocator/allocated_type_profiler_control.cc File base/allocator/allocated_type_profiler_control.cc (right): http://codereview.chromium.org/10411047/diff/98002/base/allocator/allocated_type_profiler_control.cc#newcode12 base/allocator/allocated_type_profiler_control.cc:12: namespace allocated_type { I will remove the courtesy of ...
8 years, 3 months ago (2012-08-27 17:17:12 UTC) #55
Dai Mikurube (NOT FULLTIME)
Hi Jim, Thanks for your review. I just replied your comments, and will update soon. ...
8 years, 3 months ago (2012-08-27 23:52:56 UTC) #56
Dai Mikurube (NOT FULLTIME)
Hi Jim, I fixed the patch as you suggested. And, added tests for allocated_type_profiler and ...
8 years, 3 months ago (2012-08-28 09:04:57 UTC) #57
jar (doing other things)
I just started to review your newest file addition (unittests). It appears I was unclear ...
8 years, 3 months ago (2012-08-29 01:50:01 UTC) #58
Dai Mikurube (NOT FULLTIME)
Hi Jim, Thanks for your comments. I removed "allocated" from all filenames, identifiers and macros. ...
8 years, 3 months ago (2012-08-29 04:49:32 UTC) #59
jar (doing other things)
http://codereview.chromium.org/10411047/diff/115005/base/allocator/type_profiler.cc File base/allocator/type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/115005/base/allocator/type_profiler.cc#newcode22 base/allocator/type_profiler.cc:22: return g_new_intercept != NULL || g_delete_intercept != NULL; Note ...
8 years, 3 months ago (2012-08-30 16:02:09 UTC) #60
Dai Mikurube (NOT FULLTIME)
Thank you, Jim. Updated the patch. http://codereview.chromium.org/10411047/diff/115005/base/allocator/type_profiler.cc File base/allocator/type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/115005/base/allocator/type_profiler.cc#newcode22 base/allocator/type_profiler.cc:22: return g_new_intercept != ...
8 years, 3 months ago (2012-08-31 06:10:30 UTC) #61
jar (doing other things)
Basic readability has gotten much better. Most of the comments are now starting to focus ...
8 years, 3 months ago (2012-09-02 01:37:11 UTC) #62
Dai Mikurube (NOT FULLTIME)
Thank you, Jim. Updated the patch. http://codereview.chromium.org/10411047/diff/115005/base/allocator/type_profiler.cc File base/allocator/type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/115005/base/allocator/type_profiler.cc#newcode22 base/allocator/type_profiler.cc:22: return g_new_intercept != ...
8 years, 3 months ago (2012-09-03 08:16:30 UTC) #63
jar (doing other things)
http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc#newcode29 base/allocator/allocated_type_profiler.cc:29: if (base::allocated_type::g_new_intercept) { I don't think this is simpler, ...
8 years, 3 months ago (2012-09-04 17:40:37 UTC) #64
Dai Mikurube (NOT FULLTIME)
Thanks Jim. Updated. http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc#newcode29 base/allocator/allocated_type_profiler.cc:29: if (base::allocated_type::g_new_intercept) { On 2012/09/04 17:40:38, ...
8 years, 3 months ago (2012-09-05 04:53:23 UTC) #65
Dai Mikurube (NOT FULLTIME)
Added one comment just fyi. http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc#newcode29 base/allocator/allocated_type_profiler.cc:29: if (base::allocated_type::g_new_intercept) { On ...
8 years, 3 months ago (2012-09-05 15:50:42 UTC) #66
jar (doing other things)
http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc#newcode29 base/allocator/allocated_type_profiler.cc:29: if (base::allocated_type::g_new_intercept) { Crashing is indeed the problem we're ...
8 years, 3 months ago (2012-09-06 19:31:42 UTC) #67
Dai Mikurube (NOT FULLTIME)
Thanks, Jim. Updated the patch. http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc File base/allocator/allocated_type_profiler.cc (right): http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_type_profiler.cc#newcode29 base/allocator/allocated_type_profiler.cc:29: if (base::allocated_type::g_new_intercept) { As ...
8 years, 3 months ago (2012-09-07 08:06:45 UTC) #68
jar (doing other things)
http://codereview.chromium.org/10411047/diff/133026/base/allocator/type_profiler.h File base/allocator/type_profiler.h (right): http://codereview.chromium.org/10411047/diff/133026/base/allocator/type_profiler.h#newcode43 base/allocator/type_profiler.h:43: void* __op_new_intercept__(void* ptr, Does the compiler trigger on the ...
8 years, 3 months ago (2012-09-10 16:33:33 UTC) #69
Dai Mikurube (NOT FULLTIME)
Thanks, Jim. I replied for 2 of 3 comments, and will update soon for http://codereview.chromium.org/10411047/diff/128007/base/allocator/type_profiler_unittests.cc#newcode169. ...
8 years, 3 months ago (2012-09-11 00:57:58 UTC) #70
Dai Mikurube (NOT FULLTIME)
Thanks Jim. Succeeded to fixed the change for all your comments! http://codereview.chromium.org/10411047/diff/133026/base/allocator/type_profiler.h File base/allocator/type_profiler.h (right): ...
8 years, 3 months ago (2012-09-11 08:19:28 UTC) #71
jar (doing other things)
Ryan, Can you also comment on this, since you looked at it earlier to help ...
8 years, 3 months ago (2012-09-12 00:34:08 UTC) #72
Dai Mikurube (NOT FULLTIME)
Thanks Jim. Fixed for all your comments (except for the forward declaration issue -- see ...
8 years, 3 months ago (2012-09-12 04:26:27 UTC) #73
Ryan Sleevi
I did not examine the files under tools/ or third_party/. I assume Jim has reviewed ...
8 years, 3 months ago (2012-09-12 19:13:34 UTC) #74
Ryan Sleevi
http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler.h File base/allocator/type_profiler.h (right): http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler.h#newcode26 base/allocator/type_profiler.h:26: On 2012/09/12 19:13:34, Ryan Sleevi wrote: > I think ...
8 years, 3 months ago (2012-09-12 22:04:18 UTC) #75
Dai Mikurube (NOT FULLTIME)
Thanks Ryan. At first, I replied to your comments that don't need fixes. http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler.h File ...
8 years, 3 months ago (2012-09-13 01:12:40 UTC) #76
Dai Mikurube (NOT FULLTIME)
http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler_unittests.cc File base/allocator/type_profiler_unittests.cc (right): http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler_unittests.cc#newcode113 base/allocator/type_profiler_unittests.cc:113: // Call "::operator delete" directly to drop __op_delete_intercept__. On ...
8 years, 3 months ago (2012-09-13 01:29:47 UTC) #77
Ryan Sleevi
http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler.h File base/allocator/type_profiler.h (right): http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler.h#newcode26 base/allocator/type_profiler.h:26: On 2012/09/13 01:12:40, Dai Mikurube wrote: > On 2012/09/12 ...
8 years, 3 months ago (2012-09-13 01:34:59 UTC) #78
Dai Mikurube (NOT FULLTIME)
On 2012/09/13 01:34:59, Ryan Sleevi wrote: > http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler.h > File base/allocator/type_profiler.h (right): > > http://codereview.chromium.org/10411047/diff/142005/base/allocator/type_profiler.h#newcode26 ...
8 years, 3 months ago (2012-09-13 02:02:39 UTC) #79
Dai Mikurube (NOT FULLTIME)
On 2012/09/13 02:02:39, Dai Mikurube wrote: > On 2012/09/13 01:34:59, Ryan Sleevi wrote: > > ...
8 years, 3 months ago (2012-09-13 02:06:37 UTC) #80
Dai Mikurube (NOT FULLTIME)
On 2012/09/13 02:06:37, Dai Mikurube wrote: > On 2012/09/13 02:02:39, Dai Mikurube wrote: > > ...
8 years, 3 months ago (2012-09-13 02:24:30 UTC) #81
Dai Mikurube (NOT FULLTIME)
Hi Ryan, Fixed the patch as you suggested. Thanks. http://codereview.chromium.org/10411047/diff/142005/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10411047/diff/142005/base/allocator/allocator.gyp#newcode573 base/allocator/allocator.gyp:573: ...
8 years, 3 months ago (2012-09-13 04:00:12 UTC) #82
Ryan Sleevi
It would seem like that your changes to tools/deep_memory are unnecessary at this time, or ...
8 years, 3 months ago (2012-09-13 09:13:24 UTC) #83
Dai Mikurube (NOT FULLTIME)
Thanks Ryan. Ok, I won't commit the deep_memory_profiler changes until this is landed. http://codereview.chromium.org/10411047/diff/131013/base/allocator/allocator.gyp File ...
8 years, 3 months ago (2012-09-13 10:45:04 UTC) #84
Dai Mikurube (NOT FULLTIME)
> It would seem like that your changes to tools/deep_memory are unnecessary at > this ...
8 years, 3 months ago (2012-09-14 02:52:20 UTC) #85
Ryan Sleevi
Dai, Thanks for your patience in dealing with our feedback. I believe we're in the ...
8 years, 3 months ago (2012-09-14 23:11:05 UTC) #86
Dai Mikurube (NOT FULLTIME)
Thank you for reviewing, Ryan. Updated the patch, and replied. http://codereview.chromium.org/10411047/diff/131013/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10411047/diff/131013/base/allocator/allocator.gyp#newcode609 ...
8 years, 3 months ago (2012-09-17 03:57:27 UTC) #87
Dai Mikurube (NOT FULLTIME)
WDYT? ping..
8 years, 3 months ago (2012-09-20 22:38:27 UTC) #88
jar (doing other things)
Swamped by perf and other task... will look RSN. Jim On Thu, Sep 20, 2012 ...
8 years, 3 months ago (2012-09-21 00:28:25 UTC) #89
Dai Mikurube (NOT FULLTIME)
Thanks. I was just worried about forgotten... On 2012/09/21 00:28:25, jar wrote: > Swamped by ...
8 years, 3 months ago (2012-09-21 01:03:59 UTC) #90
Dai Mikurube (NOT FULLTIME)
ping, again. just a reminder.
8 years, 2 months ago (2012-09-24 22:54:59 UTC) #91
jam
btw the content part lgtm, I defer to jar on it
8 years, 2 months ago (2012-09-25 21:24:43 UTC) #92
jar (doing other things)
Just about there.... http://codereview.chromium.org/10411047/diff/151001/base/allocator/type_profiler_control.cc File base/allocator/type_profiler_control.cc (right): http://codereview.chromium.org/10411047/diff/151001/base/allocator/type_profiler_control.cc#newcode20 base/allocator/type_profiler_control.cc:20: } nit: // anonymous http://codereview.chromium.org/10411047/diff/151001/base/allocator/type_profiler_map_unittests.cc File ...
8 years, 2 months ago (2012-09-25 22:09:31 UTC) #93
Ryan Sleevi
http://codereview.chromium.org/10411047/diff/151001/base/allocator/type_profiler_map_unittests.cc File base/allocator/type_profiler_map_unittests.cc (right): http://codereview.chromium.org/10411047/diff/151001/base/allocator/type_profiler_map_unittests.cc#newcode96 base/allocator/type_profiler_map_unittests.cc:96: int main(int argc, char** argv) { On 2012/09/25 22:09:31, ...
8 years, 2 months ago (2012-09-25 22:15:06 UTC) #94
Ryan Sleevi
http://codereview.chromium.org/10411047/diff/151001/base/allocator/type_profiler_control.cc File base/allocator/type_profiler_control.cc (right): http://codereview.chromium.org/10411047/diff/151001/base/allocator/type_profiler_control.cc#newcode20 base/allocator/type_profiler_control.cc:20: } On 2012/09/25 22:09:31, jar wrote: > nit: // ...
8 years, 2 months ago (2012-09-25 22:18:41 UTC) #95
jar (doing other things)
OK... with the nit correction made by Ryan (comment // anonymous).... LGTM
8 years, 2 months ago (2012-09-25 22:49:10 UTC) #96
Dai Mikurube (NOT FULLTIME)
Thank you all reviewers for the long discussion to improve this change! Fixed the comment. ...
8 years, 2 months ago (2012-09-26 02:36:14 UTC) #97
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/10411047/153003
8 years, 2 months ago (2012-09-26 02:36:43 UTC) #98
commit-bot: I haz the power
8 years, 2 months ago (2012-09-26 05:17:26 UTC) #99
Change committed as 158752

Powered by Google App Engine
This is Rietveld 408576698