[heap] Extract limit computation out of Heap [v8/v8 : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(0, max_old_generation_size() & (PageMetadata::kPageSize - 1));This is effectively a no-op, because max_old_generation_size() isn't initialized here.
But it causes problem because heap_limits_ isn't initialized either.
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")
HeapLimits* heap_limits() const { return heap_limits_.get(); }Can we name that just `limits()`? That would make accesses a bit short. We are already in Heap, so I think it's always clear from the context "limits" means "heap_limits".
bool Heap::AllocationLimitOvershotByFixedMargin(I see why you split that method up. Can we keep this method here for now though? IMHO splitting it up seems a bit worse than keeping it here entirely a-is (modulo adding limits()-> prefixes). I think the same applies to AllocationLimitOvershotByLargeMargin() as well and possibly other methods I haven't seen yet.
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 4 comments![]( "Open in Gerrit")
HeapLimits* heap_limits() const { return heap_limits_.get(); }Can we name that just `limits()`? That would make accesses a bit short. We are already in Heap, so I think it's always clear from the context "limits" means "heap_limits".
Done
// Ensure old_generation_size_ is a multiple of kPageSize.
DCHECK_EQ(0, max_old_generation_size() & (NormalPage::kPageSize - 1));I wanted to note: this is a no-op because max_old_generation_size = 0 here (uninitialized)
bool Heap::AllocationLimitOvershotByFixedMargin(I see why you split that method up. Can we keep this method here for now though? IMHO splitting it up seems a bit worse than keeping it here entirely a-is (modulo adding limits()-> prefixes). I think the same applies to AllocationLimitOvershotByLargeMargin() as well and possibly other methods I haven't seen yet.
Done for AllocationLimitOvershotByFixedMargin/ByLargeMargin
Not sure about EnsureMinimumRemainingAllocationLimit - I feel like conceptually it should mostly be in HeapLimits
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")
#include "include/v8-isolate.h"Do we really need that one? Maybe included by accident.
bool Heap::AllocationLimitOvershotByFixedMargin(Etienne Pierre-DorayI see why you split that method up. Can we keep this method here for now though? IMHO splitting it up seems a bit worse than keeping it here entirely a-is (modulo adding limits()-> prefixes). I think the same applies to AllocationLimitOvershotByLargeMargin() as well and possibly other methods I haven't seen yet.
Done for AllocationLimitOvershotByFixedMargin/ByLargeMargin
Not sure about EnsureMinimumRemainingAllocationLimit - I feel like conceptually it should mostly be in HeapLimits
I think this applies to all three methods. They all make sense in HeapLimits. But I also think it's more important to not split the methods. This makes it a lot more cumbersome to check out what they are doing. So I think we should keep them here for now and we can move them over later.
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. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
I tried to avoid circular dependancies as much as possible,Nit: spellcheck
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 4 comments![]( "Open in Gerrit")
PTAnL?
I tried to avoid circular dependancies as much as possible,Etienne Pierre-DorayNit: spellcheck
Done
#include "include/v8-isolate.h"Do we really need that one? Maybe included by accident.
`Isolate` is used in this file - it's transitively provided through src/heap/heap.h (though that's not IWYU). I guess we can also just forward declare it.
bool Heap::AllocationLimitOvershotByFixedMargin(Etienne Pierre-DorayI see why you split that method up. Can we keep this method here for now though? IMHO splitting it up seems a bit worse than keeping it here entirely a-is (modulo adding limits()-> prefixes). I think the same applies to AllocationLimitOvershotByLargeMargin() as well and possibly other methods I haven't seen yet.
Dominik InführDone for AllocationLimitOvershotByFixedMargin/ByLargeMargin
Not sure about EnsureMinimumRemainingAllocationLimit - I feel like conceptually it should mostly be in HeapLimits
I think this applies to all three methods. They all make sense in HeapLimits. But I also think it's more important to not split the methods. This makes it a lot more cumbersome to check out what they are doing. So I think we should keep them here for now and we can move them over later.
- 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. |