[Heap] Unify allocation retry loops [v8/v8 : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +1 |
1 comment
Related details
- Dominik Inführ
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 added 2 comments![]( "Open in Gerrit")
Thanks, this is indeed much cleaner! I just have one comment.
V8_WARN_UNUSED_RESULT auto HeapAllocator::CustomAllocateWithRetry(I understand that this was not introduced by your CL but since we are adapting this once more now and I wanted to do this for a while: What about making this non-templatized so we can easily put the retry logic back into the heap-allocator.cc file instead? We could simply expect `base::FunctionRef<bool()>` as allocation function. The bool return value would just tell us whether the allocation was successful or not. The actual allocation address could also be written into some captured local variable by the allocate-lambda. A non-templatized version could probably also be exposed a bit more easily on the API (since you mentioned that).
Performance-wise this shouldn't matter because in `HeapAllocator::AllocateRaw` we already use that method only after the allocation failed already once. At this point we need to do a GC, so inling or not for that function shouldn't matter at all. We should probably invoke "allocate()" manually first before using `CustomAllocateWithRetry()` in the other uses as well to handle the most likely case (that allocation succeeds) without regression. This should be: AllocateExternalBackingStore(), CppHeap::AllocateWithRetry and NewJSDispatchHandle.
Related details
- Etienne Pierre-Doray
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 3 comments![]( "Open in Gerrit")
if (allocation == AllocationType::kYoung) {
result = AllocateRaw<AllocationType::kYoung>(size, origin, alignment, hint);
if (result.To(&object)) {
return object;
}
} else if (allocation == AllocationType::kOld) {
result = AllocateRaw<AllocationType::kOld>(size, origin, alignment, hint);
if (result.To(&object)) {
return object;
}
}It seems to me like this + all the "other" allocation can simply be replaced by `AllocateRaw()` without regression -> this doesn't add any condition and AllocateRaw() is inline.
V8_WARN_UNUSED_RESULT auto HeapAllocator::CustomAllocateWithRetry(I understand that this was not introduced by your CL but since we are adapting this once more now and I wanted to do this for a while: What about making this non-templatized so we can easily put the retry logic back into the heap-allocator.cc file instead? We could simply expect `base::FunctionRef<bool()>` as allocation function. The bool return value would just tell us whether the allocation was successful or not. The actual allocation address could also be written into some captured local variable by the allocate-lambda. A non-templatized version could probably also be exposed a bit more easily on the API (since you mentioned that).
Performance-wise this shouldn't matter because in `HeapAllocator::AllocateRaw` we already use that method only after the allocation failed already once. At this point we need to do a GC, so inling or not for that function shouldn't matter at all. We should probably invoke "allocate()" manually first before using `CustomAllocateWithRetry()` in the other uses as well to handle the most likely case (that allocation succeeds) without regression. This should be: AllocateExternalBackingStore(), CppHeap::AllocateWithRetry and NewJSDispatchHandle.
We could simply expect base::FunctionRef<bool()> as allocation function.
Done
Performance-wise this shouldn't matter because in HeapAllocator::AllocateRaw we already use that method only after the allocation failed already once.
This is more or less true, because only kYoung and kOld have a fast path; although I don't see why we couldn't add the fast path for all allocation types (done in latest patchset).
We should probably invoke "allocate()" manually first before using CustomAllocateWithRetry() in the other uses as well
- Dominik Inführ
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 and added 2 comments![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
2 comments
if (allocation == AllocationType::kYoung) {
result = AllocateRaw<AllocationType::kYoung>(size, origin, alignment, hint);
if (result.To(&object)) {
return object;
}
} else if (allocation == AllocationType::kOld) {
result = AllocateRaw<AllocationType::kOld>(size, origin, alignment, hint);
if (result.To(&object)) {
return object;
}
}It seems to me like this + all the "other" allocation can simply be replaced by `AllocateRaw()` without regression -> this doesn't add any condition and AllocateRaw() is inline.
Good catch! I missed that we have the fast path only for Young and Old. I think I agree with you but IIRC that code path was added for performance reasons. Not really sure what benchmark that was. I think it would be less controversial if we would keep it as is but add the AllocateRaw in an else-branch after handling Young/Old.
Related details
- Etienne Pierre-Doray
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
if (allocation == AllocationType::kYoung) {
result = AllocateRaw<AllocationType::kYoung>(size, origin, alignment, hint);
if (result.To(&object)) {
return object;
}
} else if (allocation == AllocationType::kOld) {
result = AllocateRaw<AllocationType::kOld>(size, origin, alignment, hint);
if (result.To(&object)) {
return object;
}
}Dominik InführIt seems to me like this + all the "other" allocation can simply be replaced by `AllocateRaw()` without regression -> this doesn't add any condition and AllocateRaw() is inline.
Good catch! I missed that we have the fast path only for Young and Old. I think I agree with you but IIRC that code path was added for performance reasons. Not really sure what benchmark that was. I think it would be less controversial if we would keep it as is but add the AllocateRaw in an else-branch after handling Young/Old.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray 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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray 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
15 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-allocator-inl.h
Insertions: 16, Deletions: 4.
@@ -225,12 +225,24 @@
AllocationOrigin origin,
AllocationAlignment alignment,
AllocationHint hint) {
+ AllocationResult result;
Tagged<HeapObject> object;
size = ALIGN_TO_ALLOCATION_ALIGNMENT(size);
- AllocationResult result =
- AllocateRaw(size, allocation, origin, alignment, hint);
- if (result.To(&object)) [[likely]] {
- return object;
+ if (allocation == AllocationType::kYoung) {
+ result = AllocateRaw<AllocationType::kYoung>(size, origin, alignment, hint);
+ if (result.To(&object)) {
+ return object;
+ }
+ } else if (allocation == AllocationType::kOld) {
+ result = AllocateRaw<AllocationType::kOld>(size, origin, alignment, hint);
+ if (result.To(&object)) {
+ return object;
+ }
+ } else {
+ result = AllocateRaw(size, allocation, origin, alignment, hint);
+ if (result.To(&object)) {
+ return object;
+ }
}
switch (mode) {
case kLightRetry:
```
Change information
[Heap] Unify allocation retry loops
This CL reuses HeapAllocator retry loop in several places, including
oilpan. This ensure they are all aligned under 1 implementation.
Follow-up: this should also be exposed on v8-isolate so that it can be used here: https://chromium-review.googlesource.com/c/chromium/src/+/6164814
- M src/heap/cppgc-js/cpp-heap.cc
- M src/heap/cppgc-js/cpp-heap.h
- M src/heap/cppgc/garbage-collector.h
- M src/heap/cppgc/gc-invoker.cc
- M src/heap/cppgc/gc-invoker.h
- M src/heap/cppgc/heap.cc
- M src/heap/cppgc/heap.h
- M src/heap/cppgc/object-allocator.cc
- M src/heap/factory-base.cc
- M src/heap/heap-allocator-inl.h
- M src/heap/heap-allocator.cc
- M src/heap/heap-allocator.h
- M src/heap/heap.cc
- M test/unittests/heap/cppgc/gc-invoker-unittest.cc
- M test/unittests/heap/cppgc/heap-growing-unittest.cc
Code-Review: +1 by Dominik Inführ
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benoit Lize (Gerrit)
Benoit Lize has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |