[heap] Remove pretenuring support in full GCs [v8/v8 : main]
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
PTAL
This CL seems to have +0.2% on both js2 [0] and js3 [1]. In js2 splay regresses -0.6% because of the first iteration (which I would argue is still in fluctuation range). splay in js3 improves by +1.4% - but not sure whether is is an actual improvment (the js3 runner doesn't show first/average/worst).
0: https://pinpoint-dot-chromeperf.appspot.com/job/1071109f310000
1: https://pinpoint-dot-chromeperf.appspot.com/job/101ccaa3310000
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/106ef044b10000
Related details
- Dominik Inführ
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
Pretenuring is still happening on full GCs IIUC: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=2422?q=ProcessPretenuringFeedback&ss=chromium
That part didn't change.
So, the CL here implements a version that doesn't update the counters but does digest feedback and follow through on it. I find this combination a bit strange.
Related details
- Dominik Inführ
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
Bug: 463528049Is this really improving GC pause time?
Related details
- Dominik Inführ
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 2 comments![]( "Open in Gerrit")
Pretenuring is still happening on full GCs IIUC: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=2422?q=ProcessPretenuringFeedback&ss=chromium
That part didn't change.
So, the CL here implements a version that doesn't update the counters but does digest feedback and follow through on it. I find this combination a bit strange.
Agreed. But AFAIU we shouldn't update counters/digest feedback here because `global_pretenuring_feedback_` is always empty (we don't invoke MergeAllocationSitePretenuringFeedback during full GCs).
Bug: 463528049Is this really improving GC pause time?
Evacuation and ClearNonLiveReferences() should become cheaper. So I would say yes in general. The main reason I used this bug though is because the CL is part of the parallel clearing CL.
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
Dominik InführPretenuring is still happening on full GCs IIUC: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=2422?q=ProcessPretenuringFeedback&ss=chromium
That part didn't change.
So, the CL here implements a version that doesn't update the counters but does digest feedback and follow through on it. I find this combination a bit strange.
Agreed. But AFAIU we shouldn't update counters/digest feedback here because `global_pretenuring_feedback_` is always empty (we don't invoke MergeAllocationSitePretenuringFeedback during full GCs).
Can we make this clear by moving the pretenuring logic fully behind young GCs?
Related details
- Dominik Inführ
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
Dominik InführPretenuring is still happening on full GCs IIUC: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=2422?q=ProcessPretenuringFeedback&ss=chromium
That part didn't change.
So, the CL here implements a version that doesn't update the counters but does digest feedback and follow through on it. I find this combination a bit strange.
Agreed. But AFAIU we shouldn't update counters/digest feedback here because `global_pretenuring_feedback_` is always empty (we don't invoke MergeAllocationSitePretenuringFeedback during full GCs).
I added a DCHECK for this.
Related details
- Dominik Inführ
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
Dominik InführPretenuring is still happening on full GCs IIUC: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=2422?q=ProcessPretenuringFeedback&ss=chromium
That part didn't change.
So, the CL here implements a version that doesn't update the counters but does digest feedback and follow through on it. I find this combination a bit strange.
Dominik InführAgreed. But AFAIU we shouldn't update counters/digest feedback here because `global_pretenuring_feedback_` is always empty (we don't invoke MergeAllocationSitePretenuringFeedback during full GCs).
I added a DCHECK for this.
Sorry, added that comment before I saw yours. I added a DCHECK which ensures that `global_pretenuring_feedback_` is empty on full GC. We could skip it altogether as well though. I just updated the CL.
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
2 comments
explicit EvacuateNewSpaceVisitor(Heap* heap,not need for explicit
Related details
- Dominik Inführ
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 4 comments![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Commit-Queue | +2 |
Dominik InführPretenuring is still happening on full GCs IIUC: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=2422?q=ProcessPretenuringFeedback&ss=chromium
That part didn't change.
So, the CL here implements a version that doesn't update the counters but does digest feedback and follow through on it. I find this combination a bit strange.
Dominik InführAgreed. But AFAIU we shouldn't update counters/digest feedback here because `global_pretenuring_feedback_` is always empty (we don't invoke MergeAllocationSitePretenuringFeedback during full GCs).
Dominik InführI added a DCHECK for this.
Sorry, added that comment before I saw yours. I added a DCHECK which ensures that `global_pretenuring_feedback_` is empty on full GC. We could skip it altogether as well though. I just updated the CL.
Done
Bug: 463528049Dominik InführIs this really improving GC pause time?
Evacuation and ClearNonLiveReferences() should become cheaper. So I would say yes in general. The main reason I used this bug though is because the CL is part of the parallel clearing CL.
Done
explicit EvacuateNewSpaceVisitor(Heap* heap,Dominik Inführnot need for explicit
Done
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
14 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/heap/mark-compact.cc
Insertions: 2, Deletions: 3.
@@ -1742,9 +1742,8 @@
class EvacuateNewSpaceVisitor final : public EvacuateVisitorBase {
public:
- explicit EvacuateNewSpaceVisitor(Heap* heap,
- EvacuationAllocator* local_allocator,
- RecordMigratedSlotVisitor* record_visitor)
+ EvacuateNewSpaceVisitor(Heap* heap, EvacuationAllocator* local_allocator,
+ RecordMigratedSlotVisitor* record_visitor)
: EvacuateVisitorBase(heap, local_allocator, record_visitor),
promoted_size_(0),
is_incremental_marking_(heap->incremental_marking()->IsMarking()),
```
Change information
[heap] Remove pretenuring support in full GCs
In general the vast majority of GCs are Minor GCs, so not updating
AllocationSites during Major GCs shouldn't change pretenuring
heuristics much. Removing pretenuring support from Major GCs also
allows us to get rid of the Zombie state for AllocationSites. Without
the zombie state we also don't need to keep AllocationSites for
an additional GC cycle, so we do not need to mark AllocationSites
in ClearNonLiveReferences() anymore.
Removing the support for pretenuring in full GC doesn't break
resetting the pretenure decision at full GCs in
Heap::EvaluateOldSpaceLocalPretenuring. This is because this is
purely based on heap sizes before and after the full GC.
- M src/diagnostics/objects-printer.cc
- M src/heap/heap.cc
- M src/heap/mark-compact.cc
- M src/heap/pretenuring-handler-inl.h
- M src/heap/pretenuring-handler.cc
- M src/objects/allocation-site-inl.h
- M src/objects/allocation-site.h
- M src/objects/objects.cc
- M test/cctest/heap/test-heap.cc
- M test/cctest/test-heap-profiler.cc
- M test/mjsunit/compiler/deopt-pretenure.js
Code-Review: +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |