[heap] Annotate reason for finishing sweeping [v8/v8 : main]
Dominik Inführ (Gerrit)
Dominik Inführ added 5 comments![]( "Open in Gerrit")
void AtLeast(size_t new_min_old_gen_limit, size_t new_min_global_limit) {This method and the next one are new. I am adding a test for them as well. But we don't use them yet.
UpdateAllocationLimits(LimitBounds::AtMostCurrentLimits(this),Drive-by: Use different caller to make it obvious whether UpdateAllocationLimits was called from major or minor GC.
TRACE_GC_EPOCH_WITH_FLOW(tracer(), GCTracer::Scope::MC_COMPLETE_SWEEPING,This event already exists within the sweeper. That's why I remove it here and instead add ENSURE_SWEEPING_COMPLETED at the beginning of the method.
!using_initial_limit()) {In a subsequent CL we can then disable calling UpdateAllocationLimits() based on CompleteSweepingReason.
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 6 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
6 comments
enum class CompleteSweepingReason {nit: Some enums are easier to use when not being nested inside `Heap`. We already have a bunch of them and maybe this one should also go outside. (Judge yourself)
void AtLeast(size_t new_min_old_gen_limit, size_t new_min_global_limit) {This method and the next one are new. I am adding a test for them as well. But we don't use them yet.
Acknowledged
UpdateAllocationLimits(LimitBounds::AtMostCurrentLimits(this),Drive-by: Use different caller to make it obvious whether UpdateAllocationLimits was called from major or minor GC.
Shouldn't you add a string above then as well? So either adjust both or none?
TRACE_GC_EPOCH_WITH_FLOW(tracer(), GCTracer::Scope::MC_COMPLETE_SWEEPING,This event already exists within the sweeper. That's why I remove it here and instead add ENSURE_SWEEPING_COMPLETED at the beginning of the method.
Acknowledged
if (v8_flags.trace_gc_verbose) {[[unlikely]]
!using_initial_limit()) {In a subsequent CL we can then disable calling UpdateAllocationLimits() based on CompleteSweepingReason.
- 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 3 comments![]( "Open in Gerrit")
nit: Some enums are easier to use when not being nested inside `Heap`. We already have a bunch of them and maybe this one should also go outside. (Judge yourself)
Yes absolutely. I like it.
UpdateAllocationLimits(LimitBounds::AtMostCurrentLimits(this),Michael LippautzDrive-by: Use different caller to make it obvious whether UpdateAllocationLimits was called from major or minor GC.
Shouldn't you add a string above then as well? So either adjust both or none?
So far both call sites used the default value for this method ("RecomputeLimits" - the default values uses __builtin_FUNCTION() or so). It was just a bit confusing to me that both minor and major GC used the same reason - so renaming just one is enough for me to distinguish them. But I might be missing something here.
if (v8_flags.trace_gc_verbose) {Dominik Inführ[[unlikely]]
Done
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
1 comment
UpdateAllocationLimits(LimitBounds::AtMostCurrentLimits(this),Michael LippautzDrive-by: Use different caller to make it obvious whether UpdateAllocationLimits was called from major or minor GC.
Dominik InführShouldn't you add a string above then as well? So either adjust both or none?
So far both call sites used the default value for this method ("RecomputeLimits" - the default values uses __builtin_FUNCTION() or so). It was just a bit confusing to me that both minor and major GC used the same reason - so renaming just one is enough for me to distinguish them. But I might be missing something here.
You can distinguish but it's not consistent in the terms of strings :)
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")
UpdateAllocationLimits(LimitBounds::AtMostCurrentLimits(this),Michael LippautzDrive-by: Use different caller to make it obvious whether UpdateAllocationLimits was called from major or minor GC.
Dominik InführShouldn't you add a string above then as well? So either adjust both or none?
Michael LippautzSo far both call sites used the default value for this method ("RecomputeLimits" - the default values uses __builtin_FUNCTION() or so). It was just a bit confusing to me that both minor and major GC used the same reason - so renaming just one is enough for me to distinguish them. But I might be missing something here.
You can distinguish but it's not consistent in the terms of strings :)
Ah right, good point. Updated it.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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
8 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/heap/heap.cc
Insertions: 2, Deletions: 1.
@@ -2665,7 +2665,8 @@
void Heap::RecomputeLimits(GarbageCollector collector) {
if (collector == GarbageCollector::MARK_COMPACTOR) {
- const LimitsComputationResult new_limits = UpdateAllocationLimits({});
+ const LimitsComputationResult new_limits =
+ UpdateAllocationLimits({}, "RecomputeLimitsAfterMajorGC");
if (v8_flags.memory_balancer) {
// Now recompute the new allocation limit.
```
Change information
[heap] Annotate reason for finishing sweeping
This CL adds a new enum CompleteSweepingReason with all reasons for
invoking EnsureSweepingCompleted(). It is also added as an additional
argument to that method.
The CL does not change any GC behavior yet. This will be done in a
follow-up CL.
- M src/execution/isolate.h
- M src/heap/gc-tracer-inl.h
- M src/heap/gc-tracer.h
- M src/heap/heap-verifier.cc
- M src/heap/heap.cc
- M src/heap/heap.h
- M src/heap/paged-spaces.cc
- M src/heap/read-only-heap.cc
- M src/init/heap-symbols.h
- M src/profiler/heap-snapshot-generator.cc
- M test/cctest/heap/heap-utils.cc
- M test/cctest/heap/test-compaction.cc
- M test/cctest/heap/test-heap.cc
- M test/cctest/heap/test-mark-compact.cc
- M test/cctest/test-shared-strings.cc
- M test/unittests/heap/conservative-stack-visitor-unittest.cc
- M test/unittests/heap/cppgc-js/unified-heap-unittest.cc
- M test/unittests/heap/heap-unittest.cc
- M test/unittests/heap/heap-utils.cc
- M test/unittests/heap/heap-utils.h
Code-Review: +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |