Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1625753002:
Allocation sampling for paged/lo spaces (Closed)
|
Created:
4 years, 11 months ago by mattloring Modified:
4 years, 10 months ago CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAllocation sampling for paged/large object spaces
This change expands allocation sampling to include old, map, code, and large object spaces. This involved refactoring much of the observation logic out of NewSpace into Space and overriding as needed in sub-classes.
Additionally, the sampling heap profiler now maintains a pair of heap observers. One observer is used for observing new space and resetting the inline allocation limit to be periodically notified of allocations. The other observes allocation across the other spaces where there is no additional work required to observe allocations.
Tests have been updated to ensure that allocations are observed correctly for Paged and LargeObject spaces.
R=ofrobots@google.com, hpayer@chromium.org, ulan@chromium.org
BUG=
Committed: https://crrev.com/f3cdf8a9f02b58bdbd89e6fee1e0bb561df9f4c9
Cr-Commit-Position: refs/heads/master@{#33959}
Patch Set 1 #
Total comments: 27
Patch Set 2 : WIP: Allocation sampling for paged/lo spaces #
Total comments: 4
Patch Set 3 : Pause all spaces at once with PauseAllocationObserversScope #Patch Set 4 : Added tests for PagedSpace/LargeObjectSpace observation #Patch Set 5 : Re-add comments lost during refactoring #Patch Set 6 : "Rebase" #
Total comments: 25
Patch Set 7 : Use allspaces iterator and fix style issues #
Total comments: 16
Patch Set 8 : Use smart pointers for allocation observers #Patch Set 9 : Fix initialization of observers and remove unused heap #Patch Set 10 : USE for DCHECK only variable #
Messages
Total messages: 39 (11 generated)
https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2801 src/heap/heap.h:2801: explicit AllocationObserver(Heap* heap, intptr_t step_size) 'explicit' is only needed on single argument constructors. https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2807 src/heap/heap.h:2807: // Called each time the new space does an inline allocation step. This may be Update comment since it applies to more than just new space now. https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2811 src/heap/heap.h:2811: if (heap_->gc_state() != Heap::NOT_IN_GC) return; It would be better to generalize the PauseInlineAllocationObserver concept. The AllocationStep is supposed to cover >= 1 allocations, and checking the *current* heap state sounds wrong when multiple allocations are being reported in aggregate. https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h#newco... src/heap/spaces-inl.h:437: if (obj->IsHeapObject()) { Why is this check necessary here? https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc#newcode2625 src/heap/spaces.cc:2625: owner_->heap()->incremental_marking()->OldSpaceStep(size_in_bytes - You probably need to notify the observers here too. https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc#newcode3145 src/heap/spaces.cc:3145: } This loop has been duplicated in a few places. It would be good to refactor to avoid duplication. https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.h#newcode2055 src/heap/spaces.h:2055: Suggestion: These seem almost like a copy of the Add/RemoveInlineAllocationObserver. Perhaps we could have a single set of methods with a boolean flag? You may want to promote these methods and the corresponding observer lists into the superclass (Space)? https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:152: uint64_t rate_; Weak suggestion: prefer to group member together by type (e.g. pointers first). This minimizes spurious padding inserted for alignment. https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:153: base::RandomNumberGenerator* random_; Mark the above as const. E.g. SamplingHeapProfiler* const profiler_; https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:156: uint64_t rate); Any reason you make this non-static here? https://codereview.chromium.org/1625753002/diff/1/test/cctest/heap/test-space... File test/cctest/heap/test-spaces.cc (right): https://codereview.chromium.org/1625753002/diff/1/test/cctest/heap/test-space... test/cctest/heap/test-spaces.cc:925: isolate->Dispose(); Add tests for old space, lo space, map space, code space.
On 2016/01/23 16:16:31, ofrobots wrote: > https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h > File src/heap/heap.h (right): > > https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2801 > src/heap/heap.h:2801: explicit AllocationObserver(Heap* heap, intptr_t > step_size) > 'explicit' is only needed on single argument constructors. > > https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2807 > src/heap/heap.h:2807: // Called each time the new space does an inline > allocation step. This may be > Update comment since it applies to more than just new space now. > > https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2811 > src/heap/heap.h:2811: if (heap_->gc_state() != Heap::NOT_IN_GC) return; > It would be better to generalize the PauseInlineAllocationObserver concept. The > AllocationStep is supposed to cover >= 1 allocations, and checking the *current* > heap state sounds wrong when multiple allocations are being reported in > aggregate. > > https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h > File src/heap/spaces-inl.h (right): > > https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h#newco... > src/heap/spaces-inl.h:437: if (obj->IsHeapObject()) { > Why is this check necessary here? > > https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc > File src/heap/spaces.cc (right): > > https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc#newcode2625 > src/heap/spaces.cc:2625: > owner_->heap()->incremental_marking()->OldSpaceStep(size_in_bytes - > You probably need to notify the observers here too. > > https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc#newcode3145 > src/heap/spaces.cc:3145: } > This loop has been duplicated in a few places. It would be good to refactor to > avoid duplication. > > https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.h > File src/heap/spaces.h (right): > > https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.h#newcode2055 > src/heap/spaces.h:2055: > Suggestion: These seem almost like a copy of the > Add/RemoveInlineAllocationObserver. Perhaps we could have a single set of > methods with a boolean flag? You may want to promote these methods and the > corresponding observer lists into the superclass (Space)? > > https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... > File src/profiler/sampling-heap-profiler.h (right): > > https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... > src/profiler/sampling-heap-profiler.h:152: uint64_t rate_; > Weak suggestion: prefer to group member together by type (e.g. pointers first). > This minimizes spurious padding inserted for alignment. > > https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... > src/profiler/sampling-heap-profiler.h:153: base::RandomNumberGenerator* random_; > Mark the above as const. E.g. SamplingHeapProfiler* const profiler_; > > https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... > src/profiler/sampling-heap-profiler.h:156: uint64_t rate); > Any reason you make this non-static here? > > https://codereview.chromium.org/1625753002/diff/1/test/cctest/heap/test-space... > File test/cctest/heap/test-spaces.cc (right): > > https://codereview.chromium.org/1625753002/diff/1/test/cctest/heap/test-space... > test/cctest/heap/test-spaces.cc:925: isolate->Dispose(); > Add tests for old space, lo space, map space, code space. Continuing here (https://codereview.chromium.org/1625753002) due to upload failure.
https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h#newco... src/heap/spaces-inl.h:437: if (obj->IsHeapObject()) { On 2016/01/23 16:16:31, ofrobots wrote: > Why is this check necessary here? *bump* https://codereview.chromium.org/1625753002/diff/20001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1625753002/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:1427: PauseAllocationObserversScope pause_lo_observers(lo_space()); This is just begging to be refactored :). Maybe keep a list in heap of all the observers. PauseAllocationObservers should pause all of them. This probably would mean that the primary interface to add allocation observers would move to Heap. https://codereview.chromium.org/1625753002/diff/20001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/20001/src/heap/spaces.h#newco... src/heap/spaces.h:960: : allocation_observers_paused_(false), As per suggestion above, it might make sense for Heap to own this boolean flag rather than each space. They should all be paused, or none.
https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc#newcode2625 src/heap/spaces.cc:2625: owner_->heap()->incremental_marking()->OldSpaceStep(size_in_bytes - On 2016/01/23 16:16:31, ofrobots wrote: > You probably need to notify the observers here too. *bump*
https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2801 src/heap/heap.h:2801: explicit AllocationObserver(Heap* heap, intptr_t step_size) On 2016/01/23 16:16:30, ofrobots wrote: > 'explicit' is only needed on single argument constructors. Done. https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2807 src/heap/heap.h:2807: // Called each time the new space does an inline allocation step. This may be On 2016/01/23 16:16:30, ofrobots wrote: > Update comment since it applies to more than just new space now. Done. https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2811 src/heap/heap.h:2811: if (heap_->gc_state() != Heap::NOT_IN_GC) return; On 2016/01/23 16:16:31, ofrobots wrote: > It would be better to generalize the PauseInlineAllocationObserver concept. The > AllocationStep is supposed to cover >= 1 allocations, and checking the *current* > heap state sounds wrong when multiple allocations are being reported in > aggregate. I've generalized it and pausing is unified across all spaces. It would be nice to find a better way to isolate the locations where pause checks must be performed. Right now, checks are made before each call to AllocationStep by spaces other than new_space which seems analogous to what we had before. @ofrobots, how do you envision these checks being performed to allow for multiple allocations to aggregate? https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h#newco... src/heap/spaces-inl.h:437: if (obj->IsHeapObject()) { On 2016/01/23 16:16:31, ofrobots wrote: > Why is this check necessary here? An Object can be an Smi or a HeapObject so I check that here. Is the check known to be unnecessary in this instance? https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc#newcode2625 src/heap/spaces.cc:2625: owner_->heap()->incremental_marking()->OldSpaceStep(size_in_bytes - On 2016/01/23 16:16:31, ofrobots wrote: > You probably need to notify the observers here too. Done. https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.cc#newcode3145 src/heap/spaces.cc:3145: } On 2016/01/23 16:16:31, ofrobots wrote: > This loop has been duplicated in a few places. It would be good to refactor to > avoid duplication. Done. https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces.h#newcode2055 src/heap/spaces.h:2055: On 2016/01/23 16:16:31, ofrobots wrote: > Suggestion: These seem almost like a copy of the > Add/RemoveInlineAllocationObserver. Perhaps we could have a single set of > methods with a boolean flag? You may want to promote these methods and the > corresponding observer lists into the superclass (Space)? Done. https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:152: uint64_t rate_; On 2016/01/23 16:16:31, ofrobots wrote: > Weak suggestion: prefer to group member together by type (e.g. pointers first). > This minimizes spurious padding inserted for alignment. Done. https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:153: base::RandomNumberGenerator* random_; On 2016/01/23 16:16:31, ofrobots wrote: > Mark the above as const. E.g. SamplingHeapProfiler* const profiler_; Done. https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:156: uint64_t rate); On 2016/01/23 16:16:31, ofrobots wrote: > Any reason you make this non-static here? I was having problems with `incomplete type base::RandomNumberGenerator` when defining this function here. Is there another way around this? https://codereview.chromium.org/1625753002/diff/1/test/cctest/heap/test-space... File test/cctest/heap/test-spaces.cc (right): https://codereview.chromium.org/1625753002/diff/1/test/cctest/heap/test-space... test/cctest/heap/test-spaces.cc:925: isolate->Dispose(); On 2016/01/23 16:16:31, ofrobots wrote: > Add tests for old space, lo space, map space, code space. Getting to these next, just wanted to hammer out the structure of these changes first.
https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/heap.h#newcode2811 src/heap/heap.h:2811: if (heap_->gc_state() != Heap::NOT_IN_GC) return; On 2016/01/26 00:42:48, mattloring wrote: > On 2016/01/23 16:16:31, ofrobots wrote: > > It would be better to generalize the PauseInlineAllocationObserver concept. > The > > AllocationStep is supposed to cover >= 1 allocations, and checking the > *current* > > heap state sounds wrong when multiple allocations are being reported in > > aggregate. > > I've generalized it and pausing is unified across all spaces. It would be nice > to find a better way to isolate the locations where pause checks must be > performed. Right now, checks are made before each call to AllocationStep by > spaces other than new_space which seems analogous to what we had before. > @ofrobots, how do you envision these checks being performed to allow for > multiple allocations to aggregate? Looking at the way this works, it seems like non-New spaces always call this on single objects, so this will never be an aggregate for those spaces. If PauseAlloctionObservers is working correctly, the check would not be necessary (and could instead be assertions). https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/1625753002/diff/1/src/heap/spaces-inl.h#newco... src/heap/spaces-inl.h:437: if (obj->IsHeapObject()) { On 2016/01/26 00:42:48, mattloring wrote: > On 2016/01/23 16:16:31, ofrobots wrote: > > Why is this check necessary here? > > An Object can be an Smi or a HeapObject so I check that here. Is the check known > to be unnecessary in this instance? We just allocated an object on the heap; so this check should not be necessary I think. https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1625753002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:156: uint64_t rate); On 2016/01/26 00:42:48, mattloring wrote: > On 2016/01/23 16:16:31, ofrobots wrote: > > Any reason you make this non-static here? > > I was having problems with `incomplete type base::RandomNumberGenerator` when > defining this function here. Is there another way around this? Move (keep) the function in the .cc file. This can be a static function as this doesn't refer to `this`.
https://codereview.chromium.org/1625753002/diff/20001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1625753002/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:1427: PauseAllocationObserversScope pause_lo_observers(lo_space()); On 2016/01/26 00:38:25, ofrobots wrote: > This is just begging to be refactored :). > > Maybe keep a list in heap of all the observers. PauseAllocationObservers should > pause all of them. This probably would mean that the primary interface to add > allocation observers would move to Heap. Refactored to pause/unpause all spaces together. https://codereview.chromium.org/1625753002/diff/20001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/20001/src/heap/spaces.h#newco... src/heap/spaces.h:960: : allocation_observers_paused_(false), On 2016/01/26 00:38:25, ofrobots wrote: > As per suggestion above, it might make sense for Heap to own this boolean flag > rather than each space. They should all be paused, or none. This is currently enforced by the PauseAllocationObserverScope. This will be further refactored in a follow on to aggregate all observers of the spaces and handle them uniformly.
Description was changed from ========== WIP: Allocation sampling for paged/lo spaces This change expands allocation sampling to include old, map, code, and large object spaces. R=ofrobots@google.com BUG= ========== to ========== WIP: Allocation sampling for paged/lo spaces This change expands allocation sampling to include old, map, code, and large object spaces. R=ofrobots@google.com, hpayer@chromium.org, ulan@chromium.org BUG= ==========
mattloring@google.com changed reviewers: + hpayer@chromium.org, ulan@chromium.org
Description was changed from ========== WIP: Allocation sampling for paged/lo spaces This change expands allocation sampling to include old, map, code, and large object spaces. R=ofrobots@google.com, hpayer@chromium.org, ulan@chromium.org BUG= ========== to ========== Allocation sampling for paged/large object spaces This change expands allocation sampling to include old, map, code, and large object spaces. This involved refactoring much of the observation logic out of NewSpace into Space and overriding as needed in sub-classes. Additionally, the sampling heap profiler now maintains a pair of heap observers. One observer is used for observing new space and resetting the inline allocation limit to be periodically notified of allocations. The other observes allocation across the other spaces where there is no additional work required to observe allocations. Tests have been updated to ensure that allocations are observed correctly for Paged and LargeObject spaces. R=ofrobots@google.com, hpayer@chromium.org, ulan@chromium.org BUG= ==========
https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.cc#new... src/heap/spaces.cc:75: heap_->new_space()->PauseAllocationObservers(); Use the AllSpaces iterator. https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.cc#new... src/heap/spaces.cc:82: PauseAllocationObserversScope::~PauseAllocationObserversScope() { Use the AllSpaces iterator. https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:973: static_cast<void>(removed); USE(removed); https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:1034: // marking or because of idle scavenge, we 'interrupt' inline allocation every This list is not complete: allocation statistics gather also interrupts inline allocation. https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:1038: List<AllocationObserver*>* allocation_observers_ = allocate the allocation_observers in the constructor And deallocate it when the space is shut down. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:48: heap_, rate, rate, this, heap->isolate()->random_number_generator()); Use the AllSpaces iterator. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:57: heap_->new_space()->RemoveAllocationObserver(new_space_observer_); Use the AllSpaces iterator. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:98: friend class SamplingAllocationObserver; Style guide: friends come after variables. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:99: using Node = v8::AllocationProfile::Node; Style guide: Do not use using-directives. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:155: intptr_t GetNextSampleInterval(base::RandomNumberGenerator* random, Style guide: methods go before variables. https://codereview.chromium.org/1625753002/diff/100001/test/cctest/heap/test-... File test/cctest/heap/test-spaces.cc (right): https://codereview.chromium.org/1625753002/diff/100001/test/cctest/heap/test-... test/cctest/heap/test-spaces.cc:770: Code* code = Code::cast(filler); Why cast to Code?
https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.cc File src/heap/spaces.cc (right): https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.cc#new... src/heap/spaces.cc:75: heap_->new_space()->PauseAllocationObservers(); On 2016/02/08 10:13:48, Hannes Payer wrote: > Use the AllSpaces iterator. Done. https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.cc#new... src/heap/spaces.cc:82: PauseAllocationObserversScope::~PauseAllocationObserversScope() { On 2016/02/08 10:13:48, Hannes Payer wrote: > Use the AllSpaces iterator. Done. https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:973: static_cast<void>(removed); On 2016/02/08 10:13:48, Hannes Payer wrote: > USE(removed); Done. https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:1034: // marking or because of idle scavenge, we 'interrupt' inline allocation every On 2016/02/08 10:13:48, Hannes Payer wrote: > This list is not complete: allocation statistics gather also interrupts inline > allocation. Done. https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:1038: List<AllocationObserver*>* allocation_observers_ = On 2016/02/08 10:13:48, Hannes Payer wrote: > allocate the allocation_observers in the constructor > > And deallocate it when the space is shut down. What is the best strategy for deallocating a list where the contents may be shared with other lists as well (some observers may be observing other spaces as well)? https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:48: heap_, rate, rate, this, heap->isolate()->random_number_generator()); On 2016/02/08 10:13:48, Hannes Payer wrote: > Use the AllSpaces iterator. Done. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:57: heap_->new_space()->RemoveAllocationObserver(new_space_observer_); On 2016/02/08 10:13:48, Hannes Payer wrote: > Use the AllSpaces iterator. Done. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:98: friend class SamplingAllocationObserver; On 2016/02/08 10:13:48, Hannes Payer wrote: > Style guide: friends come after variables. Done. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:99: using Node = v8::AllocationProfile::Node; On 2016/02/08 10:13:48, Hannes Payer wrote: > Style guide: Do not use using-directives. Done. https://codereview.chromium.org/1625753002/diff/100001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:155: intptr_t GetNextSampleInterval(base::RandomNumberGenerator* random, On 2016/02/08 10:13:48, Hannes Payer wrote: > Style guide: methods go before variables. Done.
https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:1038: List<AllocationObserver*>* allocation_observers_ = On 2016/02/09 20:20:05, mattloring wrote: > On 2016/02/08 10:13:48, Hannes Payer wrote: > > allocate the allocation_observers in the constructor > > > > And deallocate it when the space is shut down. > > What is the best strategy for deallocating a list where the contents may be > shared with other lists as well (some observers may be observing other spaces as > well)? Does allocation_observers_ need to be dynamically allocated using new? Would an object rather than pointer work instead: List<AllocationObserver*> allocation_observers_;
https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:1038: List<AllocationObserver*>* allocation_observers_ = On 2016/02/10 08:15:38, ofrobots wrote: > On 2016/02/09 20:20:05, mattloring wrote: > > On 2016/02/08 10:13:48, Hannes Payer wrote: > > > allocate the allocation_observers in the constructor > > > > > > And deallocate it when the space is shut down. > > > > What is the best strategy for deallocating a list where the contents may be > > shared with other lists as well (some observers may be observing other spaces > as > > well)? > > Does allocation_observers_ need to be dynamically allocated using new? Would an > object rather than pointer work instead: > List<AllocationObserver*> allocation_observers_; If I do not dynamically allocate it, it causes the copy constructor for spaces to be deleted yielding: copy constructor of 'Space' is implicitly deleted because field 'allocation_observers_' has a deleted copy constructor is it better to explicitly implement a copy constructor?
On 2016/02/10 13:19:23, mattloring wrote: > https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h > File src/heap/spaces.h (right): > > https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... > src/heap/spaces.h:1038: List<AllocationObserver*>* allocation_observers_ = > On 2016/02/10 08:15:38, ofrobots wrote: > > On 2016/02/09 20:20:05, mattloring wrote: > > > On 2016/02/08 10:13:48, Hannes Payer wrote: > > > > allocate the allocation_observers in the constructor > > > > > > > > And deallocate it when the space is shut down. > > > > > > What is the best strategy for deallocating a list where the contents may be > > > shared with other lists as well (some observers may be observing other > spaces > > as > > > well)? > > > > Does allocation_observers_ need to be dynamically allocated using new? Would > an > > object rather than pointer work instead: > > List<AllocationObserver*> allocation_observers_; > > If I do not dynamically allocate it, it causes the copy constructor for spaces > to be deleted yielding: > > copy constructor of 'Space' is implicitly deleted because field > 'allocation_observers_' has a deleted copy constructor > > is it better to explicitly implement a copy constructor? Sorry, I misunderstood your original question. Your list is a list of pointers that are not owned by the list, so freeing the list doesn't free those pointers.
https://codereview.chromium.org/1625753002/diff/120001/src/heap/incremental-m... File src/heap/incremental-marking.h (right): https://codereview.chromium.org/1625753002/diff/120001/src/heap/incremental-m... src/heap/incremental-marking.h:221: Observer(IncrementalMarking& incremental_marking, Heap* heap, `heap` parameter not used? https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces-inl.h#... src/heap/spaces-inl.h:436: Object* obj = result.ToObjectChecked(); This code would be much simpler if you used AllocationResult::To<T> template function instead. See example here: https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/heap-i... https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces.h#newc... src/heap/spaces.h:956: allocation_observers_ = new List<AllocationObserver*>(); Never freed? Use SmartPointer to manage this. https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces.h#newc... src/heap/spaces.h:1040: List<AllocationObserver*>* allocation_observers_; The comment about inline allocation stepping seems appropriate here. The observers here apply to more than just inline allocation. https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:44: new_space_observer_ = new SamplingAllocationObserver( This allocation is never freed so it will leak. Use v8::base::SmartPointer to manage this pointer. https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:47: other_spaces_observer_ = new SamplingAllocationObserver( likewise. https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:126: explicit SamplingAllocationObserver(Heap* heap, intptr_t step_size, Don't need explicit here. https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:149: intptr_t GetNextSampleInterval(base::RandomNumberGenerator* random, You can use the random_ instance field instead of passing the random as argument now that this is not a static method anymore.
https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:1038: List<AllocationObserver*>* allocation_observers_ = On 2016/02/10 13:19:23, mattloring wrote: > On 2016/02/10 08:15:38, ofrobots wrote: > > On 2016/02/09 20:20:05, mattloring wrote: > > > On 2016/02/08 10:13:48, Hannes Payer wrote: > > > > allocate the allocation_observers in the constructor > > > > > > > > And deallocate it when the space is shut down. > > > > > > What is the best strategy for deallocating a list where the contents may be > > > shared with other lists as well (some observers may be observing other > spaces > > as > > > well)? > > > > Does allocation_observers_ need to be dynamically allocated using new? Would > an > > object rather than pointer work instead: > > List<AllocationObserver*> allocation_observers_; > > If I do not dynamically allocate it, it causes the copy constructor for spaces > to be deleted yielding: > > copy constructor of 'Space' is implicitly deleted because field > 'allocation_observers_' has a deleted copy constructor > > is it better to explicitly implement a copy constructor? I am not sure if I understand your question. When the space dies, all other spaces will die as well (shortly afterwards). Deallocating the list will not impact the lifetime of the the list content. The actual observers have to be freed somewhere else.
https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/100001/src/heap/spaces.h#newc... src/heap/spaces.h:1038: List<AllocationObserver*>* allocation_observers_ = On 2016/02/11 11:50:08, Hannes Payer wrote: > On 2016/02/10 13:19:23, mattloring wrote: > > On 2016/02/10 08:15:38, ofrobots wrote: > > > On 2016/02/09 20:20:05, mattloring wrote: > > > > On 2016/02/08 10:13:48, Hannes Payer wrote: > > > > > allocate the allocation_observers in the constructor > > > > > > > > > > And deallocate it when the space is shut down. > > > > > > > > What is the best strategy for deallocating a list where the contents may > be > > > > shared with other lists as well (some observers may be observing other > > spaces > > > as > > > > well)? > > > > > > Does allocation_observers_ need to be dynamically allocated using new? Would > > an > > > object rather than pointer work instead: > > > List<AllocationObserver*> allocation_observers_; > > > > If I do not dynamically allocate it, it causes the copy constructor for spaces > > to be deleted yielding: > > > > copy constructor of 'Space' is implicitly deleted because field > > 'allocation_observers_' has a deleted copy constructor > > > > is it better to explicitly implement a copy constructor? > > I am not sure if I understand your question. > When the space dies, all other spaces will die as well (shortly afterwards). > Deallocating the list will not impact the lifetime of the the list content. The > actual observers have to be freed somewhere else. Sorry for the confusion. This should be handled now by a smart pointer. https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces-inl.h File src/heap/spaces-inl.h (right): https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces-inl.h#... src/heap/spaces-inl.h:436: Object* obj = result.ToObjectChecked(); On 2016/02/11 05:51:46, ofrobots wrote: > This code would be much simpler if you used AllocationResult::To<T> template > function instead. See example here: > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/heap-i... Done. https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces.h#newc... src/heap/spaces.h:956: allocation_observers_ = new List<AllocationObserver*>(); On 2016/02/11 05:51:46, ofrobots wrote: > Never freed? Use SmartPointer to manage this. Done. https://codereview.chromium.org/1625753002/diff/120001/src/heap/spaces.h#newc... src/heap/spaces.h:1040: List<AllocationObserver*>* allocation_observers_; On 2016/02/11 05:51:46, ofrobots wrote: > The comment about inline allocation stepping seems appropriate here. The > observers here apply to more than just inline allocation. I'm guessing you meant that it doesn't. I've moved it down into newspace where it applies. https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:44: new_space_observer_ = new SamplingAllocationObserver( On 2016/02/11 05:51:46, ofrobots wrote: > This allocation is never freed so it will leak. Use v8::base::SmartPointer to > manage this pointer. Done. https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:47: other_spaces_observer_ = new SamplingAllocationObserver( On 2016/02/11 05:51:47, ofrobots wrote: > likewise. Done. https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:126: explicit SamplingAllocationObserver(Heap* heap, intptr_t step_size, On 2016/02/11 05:51:47, ofrobots wrote: > Don't need explicit here. Done. https://codereview.chromium.org/1625753002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.h:149: intptr_t GetNextSampleInterval(base::RandomNumberGenerator* random, On 2016/02/11 05:51:47, ofrobots wrote: > You can use the random_ instance field instead of passing the random as argument > now that this is not a static method anymore. Done.
On 2016/02/11 14:54:17, mattloring wrote: LGTM
https://codereview.chromium.org/1625753002/diff/120001/src/heap/incremental-m... File src/heap/incremental-marking.h (right): https://codereview.chromium.org/1625753002/diff/120001/src/heap/incremental-m... src/heap/incremental-marking.h:221: Observer(IncrementalMarking& incremental_marking, Heap* heap, On 2016/02/11 05:51:46, ofrobots wrote: > `heap` parameter not used? Done.
lgtm
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1625753002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1625753002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1625753002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1625753002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mattloring@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ofrobots@google.com, hpayer@chromium.org Link to the patchset: https://codereview.chromium.org/1625753002/#ps180001 (title: "USE for DCHECK only variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1625753002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1625753002/180001
Message was sent while issue was closed.
Description was changed from ========== Allocation sampling for paged/large object spaces This change expands allocation sampling to include old, map, code, and large object spaces. This involved refactoring much of the observation logic out of NewSpace into Space and overriding as needed in sub-classes. Additionally, the sampling heap profiler now maintains a pair of heap observers. One observer is used for observing new space and resetting the inline allocation limit to be periodically notified of allocations. The other observes allocation across the other spaces where there is no additional work required to observe allocations. Tests have been updated to ensure that allocations are observed correctly for Paged and LargeObject spaces. R=ofrobots@google.com, hpayer@chromium.org, ulan@chromium.org BUG= ========== to ========== Allocation sampling for paged/large object spaces This change expands allocation sampling to include old, map, code, and large object spaces. This involved refactoring much of the observation logic out of NewSpace into Space and overriding as needed in sub-classes. Additionally, the sampling heap profiler now maintains a pair of heap observers. One observer is used for observing new space and resetting the inline allocation limit to be periodically notified of allocations. The other observes allocation across the other spaces where there is no additional work required to observe allocations. Tests have been updated to ensure that allocations are observed correctly for Paged and LargeObject spaces. R=ofrobots@google.com, hpayer@chromium.org, ulan@chromium.org BUG= ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Allocation sampling for paged/large object spaces This change expands allocation sampling to include old, map, code, and large object spaces. This involved refactoring much of the observation logic out of NewSpace into Space and overriding as needed in sub-classes. Additionally, the sampling heap profiler now maintains a pair of heap observers. One observer is used for observing new space and resetting the inline allocation limit to be periodically notified of allocations. The other observes allocation across the other spaces where there is no additional work required to observe allocations. Tests have been updated to ensure that allocations are observed correctly for Paged and LargeObject spaces. R=ofrobots@google.com, hpayer@chromium.org, ulan@chromium.org BUG= ========== to ========== Allocation sampling for paged/large object spaces This change expands allocation sampling to include old, map, code, and large object spaces. This involved refactoring much of the observation logic out of NewSpace into Space and overriding as needed in sub-classes. Additionally, the sampling heap profiler now maintains a pair of heap observers. One observer is used for observing new space and resetting the inline allocation limit to be periodically notified of allocations. The other observes allocation across the other spaces where there is no additional work required to observe allocations. Tests have been updated to ensure that allocations are observed correctly for Paged and LargeObject spaces. R=ofrobots@google.com, hpayer@chromium.org, ulan@chromium.org BUG= Committed: https://crrev.com/f3cdf8a9f02b58bdbd89e6fee1e0bb561df9f4c9 Cr-Commit-Position: refs/heads/master@{#33959} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f3cdf8a9f02b58bdbd89e6fee1e0bb561df9f4c9 Cr-Commit-Position: refs/heads/master@{#33959} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
