[heap] Simplify GenerationSizesFromHeapSize [v8/v8 : main]
Dominik Inführ (Gerrit)
Dominik Inführ added 2 comments![]( "Open in Gerrit")
uint64_t physical_memory, size_t heap_size);Would it be better to keep size_t here for heap size? I guess it would make sense that one can't use a heap size larger than the address space?
uint64_t heap_size;Now that we don't test 4GB and 8GB on 32-bit: can we make them size_t?
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")
uint64_t physical_memory, size_t heap_size);Would it be better to keep size_t here for heap size? I guess it would make sense that one can't use a heap size larger than the address space?
This was needed for ExpectedDefaultGenerationLimitsForPhysicalMemory.
I think physical_memory can be bigger than 4Gb on 32bit devices, then we need to make sure `physical_memory / kPhysicalMemoryToOldGenerationRatio` doesn't overflow.
At the end this is capped by DefaultMaxSemiSpaceSize, but if YoungGenerationSizeFromHeapSize takes a size_t, then I need to cap before the conversion (I feel like that's a bit more complicated).
Now that we don't test 4GB and 8GB on 32-bit: can we make them size_t?
- 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")
uint64_t physical_memory, size_t heap_size);Etienne Pierre-DorayWould it be better to keep size_t here for heap size? I guess it would make sense that one can't use a heap size larger than the address space?
This was needed for ExpectedDefaultGenerationLimitsForPhysicalMemory.
I think physical_memory can be bigger than 4Gb on 32bit devices, then we need to make sure `physical_memory / kPhysicalMemoryToOldGenerationRatio` doesn't overflow.
At the end this is capped by DefaultMaxSemiSpaceSize, but if YoungGenerationSizeFromHeapSize takes a size_t, then I need to cap before the conversion (I feel like that's a bit more complicated).
So when looking at the two ConfigureDefaults methods we have on the API level, we use uint64_t for the physical/virtual memory but then size_t for the heap limits. I think this convention makes sense and we generally stick to these types also internally.
`physical_memory / kPhysicalMemoryToOldGenerationRatio` is certainly a good point though! Since we have to choose one evil here, I would rather have heap_size as size_t to be consistent. Just to see whether this makes sense, I have tested this change on your CL to not postpone landing your CL further.. see [0]. Capping that value is just one line and it keeps the complexity local to `YoungGenerationSizeFromPhysicalMemory`.
0: https://chromium-review.googlesource.com/c/v8/v8/+/7133541
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 2 comments![]( "Open in Gerrit")
PTAnL?
uint64_t physical_memory, size_t heap_size);Etienne Pierre-DorayWould it be better to keep size_t here for heap size? I guess it would make sense that one can't use a heap size larger than the address space?
Dominik InführThis was needed for ExpectedDefaultGenerationLimitsForPhysicalMemory.
I think physical_memory can be bigger than 4Gb on 32bit devices, then we need to make sure `physical_memory / kPhysicalMemoryToOldGenerationRatio` doesn't overflow.
At the end this is capped by DefaultMaxSemiSpaceSize, but if YoungGenerationSizeFromHeapSize takes a size_t, then I need to cap before the conversion (I feel like that's a bit more complicated).
So when looking at the two ConfigureDefaults methods we have on the API level, we use uint64_t for the physical/virtual memory but then size_t for the heap limits. I think this convention makes sense and we generally stick to these types also internally.
`physical_memory / kPhysicalMemoryToOldGenerationRatio` is certainly a good point though! Since we have to choose one evil here, I would rather have heap_size as size_t to be consistent. Just to see whether this makes sense, I have tested this change on your CL to not postpone landing your CL further.. see [0]. Capping that value is just one line and it keeps the complexity local to `YoungGenerationSizeFromPhysicalMemory`.
0: https://chromium-review.googlesource.com/c/v8/v8/+/7133541
- 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 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
1 comment
- Etienne Pierre-Doray
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")
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![]( "Open in Gerrit")
Change information
[heap] Simplify GenerationSizesFromHeapSize
This CL simplifies GenerationSizesFromHeapSize by using
YoungGenerationSizeFromHeapSize, directly on heap size instead of old generation.
This yields very similar young/old generation sizes, but
avoids binary searching and gives cleaner young/old gen sizes.
This doesn't affect YoungGenerationSizeFromPhysicalMemory which is what we use in production
- M src/heap/heap.cc
- M src/heap/heap.h
- M test/unittests/api/resource-constraints-unittest.cc
- M test/unittests/heap/heap-unittest.cc
Code-Review: +1 by Dominik Inführ
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |