Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2089253002:
[tracing] Optimize AllocationRegister and increase max backtrace depth. (Closed)
|
Created:
4 years, 6 months ago by Dmitry Skiba Modified:
4 years, 5 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Optimize AllocationRegister and increase max backtrace depth.
This CL both increases maximum backtrace depth (24 -> 48) and reduces
AllocationRegister memory usage (265 -> 42 MiB on 32-bit platforms) at
the expense of performance (~30% hit). See the bug for more details.
BUG=617772
Committed: https://crrev.com/557f4aad30e98c3c5434ea37ca061fd3b658d570
Cr-Commit-Position: refs/heads/master@{#403291}
Patch Set 1 #Patch Set 2 : Remove old backtrace when rewriting allocation info #Patch Set 3 : Cleanup tests #
Total comments: 71
Patch Set 4 : Address comments #
Total comments: 34
Patch Set 5 : Fix (stupid) errors #Patch Set 6 : Rename things #Patch Set 7 : Fix Windows #
Messages
Total messages: 23 (7 generated)
dskiba@google.com changed reviewers: + primiano@chromium.org
Please review. Tests are TBD, right now we have AllocationRegister tests, but I'll extract FixedHashMap tests from there, to cover all cases.
On 2016/06/22 20:50:22, Dmitry Skiba wrote: > Please review. Tests are TBD, right now we have AllocationRegister tests, but > I'll extract FixedHashMap tests from there, to cover all cases. Actually, the tests are fine. I've attempted to extract FixedHashMap tests, but since we still need AllocationRegister ones, I ended up with two sets of almost identical tests (except for Insert() arguments). So instead I've just cleaned them up a bit.
Ok did a first big pass on this. Will do a 2nd one tomorrow (didn't look at the test at all). I like this a lot, also the design is pretty elegant. I have really minor comments. Thanks a lot for doing this! https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.cc (right): https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.cc:65: // its Backtrace hash map on every Insert() call. Surprisingly, Knuth's Not sure I'm that "surprised" :) https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.cc:67: const uintptr_t kKnuthConstant = 2654435761; s/2654435761/2654435761u/ (trailing u) I never remember what to expect otherwise on 32-bit for this that wraps over 2^31 https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:35: template <class Key, class Value, class KeyHasher> Do you really need the KeyHasher arg here? I think you can just default to std::hash<Key> and require that the client ovverides that. Not sure we need to match the hipster STL template format :) https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:38: using KVPair = std::pair<const Key, Value>; Include What You Use: add #include to <utility> up https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:40: // For implementation simplicity API uses integer index instead /me loves simplicity! \o/ https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:43: using KVIndex = size_t; should we just use size_T everywhere here? I feel KVIndex just add an extra readability layer without adding too much benefit. Also, since you need < 4M entries we could keep this a uint32_t. This would reduce the amount of memory required in AllocationInfo https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:63: RemoveAll(); do you really need to call RemoveAll here? Once you free the memory the only thing you are left with is PODs. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:72: return {KVIndex(cell - cells_), false}; // not inserted shouldn't this be a static_cast<KVIndex>(cell - cells_) ? https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:81: new (&cell->kv) KVPair(key, value); why this is not just: cell->kv.first = key cell->kv.second = value? the placement ctor adds a shade of mystery around here. Or is there something why we need this I am missing? https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:83: return {KVIndex(cell - cells_), true}; // inserted sitto here about static_cast https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:87: if (index == kInvalidKVIndex) { should this be if (index >= num_cells_)? https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:108: void RemoveAll() { Looks like we never end up calling this. Should we rip this alltogether? Once you have unallocated the storage in the dtor you should be fine here (if you want to be paranoid reset also the freelist and , cells_ and buckets_ in the dtor to detect UAF). https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:116: KVIndex Find(const Key& key) const { Not sure why you have this returning an index at all. BY looking at your code it looks like that in the only two cases where you use find you first return an index, but then do a Get() to return the actuall KVPair. Unless I am missing osmething I thing that you can get rid of both Find and Get as they are and you really want a V* Find(const Key& key) This should remove the need of all a bunch of .second below You don't seem to need the actual std::pair really https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:133: KVIndex FindValidIndex(KVIndex index) const { s/FindValidIndex/FindNextValidCellIndex/ (or just FindNextValidCell) https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:143: size_t EstimateAllocatedDirty() const { maybe EstimateResidentMemory or EstimateDirtyMemory or EstimateUsedMemory... I mean, put Memory in the name :) https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:155: KVPair kv; I think if you put kv at the end you might get some better packing as you're not forcing the 8-bytes alignment( on 64 bit) between kv and next. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:158: // Conceptually this is |prev| in a double linked list. However, buckets s/double/doubly/ https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:167: Cell** pself; I agree with all the reasoning here. Just from a naming perspective shouldn't this be called p_prev ? It is still logically kind-of-a-prev-pointer https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:170: using Bucket = Cell*; It seems you use this only in one place below, I'd remove this. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:177: Cell** pcell = &buckets_[Hash(key)]; I think that p_cell is more readable (here and elsewhere) https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:182: Cell* cell = *pcell; This line IMHO just makes the loop more complicate to follow. I'd just ditch it and below do if ((*p_cell)->kv.first == key) https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:224: return KeyHasher()(key) % num_buckets_; tip: if we enforced that num_buckets was a power of two, this could be just eyHasher()(key) & (num_buckets - 1). Also if num_buckets is a template argument this would inline into a pretty fast operation. I guess a general mod operation can be quite slower in comparison. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:228: size_t const num_buckets_; I wonder if this (and num_cells_) should instead be template arguments? Not a huge deal, up to you. Maybe we can get some extra optimization? https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:234: // are accessed first, higher indices are only accessed when required. In s/when required/only when the |free_list_| is empty/ https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:235: // this way, even if a huge amount of address space has been mmapped, only I'd just say, instead of "In this way..." -> "This is to minimize the amount of resident memory used." https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:240: // contain the index of the head of the linked list for |Hash(key)|. does it really contain the index? This seems to be a pointer to it. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:242: mutable Bucket* buckets_; why is this mutable? https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:242: mutable Bucket* buckets_; I think this is more readable as: Cell*[] buckets_; (unless it causes some weird compilation issue in which case nvm) Because this is what buckets_ is, an array of cell pointers. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:252: DISALLOW_COPY_AND_ASSIGN(FixedHashMap); I think you need the template arguments here? https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:291: explicit AllocationRegister(size_t num_allocation_cells, no need for explicit if you have two args here https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:307: bool Get(const void* address, Allocation* out_allocation) const; out of curiosity what was the issue with returning nullptr? https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:324: static const size_t kNumBacktraceBuckets = 30011; // prime nit: add extra space before comment Maybe the pow2 avoids the % operation https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:329: size_t, add a comment explaining that this size_t is an index to cell (is it?) Also why this is not just a pointer to the cell instead of the index?
https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.cc (right): https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.cc:67: const uintptr_t kKnuthConstant = 2654435761; On 2016/06/23 20:46:24, Primiano Tucci wrote: > s/2654435761/2654435761u/ (trailing u) > I never remember what to expect otherwise on 32-bit for this that wraps over > 2^31 Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:35: template <class Key, class Value, class KeyHasher> On 2016/06/23 20:46:25, Primiano Tucci wrote: > Do you really need the KeyHasher arg here? > I think you can just default to std::hash<Key> and require that the client > ovverides that. Not sure we need to match the hipster STL template format :) This is for AddressHasher, which implements fast hashing (that previously was AllocationRegister::Hash function). https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:38: using KVPair = std::pair<const Key, Value>; On 2016/06/23 20:46:25, Primiano Tucci wrote: > Include What You Use: add #include to <utility> up Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:43: using KVIndex = size_t; On 2016/06/23 20:46:26, Primiano Tucci wrote: > should we just use size_T everywhere here? > I feel KVIndex just add an extra readability layer without adding too much > benefit. > Also, since you need < 4M entries we could keep this a uint32_t. This would > reduce the amount of memory required in AllocationInfo Yes, functionally KVIndex is useless, but it helps to distinguish index from more general meaning of "size of memory region". https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:63: RemoveAll(); On 2016/06/23 20:46:25, Primiano Tucci wrote: > do you really need to call RemoveAll here? Once you free the memory the only > thing you are left with is PODs. Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:72: return {KVIndex(cell - cells_), false}; // not inserted On 2016/06/23 20:46:25, Primiano Tucci wrote: > shouldn't this be a static_cast<KVIndex>(cell - cells_) ? Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:81: new (&cell->kv) KVPair(key, value); On 2016/06/23 20:46:26, Primiano Tucci wrote: > why this is not just: > cell->kv.first = key > cell->kv.second = value? > > the placement ctor adds a shade of mystery around here. > Or is there something why we need this I am missing? Actually, we can't do kv.first = key, because first is 'const Key'. So we need placement new here. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:83: return {KVIndex(cell - cells_), true}; // inserted On 2016/06/23 20:46:25, Primiano Tucci wrote: > sitto here about static_cast Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:87: if (index == kInvalidKVIndex) { On 2016/06/23 20:46:25, Primiano Tucci wrote: > should this be if (index >= num_cells_)? Right, and I'll also convert that to DCHECK, because returning just doesn't seem right. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:108: void RemoveAll() { On 2016/06/23 20:46:24, Primiano Tucci wrote: > Looks like we never end up calling this. Should we rip this alltogether? > Once you have unallocated the storage in the dtor you should be fine here (if > you want to be paranoid reset also the freelist and , cells_ and buckets_ in the > dtor to detect UAF). Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:116: KVIndex Find(const Key& key) const { On 2016/06/23 20:46:25, Primiano Tucci wrote: > Not sure why you have this returning an index at all. BY looking at your code it > looks like that in the only two cases where you use find you first return an > index, but then do a Get() to return the actuall KVPair. > > Unless I am missing osmething I thing that you can get rid of both Find and Get > as they are and you really want a > V* Find(const Key& key) > > This should remove the need of all a bunch of .second below > You don't seem to need the actual std::pair really I need index for backtrace_index, i.e. I need something that I can use to get an item / remove it, both in O(1) time. And BTW I need both key and value, because for example in BacktraceMap Backtrace is actually a key. For AllocationMap address is a key, and I need it too. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:133: KVIndex FindValidIndex(KVIndex index) const { On 2016/06/23 20:46:25, Primiano Tucci wrote: > s/FindValidIndex/FindNextValidCellIndex/ (or just FindNextValidCell) The thing with 'Cell' is that it doesn't appear anywhere in the API. I.e. there are Get/Find/Insert/Remove and then suddenly a method that deals with 'Cell'. But 'Valid' is also wrong, because by definition KVIndex has just single invalid value. I think FindNextIndex() is a better name for this - it describes what happens (finding the next index) and doesn't introduce any new (Cell) or wrong (Valid) concepts. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:143: size_t EstimateAllocatedDirty() const { On 2016/06/23 20:46:24, Primiano Tucci wrote: > maybe EstimateResidentMemory or EstimateDirtyMemory or EstimateUsedMemory... I > mean, put Memory in the name :) Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:155: KVPair kv; On 2016/06/23 20:46:25, Primiano Tucci wrote: > I think if you put kv at the end you might get some better packing as you're not > forcing the 8-bytes alignment( on 64 bit) between kv and next. Hmm, but both 'next' and 'p_prev' are pointers, so either there is a padding between 'kv' and 'next', or, if 'kv' is moved to the end, the whole structure will be padded to satisfy 'next' alignment of 8. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:158: // Conceptually this is |prev| in a double linked list. However, buckets On 2016/06/23 20:46:26, Primiano Tucci wrote: > s/double/doubly/ Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:167: Cell** pself; On 2016/06/23 20:46:25, Primiano Tucci wrote: > I agree with all the reasoning here. Just from a naming perspective shouldn't > this be called p_prev ? It is still logically kind-of-a-prev-pointer Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:170: using Bucket = Cell*; On 2016/06/23 20:46:25, Primiano Tucci wrote: > It seems you use this only in one place below, I'd remove this. Actually I added it for ctor/dtor - without it static_cast<Bucket*>(AllocateGuardedVirtualMemory(num_buckets_ * sizeof(Bucket)))), becomes more convoluted static_cast<Cell**>(AllocateGuardedVirtualMemory(num_buckets_ * sizeof(Cell*)))), And generally, it adds nice symmetry: Cell <-> num_cells_, cells_, Bucket <-> num_buckets_, buckets_ https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:177: Cell** pcell = &buckets_[Hash(key)]; On 2016/06/23 20:46:25, Primiano Tucci wrote: > I think that p_cell is more readable (here and elsewhere) Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:182: Cell* cell = *pcell; On 2016/06/23 20:46:24, Primiano Tucci wrote: > This line IMHO just makes the loop more complicate to follow. I'd just ditch it > and below do > if ((*p_cell)->kv.first == key) Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:224: return KeyHasher()(key) % num_buckets_; On 2016/06/23 20:46:25, Primiano Tucci wrote: > tip: if we enforced that num_buckets was a power of two, this could be just > eyHasher()(key) & (num_buckets - 1). > Also if num_buckets is a template argument this would inline into a pretty fast > operation. I guess a general mod operation can be quite slower in comparison. Done. However, it required change to the backtrace hash function, which became so specialized that I moved it inside the class. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:228: size_t const num_buckets_; On 2016/06/23 20:46:25, Primiano Tucci wrote: > I wonder if this (and num_cells_) should instead be template arguments? Not a > huge deal, up to you. Maybe we can get some extra optimization? Hmm, unlike number of buckets number of cells is not used much, so we won't gain much. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:234: // are accessed first, higher indices are only accessed when required. In On 2016/06/23 20:46:24, Primiano Tucci wrote: > s/when required/only when the |free_list_| is empty/ Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:235: // this way, even if a huge amount of address space has been mmapped, only On 2016/06/23 20:46:25, Primiano Tucci wrote: > I'd just say, instead of "In this way..." -> "This is to minimize the amount of > resident memory used." Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:240: // contain the index of the head of the linked list for |Hash(key)|. On 2016/06/23 20:46:25, Primiano Tucci wrote: > does it really contain the index? This seems to be a pointer to it. Right, this is a leftover from the old implementation. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:242: mutable Bucket* buckets_; On 2016/06/23 20:46:25, Primiano Tucci wrote: > why is this mutable? Find() is const, and is uses Lookup() which returns Cell**, so Lookup() should also be const, but it won't be able to return Cell** unless buckets_ is mutable. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:252: DISALLOW_COPY_AND_ASSIGN(FixedHashMap); On 2016/06/23 20:46:26, Primiano Tucci wrote: > I think you need the template arguments here? Yeah, you can just use class name in templates, and it means "class name and all template arguments". Here is some quote from the standard I found on the SO: 14.6.1/1: "Like normal (non-template) classes, class templates have an injected-class-name (Clause 9). The injected-class-name [...] is equivalent to the template-name followed by the template-parameters of the class template enclosed in <>." https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:291: explicit AllocationRegister(size_t num_allocation_cells, On 2016/06/23 20:46:26, Primiano Tucci wrote: > no need for explicit if you have two args here Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:307: bool Get(const void* address, Allocation* out_allocation) const; On 2016/06/23 20:46:25, Primiano Tucci wrote: > out of curiosity what was the issue with returning nullptr? I simply can't :) The issue is that previously 'Allocation' was the value stored in the AllocationRegister, but now it's split between several maps. So to limit number of changes I'm faking the old API and returning Allocation by value (see ConstIterator for example). https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:324: static const size_t kNumBacktraceBuckets = 30011; // prime On 2016/06/23 20:46:26, Primiano Tucci wrote: > nit: add extra space before comment > Maybe the pow2 avoids the % operation Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:329: size_t, On 2016/06/23 20:46:26, Primiano Tucci wrote: > add a comment explaining that this size_t is an index to cell (is it?) > Also why this is not just a pointer to the cell instead of the index? Yeah, it desperately needs a comment, because actually that is a number of references to a backtrace :)
OK LGTM with some final comments, plz look at them. Maybe have ssid or Maria have a final pass on this, since this is a biggish CL there is the non-zero probability that some silly bound / nullptr bug sneaked in :) https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:35: template <class Key, class Value, class KeyHasher> On 2016/06/28 10:55:00, Dmitry Skiba wrote: > On 2016/06/23 20:46:25, Primiano Tucci wrote: > > Do you really need the KeyHasher arg here? > > I think you can just default to std::hash<Key> and require that the client > > ovverides that. Not sure we need to match the hipster STL template format :) > > This is for AddressHasher, which implements fast hashing (that previously was > AllocationRegister::Hash function). Ahh Ok I see the point now. Address is a simple type and you cannot re-define std::hash<void*>. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:43: using KVIndex = size_t; On 2016/06/28 10:54:59, Dmitry Skiba wrote: > On 2016/06/23 20:46:26, Primiano Tucci wrote: > > should we just use size_T everywhere here? > > I feel KVIndex just add an extra readability layer without adding too much > > benefit. > > Also, since you need < 4M entries we could keep this a uint32_t. This would > > reduce the amount of memory required in AllocationInfo > > Yes, functionally KVIndex is useless, but it helps to distinguish index from > more general meaning of "size of memory region". Ok I see, makes sense. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:133: KVIndex FindValidIndex(KVIndex index) const { On 2016/06/28 10:55:00, Dmitry Skiba wrote: > On 2016/06/23 20:46:25, Primiano Tucci wrote: > > s/FindValidIndex/FindNextValidCellIndex/ (or just FindNextValidCell) > > The thing with 'Cell' is that it doesn't appear anywhere in the API. I.e. there > are Get/Find/Insert/Remove and then suddenly a method that deals with 'Cell'. > But 'Valid' is also wrong, because by definition KVIndex has just single invalid > value. Oh right realized only now. I thought this was a private method, is not. shouldn't this be private? > I think FindNextIndex() is a better name for this - it describes what happens > (finding the next index) and doesn't introduce any new (Cell) or wrong (Valid) > concepts. At this point just Next(index). If you call it FindNextIndex sounds like you are looking for an index, at which point I'd ask "why you don't return just index+1"? The reason is that what you mean with FindNextIndex is FindNextAndReturnItsIndex, which is an awkward name. So, since everything is index-based by design, let's go for just FindNext. But then, why the Find part at and not just Next(index) at this point? At least that recalls the next() operator in python and std::next which are logically the same. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:155: KVPair kv; On 2016/06/28 10:54:59, Dmitry Skiba wrote: > On 2016/06/23 20:46:25, Primiano Tucci wrote: > > I think if you put kv at the end you might get some better packing as you're > not > > forcing the 8-bytes alignment( on 64 bit) between kv and next. > > Hmm, but both 'next' and 'p_prev' are pointers, so either there is a padding > between 'kv' and 'next', or, if 'kv' is moved to the end, the whole structure > will be padded to satisfy 'next' alignment of 8. Right silly comment from my side, needs to be aligned to pointer size anyways because of the pointer fields. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:242: mutable Bucket* buckets_; On 2016/06/28 10:54:59, Dmitry Skiba wrote: > On 2016/06/23 20:46:25, Primiano Tucci wrote: > > why is this mutable? > > Find() is const, and is uses Lookup() which returns Cell**, so Lookup() should > also be const, but it won't be able to return Cell** unless buckets_ is mutable. I see, isn't the right thing to do having Lookup retuning a const Cell ptr and use const_cast in Insert to amend the constness of the access path ? https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.cc (right): https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:18: index_ = register_.allocations_.FindNextIndex(index_ + 1); shouldn't the +1 be part of the FindNext function, as there is "Next" in the name? https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:53: return (total_value * 131101) >> 14; Not sure I follow the comment when you say "constants from AddressHasher". I'd probably just say "this magic constatnt was chosen empirically on real traces yada yada ...." https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:65: const uintptr_t a = 131101; we should call this "the Ruud constant" :) https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:43: "Key and Value shouldn't have destructors"); Excellent, TIL std::is_trivially_destructible. I thought that we needed this check while reviewing this but didn't know there was a way to enforce it. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:45: using KVPair = std::pair<const Key, Value>; to be honest I think that this "const" here is causing you a bit of awkwardness around to maintain const correctness. If this was non-const you could avoid the placement new. The reality here is that the keys are not really const because you constantly need to initialize and re-initialize the cells. I understand that you want to just prevent the backtrace to be touched once initialized, but I feel this is causing your code to become unnecessarily unreadable. If you really want const correctness, make Cell have a std::pair<K,V> (Without const) add an Initialize(const KVPair&) method and use const_cast there. But that is probably overkill as well. So I'd personally just drop the const to keep it simple. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:77: return {static_cast<KVIndex>(cell - cells_), false}; // not inserted nit: add extra space before // https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:85: // Initialize key/value pair. Since key is 'const Key' this is the right your comment here is a hint to my comment above (about removing const). IMHO placement new makes the code too unreadable here. Just drop the const (insert FB meme here) :) https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:111: return cell ? KVIndex(cell - cells_) : kInvalidKVIndex; KVIndex -> static_cast https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:145: friend base::trace_event::AllocationRegisterTest; out of curiosity why did you drop "class" here? https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:213: if (NumBuckets == (NumBuckets & ~(NumBuckets - 1))) { hmm this is going to hurt your fastpath. Sorry probably I wasn't clear in my previous comment. My suggestion is to make it a requirement that NumBuckets should be a power of two and add a static_assert to enforce it. Either you make it so and always use line 215 (which will very likely end up being inlined), or always use the generic version. At this point the cost of the branch will eat the saving of not doing a division and just make the code more complicated. I'd just static_asswer(power_of_two) and always use 215, unless there is a strong reason why you want numBuckets to be a non-power-of-two. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:271: using AllocationKVIndex = size_t; nit: not sure what the "KV" part adds to the name, maybe just AllocationIndex? https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:271: using AllocationKVIndex = size_t; shouldn't this be = FixedHashMap::Index ? https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:281: AllocationRegister(size_t num_allocation_cells, size_t num_backtrace_cells); since you carefully avoided exposing the "cells" maybe at this point these two vars can be called: allocation_capacity and backtrace_capacity which is IMHO more readable? https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:305: friend AllocationRegisterTest; y u not like class? :) https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:327: // drops to zero, the backtrace is removed from the map. ahhh now makes way more sense. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register_posix.cc (right): https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register_posix.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. I think you have to update also heap_profiler_allocation_register_win.cc https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register_unittest.cc (right): https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register_unittest.cc:225: Allocation a; s/a/ignored/
The CQ bit was checked by primiano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
On 2016/06/28 14:55:23, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) Looks like most of the tests are failing, at least on mac :/
On 2016/06/28 15:47:28, Primiano Tucci wrote: > On 2016/06/28 14:55:23, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) > > Looks like most of the tests are failing, at least on mac :/ Yeah, I made a stupid mistake (== vs !=). I was testing in a different setup (to becnhmark against "standard" impl), which had all changes except for that.
Fix for windows is on the way (I didn't know we have heap_profiler_allocation_register_win.cc :) https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:133: KVIndex FindValidIndex(KVIndex index) const { On 2016/06/28 14:23:07, Primiano Tucci wrote: > On 2016/06/28 10:55:00, Dmitry Skiba wrote: > > On 2016/06/23 20:46:25, Primiano Tucci wrote: > > > s/FindValidIndex/FindNextValidCellIndex/ (or just FindNextValidCell) > > > > The thing with 'Cell' is that it doesn't appear anywhere in the API. I.e. > there > > are Get/Find/Insert/Remove and then suddenly a method that deals with 'Cell'. > > But 'Valid' is also wrong, because by definition KVIndex has just single > invalid > > value. > > Oh right realized only now. I thought this was a private method, is not. > shouldn't this be private? > > > > I think FindNextIndex() is a better name for this - it describes what happens > > (finding the next index) and doesn't introduce any new (Cell) or wrong (Valid) > > concepts. > At this point just Next(index). If you call it FindNextIndex sounds like you are > looking for an index, at which point I'd ask "why you don't return just > index+1"? The reason is that what you mean with FindNextIndex is > FindNextAndReturnItsIndex, which is an awkward name. So, since everything is > index-based by design, let's go for just FindNext. But then, why the Find part > at and not just Next(index) at this point? At least that recalls the next() > operator in python and std::next which are logically the same. Done. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:242: mutable Bucket* buckets_; On 2016/06/28 14:23:07, Primiano Tucci wrote: > On 2016/06/28 10:54:59, Dmitry Skiba wrote: > > On 2016/06/23 20:46:25, Primiano Tucci wrote: > > > why is this mutable? > > > > Find() is const, and is uses Lookup() which returns Cell**, so Lookup() should > > also be const, but it won't be able to return Cell** unless buckets_ is > mutable. > > I see, isn't the right thing to do having Lookup retuning a const Cell ptr and > use const_cast in Insert to amend the constness of the access path ? Hmm, but mutable was created exactly to avoid doing const_cast<>, which is always very suspicious. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.cc (right): https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:18: index_ = register_.allocations_.FindNextIndex(index_ + 1); On 2016/06/28 14:23:07, Primiano Tucci wrote: > shouldn't the +1 be part of the FindNext function, as there is "Next" in the > name? Yeah, but the problem then is how do you find the first valid index? Right now you start with 0, and can actually get 0 back if cell #0 is occupied. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:53: return (total_value * 131101) >> 14; On 2016/06/28 14:23:07, Primiano Tucci wrote: > Not sure I follow the comment when you say "constants from AddressHasher". > I'd probably just say "this magic constatnt was chosen empirically on real > traces yada yada ...." Done. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:65: const uintptr_t a = 131101; On 2016/06/28 14:23:07, Primiano Tucci wrote: > we should call this "the Ruud constant" :) Yup :) This hash function is pretty good. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:43: "Key and Value shouldn't have destructors"); On 2016/06/28 14:23:07, Primiano Tucci wrote: > Excellent, TIL std::is_trivially_destructible. I thought that we needed this > check while reviewing this but didn't know there was a way to enforce it. Yeah, the only problem is that std::is_trivially_destructible is having issues on Linux, but luckily we have template_util.h https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:45: using KVPair = std::pair<const Key, Value>; On 2016/06/28 14:23:07, Primiano Tucci wrote: > to be honest I think that this "const" here is causing you a bit of awkwardness > around to maintain const correctness. If this was non-const you could avoid the > placement new. > The reality here is that the keys are not really const because you constantly > need to initialize and re-initialize the cells. > I understand that you want to just prevent the backtrace to be touched once > initialized, but I feel this is causing your code to become unnecessarily > unreadable. > > If you really want const correctness, make Cell have a std::pair<K,V> (Without > const) add an Initialize(const KVPair&) method and use const_cast there. But > that is probably overkill as well. So I'd personally just drop the const to keep > it simple. This is how std::map/set/etc does it, and it makes sense, since we don't want key to change while it's added to the hash table. And actually there is no other way - if we make it just pair<K,V> then non-const Get() method (which we need, because it's OK to mutate Value) will expose mutable Key. Also I really don't like const_cast<> - it feels super dirty. And actually placement new initialization of kv allows us not to care about copy-constructors of Key and Value. If we wanted to just assign them, we would have to make sure both Key and Value are trivially copyable (is_trivially_copyable), and that doesn't reliably work on GCC - see how we jump through hoops in bit_cast.h I think what we have now strikes middle ground - we don't care about destructors, so we can just in-place construct kv all the time. It's just C++ syntax for that is a little bit scary. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:77: return {static_cast<KVIndex>(cell - cells_), false}; // not inserted On 2016/06/28 14:23:07, Primiano Tucci wrote: > nit: add extra space before // Done. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:111: return cell ? KVIndex(cell - cells_) : kInvalidKVIndex; On 2016/06/28 14:23:07, Primiano Tucci wrote: > KVIndex -> static_cast Done. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:145: friend base::trace_event::AllocationRegisterTest; On 2016/06/28 14:23:07, Primiano Tucci wrote: > out of curiosity why did you drop "class" here? Yeah, so FixedHashMap is inside 'internal' namespace, so just doing 'friend class AllocationRegisterTest' doesnt work, as it injects (and makes a friend) AllocationRegisterTest into 'internal' namespace. So I had to forward-declare AllocationRegisterTest at the top, and since it's declared, we don't need class here. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:213: if (NumBuckets == (NumBuckets & ~(NumBuckets - 1))) { On 2016/06/28 14:23:07, Primiano Tucci wrote: > hmm this is going to hurt your fastpath. Sorry probably I wasn't clear in my > previous comment. > My suggestion is to make it a requirement that NumBuckets should be a power of > two and add a static_assert to enforce it. > Either you make it so and always use line 215 (which will very likely end up > being inlined), or always use the generic version. At this point the cost of the > branch will eat the saving of not doing a division and just make the code more > complicated. > I'd just static_asswer(power_of_two) and always use 215, unless there is a > strong reason why you want numBuckets to be a non-power-of-two. I don't think that's the case - NumBuckets is a template argument, so if() above equivalent to if (true) or if (false) depending on NumBuckets. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:271: using AllocationKVIndex = size_t; On 2016/06/28 14:23:07, Primiano Tucci wrote: > nit: not sure what the "KV" part adds to the name, maybe just AllocationIndex? Removed KV. Yes, it should be AllocationMap::KVIndex, the only problem that AllocationMap is declared after this class. We could fix this by moving declaration out of this class, but that's too much hassle, and besides this is how it was before - it was uint32_t to match CellIndex declaration below. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:281: AllocationRegister(size_t num_allocation_cells, size_t num_backtrace_cells); On 2016/06/28 14:23:07, Primiano Tucci wrote: > since you carefully avoided exposing the "cells" maybe at this point these two > vars can be called: > allocation_capacity and backtrace_capacity which is IMHO more readable? Done. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:305: friend AllocationRegisterTest; On 2016/06/28 14:23:07, Primiano Tucci wrote: > y u not like class? :) It's not needed, since I forward-declared it on top of the file.
k still LGTM. Good luck with windows, should hopefully be a minor thing. https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2089253002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:242: mutable Bucket* buckets_; On 2016/06/29 16:12:26, Dmitry Skiba wrote: > On 2016/06/28 14:23:07, Primiano Tucci wrote: > > On 2016/06/28 10:54:59, Dmitry Skiba wrote: > > > On 2016/06/23 20:46:25, Primiano Tucci wrote: > > > > why is this mutable? > > > > > > Find() is const, and is uses Lookup() which returns Cell**, so Lookup() > should > > > also be const, but it won't be able to return Cell** unless buckets_ is > > mutable. > > > > I see, isn't the right thing to do having Lookup retuning a const Cell ptr and > > use const_cast in Insert to amend the constness of the access path ? > > Hmm, but mutable was created exactly to avoid doing const_cast<>, which is > always very suspicious. Well depends on the cast. mutable is more for things lik mutexes which you need to change but they are not actually part of the state of the class. Instead in this case the bucets are part of the state of the class. The problem is that in one case you use lookup that is const and promises to not change it (which is still true) but want to change its result. Anyways, whatever, not a huge deal but I still think in this case const_Cast was the way to go, as it's insert that is special, not buckets_. There is a clean solution that avoids both mutable and const_cast, which is having both a const and non const version of Lookup. But let's not go there :) https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.cc (right): https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:18: index_ = register_.allocations_.FindNextIndex(index_ + 1); On 2016/06/29 16:12:26, Dmitry Skiba wrote: > On 2016/06/28 14:23:07, Primiano Tucci wrote: > > shouldn't the +1 be part of the FindNext function, as there is "Next" in the > > name? > > Yeah, but the problem then is how do you find the first valid index? Right now > you start with 0, and can actually get 0 back if cell #0 is occupied. ah ok. Fine then. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:45: using KVPair = std::pair<const Key, Value>; On 2016/06/29 16:12:26, Dmitry Skiba wrote: > On 2016/06/28 14:23:07, Primiano Tucci wrote: > > to be honest I think that this "const" here is causing you a bit of > awkwardness > > around to maintain const correctness. If this was non-const you could avoid > the > > placement new. > > The reality here is that the keys are not really const because you constantly > > need to initialize and re-initialize the cells. > > I understand that you want to just prevent the backtrace to be touched once > > initialized, but I feel this is causing your code to become unnecessarily > > unreadable. > > > > If you really want const correctness, make Cell have a std::pair<K,V> (Without > > const) add an Initialize(const KVPair&) method and use const_cast there. But > > that is probably overkill as well. So I'd personally just drop the const to > keep > > it simple. > > This is how std::map/set/etc does it, and it makes sense, since we don't want > key to change while it's added to the hash table. And actually there is no other > way - if we make it just pair<K,V> then non-const Get() method (which we need, > because it's OK to mutate Value) will expose mutable Key. Also I really don't > like const_cast<> - it feels super dirty. And actually placement new > initialization of kv allows us not to care about copy-constructors of Key and > Value. If we wanted to just assign them, we would have to make sure both Key and > Value are trivially copyable (is_trivially_copyable), and that doesn't reliably > work on GCC - see how we jump through hoops in bit_cast.h > > I think what we have now strikes middle ground - we don't care about > destructors, so we can just in-place construct kv all the time. It's just C++ > syntax for that is a little bit scary. ok fine. As we don't have destructors anymore agree this is borderline but more on the ok side :) https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:145: friend base::trace_event::AllocationRegisterTest; On 2016/06/29 16:12:26, Dmitry Skiba wrote: > On 2016/06/28 14:23:07, Primiano Tucci wrote: > > out of curiosity why did you drop "class" here? > > Yeah, so FixedHashMap is inside 'internal' namespace, so just doing 'friend > class AllocationRegisterTest' doesnt work, as it injects (and makes a friend) > AllocationRegisterTest into 'internal' namespace. So I had to forward-declare > AllocationRegisterTest at the top, and since it's declared, we don't need class > here. Ok the namespace thing makes sense, but my point is that for doc purposes IMHO you still want "class" here (unless it causes a problem). Otherwise it sounds like you are making a friend function which is not the case. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:213: if (NumBuckets == (NumBuckets & ~(NumBuckets - 1))) { On 2016/06/29 16:12:26, Dmitry Skiba wrote: > On 2016/06/28 14:23:07, Primiano Tucci wrote: > > hmm this is going to hurt your fastpath. Sorry probably I wasn't clear in my > > previous comment. > > My suggestion is to make it a requirement that NumBuckets should be a power of > > two and add a static_assert to enforce it. > > Either you make it so and always use line 215 (which will very likely end up > > being inlined), or always use the generic version. At this point the cost of > the > > branch will eat the saving of not doing a division and just make the code more > > complicated. > > I'd just static_asswer(power_of_two) and always use 215, unless there is a > > strong reason why you want numBuckets to be a non-power-of-two. > > I don't think that's the case - NumBuckets is a template argument, so if() above > equivalent to if (true) or if (false) depending on NumBuckets. Ah k didn't realize that is a constexpr sorry. https://codereview.chromium.org/2089253002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:305: friend AllocationRegisterTest; On 2016/06/29 16:12:26, Dmitry Skiba wrote: > On 2016/06/28 14:23:07, Primiano Tucci wrote: > > y u not like class? :) > > It's not needed, since I forward-declared it on top of the file. Yup I know but this looks like a friend function now :) if i'm reading line 305 I don't remember line 23 up there anymore.
dskiba@google.com changed reviewers: + mariakhomenko@chromium.org, ssid@chromium.org
Guys, please review just in case we missed something with Primiano.
The CQ bit was checked by dskiba@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/2089253002/#ps120001 (title: "Fix Windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [tracing] Optimize AllocationRegister and increase max backtrace depth. This CL both increases maximum backtrace depth (24 -> 48) and reduces AllocationRegister memory usage (265 -> 42 MiB on 32-bit platforms) at the expense of performance (~30% hit). See the bug for more details. BUG=617772 ========== to ========== [tracing] Optimize AllocationRegister and increase max backtrace depth. This CL both increases maximum backtrace depth (24 -> 48) and reduces AllocationRegister memory usage (265 -> 42 MiB on 32-bit platforms) at the expense of performance (~30% hit). See the bug for more details. BUG=617772 Committed: https://crrev.com/557f4aad30e98c3c5434ea37ca061fd3b658d570 Cr-Commit-Position: refs/heads/master@{#403291} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/557f4aad30e98c3c5434ea37ca061fd3b658d570 Cr-Commit-Position: refs/heads/master@{#403291} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
