From 0d7f717a9204d063055338edacf45633a2fbc1d6 Mon Sep 17 00:00:00 2001 From: "Gang Zhao (Hermes)" Date: Thu, 21 Nov 2024 11:14:37 -0800 Subject: [PATCH] Make SHSegmentInfo explicit in CardTable (#1505) Summary: Pull Request resolved: https://github.com/facebook/hermes/pull/1505 Currently `SHSegmentInfo` lives in the prefix of CardTable inline storage (to be specific, prefix of the `cards_` array). But this is only defined in one comment. Add it into a union with the `cards_` array to make it clear. It also simplifies the reasoning of following diffs, in which we need to add more fields to `SHSegmentInfo`. In addition, `kFirstUsedIndex` should take into account of the size of `SHSegmentInfo`, since the size of `SHSegmentInfo` could be larger than `(2 * kCardTableSize) >> kLogCardSize)` for small segment size. Differential Revision: D61747499 --- include/hermes/VM/CardTableNC.h | 42 +++++++++++++++---------- unittests/VMRuntime/CardTableNCTest.cpp | 7 +++-- 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/include/hermes/VM/CardTableNC.h b/include/hermes/VM/CardTableNC.h index 2d53791ef91..67f120051bf 100644 --- a/include/hermes/VM/CardTableNC.h +++ b/include/hermes/VM/CardTableNC.h @@ -77,21 +77,24 @@ class CardTable { /// guaranteed by a static_assert below. static constexpr size_t kHeapBytesPerCardByte = kCardSize; - /// A prefix of every segment is occupied by auxilary data - /// structures. The card table is the first such data structure. - /// The card table maps to the segment. Only the suffix of the card - /// table that maps to the suffix of entire segment that is used for - /// allocation is ever used; the prefix that maps to the card table - /// itself is not used. (Nor is the portion that of the card table - /// that maps to the other auxiliary data structure, the mark bit - /// array, but we don't attempt to calculate that here.) - /// It is useful to know the size of this unused region of - /// the card table, so it can be used for other purposes. - /// Note that the total size of the card table is 2 times - /// kCardTableSize, since the CardTable contains two byte arrays of - /// that size (cards_ and _boundaries_). + /// A prefix of every segment is occupied by auxiliary data structures. The + /// card table is the first such data structure. The card table maps to the + /// segment. Only the suffix of the card table that maps to the suffix of + /// entire segment that is used for allocation is ever used; the prefix that + /// maps to the card table itself is not used, nor is the portion of the card + /// table that maps to the other auxiliary data structure: the mark bit array + /// and guard pages. This small space can be used for other purpose, such as + /// storing the SHSegmentInfo (we assert in AlignedHeapSegment that its + /// size won't exceed this unused space). The actual first used index should + /// take into account all these structures. Here we only calculate for + /// CardTable and size of SHSegmentInfo. It's only used as starting index for + /// clearing/dirtying range of bits. + /// Note that the total size of the card table is 2 times kCardTableSize, + /// since the CardTable contains two byte arrays of that size (cards_ and + /// boundaries_). And this index must be larger than the size of SHSegmentInfo + /// to avoid corrupting it when clearing/dirtying bits. static constexpr size_t kFirstUsedIndex = - (2 * kCardTableSize) >> kLogCardSize; + std::max(sizeof(SHSegmentInfo), (2 * kCardTableSize) >> kLogCardSize); CardTable() = default; /// CardTable is not copyable or movable: It must be constructed in-place. @@ -255,9 +258,14 @@ class CardTable { void cleanOrDirtyRange(size_t from, size_t to, CardStatus cleanOrDirty); - /// This needs to be atomic so that the background thread in Hades can safely - /// dirty cards when compacting. - std::array, kCardTableSize> cards_{}; + union { + /// The bytes occupied by segmentInfo_ are guaranteed to be not override by + /// writes to cards_ array. See static assertions in AlignedHeapSegment. + SHSegmentInfo segmentInfo_; + /// This needs to be atomic so that the background thread in Hades can + /// safely dirty cards when compacting. + std::array, kCardTableSize> cards_{}; + }; /// See the comment at kHeapBytesPerCardByte above to see why this is /// necessary. diff --git a/unittests/VMRuntime/CardTableNCTest.cpp b/unittests/VMRuntime/CardTableNCTest.cpp index 22ddfc8a82c..320cb677eb0 100644 --- a/unittests/VMRuntime/CardTableNCTest.cpp +++ b/unittests/VMRuntime/CardTableNCTest.cpp @@ -58,9 +58,10 @@ void CardTableNCTest::dirtyRangeTest( CardTableNCTest::CardTableNCTest() { // For purposes of this test, we'll assume the first writeable byte of - // the segment comes just after the card table (which is at the - // start of the segment). - auto first = seg.lowLim() + sizeof(CardTable); + // the segment comes just after the memory region that can be mapped by + // kFirstUsedIndex bytes. + auto first = seg.lowLim() + + CardTable::kFirstUsedIndex * CardTable::kHeapBytesPerCardByte; auto last = reinterpret_cast(llvh::alignDown( reinterpret_cast(seg.hiLim() - 1), CardTable::kCardSize));