Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1371053002:
[Tracing] Add allocation register for heap profiling (Closed)
|
Created:
5 years, 2 months ago by Ruud van Asseldonk Modified:
5 years, 2 months ago Reviewers:
Primiano Tucci (use gerrit) CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@backtrace Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Tracing] Add allocation register for heap profiling
Malloc and PartitionAlloc dumpers will use this allocation register (a
hash table) to keep track of all outstanding allocations. The hash table
is tailored for this purpose. It handles its own memory management, to
avoid allocation reentrancy issues when doing the bookkeeping. This is
part of the heap profiler in chrome://tracing.
BUG=524631
Committed: https://crrev.com/fd577b4f6c1f24709c274d8970d4f8e6369bb1ff
Cr-Commit-Position: refs/heads/master@{#352664}
Patch Set 1 #
Total comments: 32
Patch Set 2 : Address some primiano comments #Patch Set 3 : Rename fresh_ to next_unused_cell_ #Patch Set 4 : Merge AllocationLessHashTable and AllocationRegister #Patch Set 5 : _linux -> _posix #
Total comments: 46
Patch Set 6 : Try to get to compile on Apple platforms #Patch Set 7 : Address primiano comments + try to fix build on Win/Mac #
Depends on Patchset: Messages
Total messages: 19 (6 generated)
ruuda@google.com changed reviewers: + primiano@chromium.org
Some initial comments, will take a more thorough look once after the first cleanup. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... File base/trace_event/memory_profiler_allocation_register.h (right): https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:17: size_t BASE_EXPORT GetSystemPageSize(); Just use GetPageSize() from base/process_metrics.h ? https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:19: size_t RoundUpToPageSize(size_t size); Just use bits::Align(size, base::GetPageSize()) where you need it https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:26: void* AllocateVirtualMemory(size_t min_size, VirtualMemoryGuard guard); I think these (Alloc+free) should be private static methods of the hash table below. By looking at them seems like you intend to make them visible to clients, which is not really the case. Also, I'd rename these to AllocatePages and FreePages. Otherwise it feels like you are just allocating virtual address space and not backing that by physical memory immediately, which is NOT the case here (and I don't suggest to go there). Also, I'd make the guard implicit and reduce the complexity of this code. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:48: template <typename Value> As you are going to have only one specialization of this, just add a "using value_type = AllocationContext;" https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:49: class AllocationlessHashTable { From [1] "Use the specified order of declarations within a class: public: before private:, methods before data members (variables), etc." [1] https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:52: private: Use a using cell_index_type = uint32_t and the overall code should look cleaner https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:61: const uint32_t table_size_ = 0x40000; I think this should be kNumBuckets (regardless of the name, this should be named kCamelCase) https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:62: const uint32_t table_size_mask_ = 0x3ffff; kBucketsMask = kNumBuckets - 1? Also you might add a static_assert(is power of two) above https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:70: // dozens of mebibytes of address space. Just s/mebibytes/MiB/? https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:71: const uint32_t capacity_ = table_size_ * 10; kNumCells / kNumMaxCells ? https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:93: uint32_t* const indices_; This should really be named "buckets" https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:97: uint32_t free_; free_list_ https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:101: uint32_t fresh_; next_unused_cell_ Also say in the comment "from the |cells_| storage" and mention that this also gives the high water mark https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:160: AllocationlessHashTable() Why this is inline? Move to the .cc same for the dtor https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:284: friend class AllocationRegisterTest; move this under private: https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:295: public: From [1] "Use the specified order of declarations within a class: public: before private:, methods before data members (variables), etc." [1] https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... File base/trace_event/memory_profiler_allocation_register_win.cc (right): https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register_win.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. I'd probably defer introducing this file to once you have a working implementation. You don't want to be debugging on Windows (already), so let's just explicitly not support windows yet.
I addressed most comments but kept the hash table templated for now. I really think that keeping hash table logic and allocation logic separate aids readability. Knowing nothing about |Value| in the hash table simplifies things because there is only one thing you can do, the implementation is obvious. Knowing the concrete type can only add distracting details. On the other hand, doing this does add some boilerplate, and the hash table does make assumptions about its use so it is not really generic. Getting rid of the template is not as simple as using a typedef because of declaration order and access modifiers. I’ll merge |AllocationRegister| and |AllocationlessHashTable| in a different CL and then we can decide which one is cleaner. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... File base/trace_event/memory_profiler_allocation_register.h (right): https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:17: size_t BASE_EXPORT GetSystemPageSize(); On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > Just use GetPageSize() from base/process_metrics.h ? Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:19: size_t RoundUpToPageSize(size_t size); On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > Just use bits::Align(size, base::GetPageSize()) where you need it Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:26: void* AllocateVirtualMemory(size_t min_size, VirtualMemoryGuard guard); On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > I think these (Alloc+free) should be private static methods of the hash table below. > By looking at them seems like you intend to make them visible to clients, which is not really the case. This is not possible if |AllocationlessHashTable| is templated. The functions are not marked |BASE_EXPORT|. > Also, I'd rename these to AllocatePages and FreePages. > Otherwise it feels like you are just allocating virtual address space and not backing that by physical memory immediately, which is NOT the case here (and I don't suggest to go there). For |AllocatePages| it would be really confusing that the argument is in bytes, not the number of pages. Also, this does not back the address space by physical memory, that is up to the OS. (Windows does add to the confusion by having MEM_RESERVE and MEM_COMMIT but that is a Windows implementation detail.) It does really allocate address space, but the access pattern abuses implementation knowledge of the OS to ensure that unused address space is never backed. > Also, I'd make the guard implicit and reduce the complexity of this code. Good idea, done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:52: private: On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > Use a using cell_index_type = uint32_t > and the overall code should look cleaner Great idea, done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:61: const uint32_t table_size_ = 0x40000; On 2015/10/05 at 14:11:06, Primiano Tucci wrote: > I think this should be kNumBuckets (regardless of the name, this should be named kCamelCase) Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:62: const uint32_t table_size_mask_ = 0x3ffff; On 2015/10/05 at 14:11:06, Primiano Tucci wrote: > kBucketsMask = kNumBuckets - 1? > Also you might add a static_assert(is power of two) above Done. Added a comment instead. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:70: // dozens of mebibytes of address space. On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > Just s/mebibytes/MiB/? Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:71: const uint32_t capacity_ = table_size_ * 10; On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > kNumCells / kNumMaxCells ? Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:93: uint32_t* const indices_; On 2015/10/05 at 14:11:06, Primiano Tucci wrote: > This should really be named "buckets" I agree. Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:97: uint32_t free_; On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > free_list_ Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:101: uint32_t fresh_; On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > next_unused_cell_ > Also say in the comment "from the |cells_| storage" and mention that this also gives the high water mark Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:160: AllocationlessHashTable() On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > Why this is inline? Move to the .cc > same for the dtor Because the class is templated. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:284: friend class AllocationRegisterTest; On 2015/10/05 at 14:11:05, Primiano Tucci wrote: > move this under private: Done. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register.h:295: public: On 2015/10/05 at 14:11:06, Primiano Tucci wrote: > From [1] "Use the specified order of declarations within a class: public: before private:, methods before data members (variables), etc." > > [1] https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... Not possible in this case, but I think it can be done if the hash table and allocation register are entangled. https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... File base/trace_event/memory_profiler_allocation_register_win.cc (right): https://codereview.chromium.org/1371053002/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_register_win.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/05 at 14:11:06, Primiano Tucci wrote: > I'd probably defer introducing this file to once you have a working implementation. You don't want to be debugging on Windows (already), so let's just explicitly not support windows yet. I ran it on the Windows trybots to debug and it works. What is the alternative, #ifdefing all the code to only run on Linux? (I do have to change _linux to _posix and get it to build on the other platforms as well.)
After merging |AllocationlessHashTable| and |AllocationRegister| I am convinced that the merged version is cleaner. Nit ahead.
Looks beautiful to me. Thanks for all the efforts and for standing my whims, but I think the final result if really worth it. This is not perfectly readable and just as complex as needed. I have just some final nits, but they are just comments and renames, really nothing major. LGTM which much excitement! https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... File base/trace_event/memory_profiler_allocation_register.cc (right): https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:20: free_list_ = 0; why don't you inizialize also these in the ctor? I think that the clang-checker will complain that free_list_ is not initialized (not sure if the plugin is smart enough to look in the ctor body) https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:49: CellIndex* idx_ptr = Lookup(address); I'd probably move the comment that says that idx_ptr is the pointer to either the bucket or the next member here, as this is the place where I would have really needed that comment https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:57: CellIndex free_idx = *idx_ptr; move this up before the if, and use if(free_idx == 0) in the if https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:58: *idx_ptr = cells_[free_idx].next; I this this code below would be more readable if you add an extra variable: Cell* freed_cell = &cells_[free_idx]; and use freed_cell below Also note freed vs free, IMHO that conveys more the information about "this is the cell/index being freed" and avoids the ambiguity of "was this free already before the remove or happened to become free after the the remove?" https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:72: ++iterator; Why don't you just construct the iterator passing 1 above? In essence, why this is not just a return ConstIterator(*this, 1) ? in which case could be inlined in the header as becomes trivial https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:91: do I think you still need {braces} as this is a multiline while (i.e. a similar for() loop would have been similarly {braced}) https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:110: // recorded from a Chrome trace run). It is the first prime after 2^17. For This is worth the entire CL https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:123: CellIndex* idx_ptr = buckets_ + Hash(address); IMHO probably slightly more readable if you: CellIndex* idx_ptr = &buckets_[Hash(address)] https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:156: return idx; I'd probably add a DCHECK here, so in debug builds we can figure out the issue without debugging a more cryptic SEGFAULT later https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... File base/trace_event/memory_profiler_allocation_register.h (right): https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:17: // freed. It is an mmap-backed hash table that stores size and context indexed nit: s/an mmap/a mmap/ https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:29: // An entry in the hash table; contains the details about an allocation. Hmm not really (w.r.t. comment). The real entry here is Cell. Allocation is the public-facing information stored https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:60: // Inserts the address, size and context into the table. If the address was I'd just say "Inserts an allocation and its metadata into the table" so you don't have to keep the comment updated if you add more stuff https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:82: // the list terminator. Address, size and context are not stored directly but I'd remove the latter sentence, in the beginning it was a bit confusing. For a moment I though that address, size and context were not part of the memory record of Cell, which is not true. You are just saying taht you are encapsulating, but IMHO there is no actual need for a comment. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:91: uint32_t Hash(void* address); shouldn't this be a static method? https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:99: // Returns the index of a cell that is not being used to store an entry. Probably want to make this comment a bit stronger to highlight the fact that this operation is not idempotent and every time carves out a new cell. Something like "Acquires a new cell, either from the freelist or bumping up the water-mark, and returns its index" https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:105: static void* AllocateVirtualMemory(size_t min_size); plz move static methods before other non-static methods https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:105: static void* AllocateVirtualMemory(size_t min_size); I'd just call the 2nd method size, just because then FreeVirtualMemory's signature becomes clearer. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:120: const uint32_t kNumBuckets = 0x40000; move these constants up to line 93. From https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... Within each section, the declarations generally should be in the following order: Typedefs and Enums Constants (static const data members) Constructors Destructor Methods, including static methods Data Members (except static const data members) https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... File base/trace_event/memory_profiler_allocation_register_posix.cc (right): https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_posix.cc:26: MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); I think you need some ifdef guard here, IIRC MAP_ANONYMOUS is not posix and mac will be unhappy https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_posix.cc:33: void* guard_addr = static_cast<void*>(static_cast<uint8_t*>(addr) + size); s/uint8_t/uintptr_t/ (and use reinterpret_cast). afaik that is the one you want to do pointer arith with void* P.S. Let me guess: you did that but preferred to use static_Cast because reinterpret_cast wouldn't fit in one line, eh? :P https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_posix.cc:34: size_t guard_size = map_size - size; probably the code here would be a bit more readable if you: make guard_size a constatnt in the anonymous namespace: size_t GetGuardSize() { return GetPageSize(); } (Ideally it would be a const but you cannot write const kGuardSize = GetPageSize() as it would cause a static initializer and our checkers will barf) then say const size_t map_size = size + GetGuardSize() and use GetGuardSize() below https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... File base/trace_event/memory_profiler_allocation_register_unittest.cc (right): https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_unittest.cc:21: uint32_t GetTableSize(const AllocationRegister& reg) { Uh? Aren't this static constants. How is this different from AllocationRegister::kNumBuckets? If this is just for getting around the protection (AllocationRegister is friend, the TEST_F are not) just re-define them using "using" in this class and everything should work. I mean: class ALlocationRegisterTest ... { using kNumBuckets = AllocationRegister::kNumBuckets; .. } TEST_F(ALlocationRegisterTest, Foo) { bla = kNumBuckets; } https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_unittest.cc:25: uint32_t GetNumCells(const AllocationRegister& reg) { return reg.kNumCells; } ditto here https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_unittest.cc:206: TEST_F(AllocationRegisterTest, OverflowDeathTest) { Hmm how long does it take to run this test? Having a test which fills 2.6 Mln objects might take non negligible time and the benefit here would not be worth the time spent for running the test. If this takes more than a bunch of hundreds ms it's probably not worth it. Also, IIRC death test require #ifdef guards, are not supported in all platforms, see codesearch. I guess they are more for checking security invariants.
Looks beautiful to me. Thanks for all the efforts and for standing my whims, but I think the final result if really worth it. This is not perfectly readable and just as complex as needed. I have just some final nits, but they are just comments and renames, really nothing major. LGTM which much excitement!
On 2015/10/06 13:34:46, Primiano Tucci wrote: > Looks beautiful to me. Thanks for all the efforts and for standing my whims, but > I think the final result if really worth it. This is not perfectly readable and > just as complex as needed. > I have just some final nits, but they are just comments and renames, really > nothing major. > > LGTM which much excitement! Ehm I think I managed to click twice on the submit button. Note my comments on #6
https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... File base/trace_event/memory_profiler_allocation_register.cc (right): https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:20: free_list_ = 0; On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > why don't you inizialize also these in the ctor? > I think that the clang-checker will complain that free_list_ is not initialized (not sure if the plugin is smart enough to look in the ctor body) Done. The checker did not complain, |free_list_| was initialized perfectly well in the constructor and all members were initialized in declaration order. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:49: CellIndex* idx_ptr = Lookup(address); On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > I'd probably move the comment that says that idx_ptr is the pointer to either the bucket or the next member here, as this is the place where I would have really needed that comment Done, though it is not actually relevant where the index is stored. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:57: CellIndex free_idx = *idx_ptr; On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > move this up before the if, and use if(free_idx == 0) in the if As you wish. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:58: *idx_ptr = cells_[free_idx].next; On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > I this this code below would be more readable if you add an extra variable: > > Cell* freed_cell = &cells_[free_idx]; > and use freed_cell below > > Also note freed vs free, IMHO that conveys more the information about "this is the cell/index being freed" and avoids the ambiguity of "was this free already before the remove or happened to become free after the the remove?" Done. Yeah freeing is not atomic so I think it is impossible to be perfectly accurate with the names. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:72: ++iterator; On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > Why don't you just construct the iterator passing 1 above? > In essence, why this is not just a return ConstIterator(*this, 1) ? > in which case could be inlined in the header as becomes trivial Because cell 1 might be free. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:91: do On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > I think you still need {braces} as this is a multiline while (i.e. a similar for() loop would have been similarly {braced}) As you wish. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:123: CellIndex* idx_ptr = buckets_ + Hash(address); On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > IMHO probably slightly more readable if you: > CellIndex* idx_ptr = &buckets_[Hash(address)] As you wish. Fun fact: as |&buckets_[Hash(address)]| desugars to |buckets_ + Hash(address)| and + is commutative, you can also write |&Hash(address)[buckets_]|. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.cc:156: return idx; On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > I'd probably add a DCHECK here, so in debug builds we can figure out the issue without debugging a more cryptic SEGFAULT later Done. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... File base/trace_event/memory_profiler_allocation_register.h (right): https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:17: // freed. It is an mmap-backed hash table that stores size and context indexed On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > nit: s/an mmap/a mmap/ Changed to “a memory map-backed” to avoid all ambiguity. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:29: // An entry in the hash table; contains the details about an allocation. On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > Hmm not really (w.r.t. comment). The real entry here is Cell. Allocation is the public-facing information stored To me, an entry is the thing you put in / get out, |Cell| is an implementation detail. Changed the comment. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:60: // Inserts the address, size and context into the table. If the address was On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > I'd just say "Inserts an allocation and its metadata into the table" > so you don't have to keep the comment updated if you add more stuff Done. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:82: // the list terminator. Address, size and context are not stored directly but On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > I'd remove the latter sentence, in the beginning it was a bit confusing. > For a moment I though that address, size and context were not part of the memory record of Cell, which is not true. You are just saying taht you are encapsulating, but IMHO there is no actual need for a comment. Done. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:91: uint32_t Hash(void* address); On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > shouldn't this be a static method? It can be, yes. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:99: // Returns the index of a cell that is not being used to store an entry. On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > Probably want to make this comment a bit stronger to highlight the fact that this operation is not idempotent and every time carves out a new cell. Something like "Acquires a new cell, either from the freelist or bumping up the water-mark, and returns its index" Done. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:105: static void* AllocateVirtualMemory(size_t min_size); On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > plz move static methods before other non-static methods Done. At least I grouped all the static methods together this time ... > I'd just call the 2nd method size, just because then FreeVirtualMemory's signature becomes clearer. Done. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register.h:120: const uint32_t kNumBuckets = 0x40000; On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > move these constants up to line 93. From https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... > > Within each section, the declarations generally should be in the following order: > > Typedefs and Enums > Constants (static const data members) > Constructors > Destructor > Methods, including static methods > Data Members (except static const data members) Done. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... File base/trace_event/memory_profiler_allocation_register_posix.cc (right): https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_posix.cc:26: MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > I think you need some ifdef guard here, IIRC MAP_ANONYMOUS is not posix and mac will be unhappy Yep, found that out via the trybots. PartitionAlloc solved it by #defining it to MAP_ANON if not defined, so did that here as well (see patchset 6). https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_posix.cc:33: void* guard_addr = static_cast<void*>(static_cast<uint8_t*>(addr) + size); On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > s/uint8_t/uintptr_t/ (and use reinterpret_cast). > afaik that is the one you want to do pointer arith with void* > > P.S. Let me guess: you did that but preferred to use static_Cast because reinterpret_cast wouldn't fit in one line, eh? :P Done. I casted to |uint8_t| because this is really counting bytes, and |uint8_t| is a byte. My OCD was unhappier about the |AllocationRegister| constructor (the comments don’t line up, and moving the colon before the comment makes it even worse ...) https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_posix.cc:34: size_t guard_size = map_size - size; On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > probably the code here would be a bit more readable if you: > make guard_size a constatnt in the anonymous namespace: > size_t GetGuardSize() { return GetPageSize(); } > (Ideally it would be a const but you cannot write const kGuardSize = GetPageSize() as it would cause a static initializer and our checkers will barf) > > then say > const size_t map_size = size + GetGuardSize() > > and use GetGuardSize() below As you wish. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... File base/trace_event/memory_profiler_allocation_register_unittest.cc (right): https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_unittest.cc:21: uint32_t GetTableSize(const AllocationRegister& reg) { On 2015/10/06 at 13:34:45, Primiano Tucci wrote: > Uh? Aren't this static constants. How is this different from AllocationRegister::kNumBuckets? > > If this is just for getting around the protection (AllocationRegister is friend, the TEST_F are not) just re-define them using "using" in this class and everything should work. > > I mean: > class ALlocationRegisterTest ... { > using kNumBuckets = AllocationRegister::kNumBuckets; > > .. > } > > TEST_F(ALlocationRegisterTest, Foo) > { > bla = kNumBuckets; > } Done. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_unittest.cc:25: uint32_t GetNumCells(const AllocationRegister& reg) { return reg.kNumCells; } On 2015/10/06 at 13:34:45, Primiano Tucci wrote: > ditto here Done. https://codereview.chromium.org/1371053002/diff/80001/base/trace_event/memory... base/trace_event/memory_profiler_allocation_register_unittest.cc:206: TEST_F(AllocationRegisterTest, OverflowDeathTest) { On 2015/10/06 at 13:34:44, Primiano Tucci wrote: > Hmm how long does it take to run this test? > Having a test which fills 2.6 Mln objects might take non negligible time and the benefit here would not be worth the time spent for running the test. > If this takes more than a bunch of hundreds ms it's probably not worth it. It is pretty heavy (~400 ms on my machine), but I did want to add it to verify that the guard page works. Perhaps we can keep it as a disabled test? > Also, IIRC death test require #ifdef guards, are not supported in all platforms, see codesearch. I guess they are more for checking security invariants. Thought I fixed that in patchset 6, but then -Werror refused to compile due to unused variables. Should be fixed now.
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1371053002/#ps120001 (title: "Address primiano comments + try to fix build on Win/Mac")
The CQ bit was unchecked by ruuda@google.com
The CQ bit was checked by ruuda@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1371053002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1371053002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fd577b4f6c1f24709c274d8970d4f8e6369bb1ff Cr-Commit-Position: refs/heads/master@{#352664}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1387483006/ by kuan@chromium.org. The reason for reverting is: broke builds: https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg... https://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20%28dbg%29/b....
Message was sent while issue was closed.
On 2015/10/06 21:02:19, kuan wrote: > A revert of this CL (patchset #7 id:120001) has been created in > https://codereview.chromium.org/1387483006/ by mailto:kuan@chromium.org. > > The reason for reverting is: broke builds: > > https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg... > > https://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20%28dbg%29/b.... Bummer. NaCl build again (I wonder why there is no coverage on the CQ, filed crbug.com/540566) I think you want to exclude the posix .cc files when: is_nacl in GN, >(nacl_untrusted_build)==1
Message was sent while issue was closed.
Patchset #8 (id:140001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
