[heap] Funnel read-only allocations into read-only region [v8/v8 : main]
Michael Lippautz (Gerrit)
Michael Lippautz added 3 comments![]( "Open in Gerrit")
ptal
Leszek: Igor is out; I hope you can review this as well.
constexpr size_t kMinimumTrustedRangeSize = 512 * MB;This is merely used in a DCHECK.
constexpr size_t kMinimumCodeRangeSize = 64 * MB;I don't think there's any restrictions here to force us into using a 3-4MB code range?
Related details
- Dominik Inführ
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 5 comments![]( "Open in Gerrit")
constexpr Address kContiguousReadOnlySpaceMask =Nit: What I would also find helpful here is if you would give the bit/hex pattern for this variable with the e.g. 8MB defined in BUILD.gn.
static_assert(base::bits::IsPowerOfTwo(kContiguousReadOnlyReservationSize));Nit: I would add here an explanation that we need to carve out this region in all our 3 cages: trusted, code and main cage. Maybe also mention `HeapLayout::InReadOnlySpace()` why this is the case.
V8_CONTIGUOUS_COMPRESSED_RO_SPACE_SIZE_MB * 1024 * 1024;Nit: MB instead of 1024 * 1024?
constexpr size_t kMinimumCodeRangeSize = 64 * MB;I don't think there's any restrictions here to force us into using a 3-4MB code range?
I guess alternatively we could also use `kMinimumCodeRangeSize = (4 + V8_CONTIGUOUS_COMPRESSED_RO_SPACE_SIZE_MB) * MB`. This would keep the current minimum for uncompressed builds and would dynamically adapt when changing that variable as well.
Related details
- Leszek Swirski
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
if (page_allocator_->contains(cage_aligned_base)) {I think you need to check both start and end of the reserved region, in case the allocator doesn't include the base but does include the end of the reservation:
```
base
|--reserved--|
|----code space----|
```
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
constexpr size_t kMinimumCodeRangeSize = 64 * MB;Dominik InführI don't think there's any restrictions here to force us into using a 3-4MB code range?
I guess alternatively we could also use `kMinimumCodeRangeSize = (4 + V8_CONTIGUOUS_COMPRESSED_RO_SPACE_SIZE_MB) * MB`. This would keep the current minimum for uncompressed builds and would dynamically adapt when changing that variable as well.
We anyway usually reserve a bigger space than this so it's ok I think. Maybe non-chromium embedders will complain, in which case we may want this conditional on V8_CONTIGUOUS_COMPRESSED_RO_SPACE
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
| 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
| Commit-Queue | +1 |
6 comments
Nit: What I would also find helpful here is if you would give the bit/hex pattern for this variable with the e.g. 8MB defined in BUILD.gn.
Done
static_assert(base::bits::IsPowerOfTwo(kContiguousReadOnlyReservationSize));Nit: I would add here an explanation that we need to carve out this region in all our 3 cages: trusted, code and main cage. Maybe also mention `HeapLayout::InReadOnlySpace()` why this is the case.
Done
V8_CONTIGUOUS_COMPRESSED_RO_SPACE_SIZE_MB * 1024 * 1024;Nit: MB instead of 1024 * 1024?
Done
constexpr size_t kMinimumTrustedRangeSize = 512 * MB;This is merely used in a DCHECK.
Resolving
constexpr size_t kMinimumCodeRangeSize = 64 * MB;Dominik InführI don't think there's any restrictions here to force us into using a 3-4MB code range?
Leszek SwirskiI guess alternatively we could also use `kMinimumCodeRangeSize = (4 + V8_CONTIGUOUS_COMPRESSED_RO_SPACE_SIZE_MB) * MB`. This would keep the current minimum for uncompressed builds and would dynamically adapt when changing that variable as well.
We anyway usually reserve a bigger space than this so it's ok I think. Maybe non-chromium embedders will complain, in which case we may want this conditional on V8_CONTIGUOUS_COMPRESSED_RO_SPACE
Keeping this one static for now so that non-Chromium embedders will see the error and have a choice.
I think you need to check both start and end of the reserved region, in case the allocator doesn't include the base but does include the end of the reservation:
```
base
|--reserved--|
|----code space----|
```
- Leszek Swirski
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted Code-Review+1![]( "Open in Gerrit")
- Michael Lippautz
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
| 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
16 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/common/globals.h
Insertions: 0, Deletions: 1.
@@ -772,7 +772,6 @@
constexpr Address kContiguousReadOnlySpaceMask =
(kPtrComprCageBaseAlignment - 1) ^ (kContiguousReadOnlyReservationSize - 1);
// Bound the worst case consumption of contiguous RO space across the various
-constexpr Address a = kContiguousReadOnlyReservationSize - 1;
// cages/regions.
static_assert(kMinimumTrustedRangeSize >= 512 * MB);
static_assert(!kPlatformRequiresCodeRange || kMinimumCodeRangeSize >= 64 * MB);
```
```
The name of the file: src/heap/code-range.cc
Insertions: 4, Deletions: 1.
@@ -250,7 +250,10 @@
//
// |-- No objects must be allocated here --|
// ^ data_cage_ro_start ^ data_cage_ro_end
- //
+
+ // The checks below only work when the code range is not fully contained
+ // within the aligned region.
+ CHECK_GE(size(), kMinimumCodeRangeSize);
const Address data_cage_ro_start = base() % kPtrComprCageBaseAlignment;
const Address data_cage_ro_end =
base() % kPtrComprCageBaseAlignment + kContiguousReadOnlyReservationSize;
```
Change information
[heap] Funnel read-only allocations into read-only region
For configuration that use pointer compression and relies on a shared
single data cage, which is the default in Chrome on 64-bit, this CL
carves out an explicit contiguous memory region for read-only pages.
Additionally, we also make sure that the same region is not used by
the TrustedRange and CodeRange. As a consequence we can check for
read-only objects by merely inspecting the object's address.
The change here does not remove the page flag yet but merely rewire
the allocations and use the condition in a DCHECK.
In a follow up this property can be used for:
- Checking for RO objects locally which is faster than checking page
flags. It can also be combined with a partial Smi check (when we
know that we are dealing with non-InstructionStream slots).
- Moving the RO page bit away from MemoryChunk.
A side effect is that this also filters out (more) small offsets for
conservative stack scanning as the contiguous region is larger than
the current static roots.
- M BUILD.gn
- M gni/v8.gni
- M src/common/globals.h
- M src/heap/code-range.cc
- M src/heap/heap-layout-inl.h
- M src/heap/memory-allocator.cc
- M src/heap/memory-allocator.h
- M src/heap/read-only-spaces.cc
- M src/heap/trusted-range.cc
- M src/init/isolate-group.cc
- M src/init/isolate-group.h
Code-Review: +1 by Leszek Swirski, +1 by Dominik Inführ
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Achenbach (Gerrit)
Michael Achenbach has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |