[scudo] Make the boundary of memory group aligned with region begin

[Chia-hungDuan]

Summary

This alignment guarantee enables simpler group range check while page
releasing and a potential optimization which is, now all the pointers
from the same group are also inth same region, that means the complexity
in markFreeBlocks() can be reduced as well.

[Chia-hungDuan]

Add one more context, on SizeClassAllocator64, now the pointer always gets compacted at least by using the offset from the region begin. This means the PrimaryCompactPtrT is able to use u32 in DefaultConfig as well. This will be done in later metadata size reduction work.

[cferris]

compiler-rt/lib/scudo/standalone/primary32.h

117

Would it be better to name this compactPtrGroupBase? And use Base instead of Beg for all of the variable names?

You use base in a few other places, and I think it makes it easier to understand the code since you are currently using both beg and base and sometimes comparing the values.

121–124

Since this function doesn't do anything, do you need it any more? Or do you want to keep it so that the primary32 and primary64 are the same?

[Chia-hungDuan]

1. Address review comment
2. Restrict (GroupSize == RegionSize) in SizeClassAllocator32 so that the logic of page releasing will be easier

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/primary32.h

117

Agree, done!

121–124

Right, it tries to align the style with primary64

[cferris]

Only a couple of nits.

compiler-rt/lib/scudo/standalone/local_cache.h

65–66

base

compiler-rt/lib/scudo/standalone/primary32.h

360

What is RegionBeg? I think the names have changed so I'm not sure if this should be RegionSize instead?

[Chia-hungDuan]

Address review comment

compiler-rt/lib/scudo/standalone/primary32.h

360

Oops, forgot to update the log. Done.

[cferris]

LGTM

[Chia-hungDuan]

Rebase only

compiler-rt/lib/scudo/standalone/local_cache.h

private:
     u16 Count;
   };
 
   // A BatchGroup is used to collect blocks. Each group has a group id to
   // identify the group kind of contained blocks.
   struct BatchGroup {
     // `Next` is used by IntrusiveList.
     BatchGroup *Next;
-    // The identifier of each group
-    uptr GroupId;
+    // The compact begin address of each group
+    uptr CompactPtrGroupBeg;
     // Cache value of TransferBatch::getMaxCached()
     u16 MaxCachedPerBatch;
     // Number of blocks pushed into this group. This is an increment-only
     // counter.
     uptr PushedBlocks;
     // This is used to track how many blocks are pushed since last time we
     // checked `PushedBlocks`. It's useful for page releasing to determine the
     // usage of a BatchGroup.

compiler-rt/lib/scudo/standalone/primary32.h

public:
   CompactPtrT compactPtr(UNUSED uptr ClassId, uptr Ptr) const {
     return static_cast<CompactPtrT>(Ptr);
   }
 
   void *decompactPtr(UNUSED uptr ClassId, CompactPtrT CompactPtr) const {
     return reinterpret_cast<void *>(static_cast<uptr>(CompactPtr));
   }
 
   uptr compactPtrGroup(CompactPtrT CompactPtr) {
-    return CompactPtr >> GroupSizeLog;
+    const uptr Mask = (static_cast<uptr>(1) << GroupSizeLog) - 1;
+    return CompactPtr & ~Mask;
   }
 
-  uptr batchGroupBase(uptr GroupId) { return GroupId << GroupSizeLog; }
+  uptr decompactGroupBase(uptr CompactPtrGroupBeg) {
+    return CompactPtrGroupBeg;
+  }
 
   TransferBatch *popBatch(CacheT *C, uptr ClassId) {
     DCHECK_LT(ClassId, NumClasses);
     SizeClassInfo *Sci = getSizeClassInfo(ClassId);
     ScopedLock L(Sci->Mutex);
     TransferBatch *B = popBatchImpl(C, ClassId, Sci);
     if (UNLIKELY(!B)) {
       if (UNLIKELY(!populateFreeList(C, ClassId, Sci)))
uptr allocateRegionSlow() {
     } else {
       Region = roundUpTo(MapBase, RegionSize);
       unmap(reinterpret_cast<void *>(MapBase), Region - MapBase);
       MapSize = RegionSize;
     }
     const uptr End = Region + MapSize;
     if (End != MapEnd)
       unmap(reinterpret_cast<void *>(End), MapEnd - End);
+
+    DCHECK_EQ(Region % RegionSize, 0U);
+    static_assert(Config::PrimaryRegionSizeLog >= GroupSizeLog,
+                  "Memory group should be aligned with RegionBeg (which is "
+                  "aligned with RegionSize)");
+
     return Region;
   }
 
   uptr allocateRegion(SizeClassInfo *Sci, uptr ClassId) REQUIRES(Sci->Mutex) {
     DCHECK_LT(ClassId, NumClasses);
     uptr Region = 0;
     {
       ScopedLock L(RegionsStashMutex);
private:
   // same group then we will skip checking the group id of each block.
   //
   // The region mutex needs to be held while calling this method.
   void pushBlocksImpl(CacheT *C, uptr ClassId, SizeClassInfo *Sci,
                       CompactPtrT *Array, u32 Size, bool SameGroup = false)
       REQUIRES(Sci->Mutex) {
     DCHECK_GT(Size, 0U);
 
-    auto CreateGroup = [&](uptr GroupId) {
+    auto CreateGroup = [&](uptr CompactPtrGroupBeg) {
       BatchGroup *BG = nullptr;
       TransferBatch *TB = nullptr;
       if (ClassId == SizeClassMap::BatchClassId) {
         DCHECK_GE(Size, 2U);
 
         // Free blocks are recorded by TransferBatch in freelist, blocks of
         // BatchClassId are included. In order not to use additional memory to
         // record blocks of BatchClassId, they are self-contained. I.e., A
auto CreateGroup = [&](uptr CompactPtrGroupBeg) {
       } else {
         BG = C->createGroup();
         BG->Batches.clear();
 
         TB = C->createBatch(ClassId, nullptr);
         TB->clear();
       }
 
-      BG->GroupId = GroupId;
+      BG->CompactPtrGroupBeg = CompactPtrGroupBeg;
       // TODO(chiahungduan): Avoid the use of push_back() in `Batches`.
       BG->Batches.push_front(TB);
       BG->PushedBlocks = 0;
       BG->PushedBlocksAtLastCheckpoint = 0;
       BG->MaxCachedPerBatch =
           TransferBatch::getMaxCached(getSizeByClassId(ClassId));
 
       return BG;
auto InsertBlocks = [&](BatchGroup *BG, CompactPtrT *Array, u32 Size) {
       BG->PushedBlocks += Size;
     };
 
     BatchGroup *Cur = Sci->FreeList.front();
 
     if (ClassId == SizeClassMap::BatchClassId) {
       if (Cur == nullptr) {
         // Don't need to classify BatchClassId.
-        Cur = CreateGroup(/*GroupId=*/0);
+        Cur = CreateGroup(/*CompactPtrGroupBeg=*/0);
         Sci->FreeList.push_front(Cur);
       }
       InsertBlocks(Cur, Array, Size);
       return;
     }
 
     // In the following, `Cur` always points to the BatchGroup for blocks that
     // will be pushed next. `Prev` is the element right before `Cur`.
     BatchGroup *Prev = nullptr;
 
-    while (Cur != nullptr && compactPtrGroup(Array[0]) > Cur->GroupId) {
+    while (Cur != nullptr &&
+           compactPtrGroup(Array[0]) > Cur->CompactPtrGroupBeg) {
       Prev = Cur;
       Cur = Cur->Next;
     }
 
-    if (Cur == nullptr || compactPtrGroup(Array[0]) != Cur->GroupId) {
+    if (Cur == nullptr ||
+        compactPtrGroup(Array[0]) != Cur->CompactPtrGroupBeg) {
       Cur = CreateGroup(compactPtrGroup(Array[0]));
       if (Prev == nullptr)
         Sci->FreeList.push_front(Cur);
       else
         Sci->FreeList.insert(Prev, Cur);
     }
 
     // All the blocks are from the same group, just push without checking group
     // id.
     if (SameGroup) {
       for (u32 I = 0; I < Size; ++I)
-        DCHECK_EQ(compactPtrGroup(Array[I]), Cur->GroupId);
+        DCHECK_EQ(compactPtrGroup(Array[I]), Cur->CompactPtrGroupBeg);
 
       InsertBlocks(Cur, Array, Size);
       return;
     }
 
     // The blocks are sorted by group id. Determine the segment of group and
     // push them to their group together.
     u32 Count = 1;
     for (u32 I = 1; I < Size; ++I) {
       if (compactPtrGroup(Array[I - 1]) != compactPtrGroup(Array[I])) {
-        DCHECK_EQ(compactPtrGroup(Array[I - 1]), Cur->GroupId);
+        DCHECK_EQ(compactPtrGroup(Array[I - 1]), Cur->CompactPtrGroupBeg);
         InsertBlocks(Cur, Array + I - Count, Count);
 
-        while (Cur != nullptr && compactPtrGroup(Array[I]) > Cur->GroupId) {
+        while (Cur != nullptr &&
+               compactPtrGroup(Array[I]) > Cur->CompactPtrGroupBeg) {
           Prev = Cur;
           Cur = Cur->Next;
         }
 
-        if (Cur == nullptr || compactPtrGroup(Array[I]) != Cur->GroupId) {
+        if (Cur == nullptr ||
+            compactPtrGroup(Array[I]) != Cur->CompactPtrGroupBeg) {
           Cur = CreateGroup(compactPtrGroup(Array[I]));
           DCHECK_NE(Prev, nullptr);
           Sci->FreeList.insert(Prev, Cur);
         }
 
         Count = 1;
       } else {
         ++Count;
NOINLINE uptr releaseToOSMaybe(SizeClassInfo *Sci, uptr ClassId,
     const uptr First = Sci->MinRegionIndex;
     const uptr Last = Sci->MaxRegionIndex;
     DCHECK_NE(Last, 0U);
     DCHECK_LE(First, Last);
     uptr TotalReleasedBytes = 0;
     const uptr Base = First * RegionSize;
     const uptr NumberOfRegions = Last - First + 1U;
     const uptr GroupSize = (1U << GroupSizeLog);
-    const uptr CurRegionGroupId =
+    const uptr CurGroupBeg =
         compactPtrGroup(compactPtr(ClassId, Sci->CurrentRegion));
 
     ReleaseRecorder Recorder(Base);
     PageReleaseContext Context(BlockSize, RegionSize, NumberOfRegions,
                                /*ReleaseSize=*/RegionSize);
 
     auto DecompactPtr = [](CompactPtrT CompactPtr) {
       return reinterpret_cast<uptr>(CompactPtr);
     };
     for (BatchGroup &BG : Sci->FreeList) {
       const uptr PushedBytesDelta =
           BG.PushedBlocks - BG.PushedBlocksAtLastCheckpoint;
       if (PushedBytesDelta * BlockSize < PageSize)
         continue;
 
-      uptr AllocatedGroupSize = BG.GroupId == CurRegionGroupId
+      uptr AllocatedGroupSize = BG.CompactPtrGroupBeg == CurGroupBeg
                                     ? Sci->CurrentRegionAllocated
                                     : GroupSize;
       if (AllocatedGroupSize == 0)
         continue;
 
       // TransferBatches are pushed in front of BG.Batches. The first one may
       // not have all caches used.
       const uptr NumBlocks = (BG.Batches.size() - 1) * BG.MaxCachedPerBatch +
for (BatchGroup &BG : Sci->FreeList) {
       // range need to be from same region. In SizeClassAllocator32, this is
       // true when GroupSize and RegionSize are the same. Another tirck case is,
       // while doing range marking, the last block in a region needs the logic
       // to mark the last page. However, in SizeClassAllocator32, the RegionSize
       // recorded in PageReleaseContext may be different from
       // `CurrentRegionAllocated` of the current region. This exception excludes
       // the chance of doing range marking for the current region.
       const bool CanDoRangeMark =
-          GroupSize == RegionSize && BG.GroupId != CurRegionGroupId;
+          GroupSize == RegionSize && BG.CompactPtrGroupBeg != CurGroupBeg;
 
       if (!CanDoRangeMark || NumBlocks != MaxContainedBlocks) {
         if (CanDoRangeMark)
           DCHECK_LT(NumBlocks, MaxContainedBlocks);
 
         // Note that we don't always visit blocks in each BatchGroup so that we
         // may miss the chance of releasing certain pages that cross
         // BatchGroups.
         Context.markFreeBlocks(BG.Batches, DecompactPtr, Base);
       } else {
         for (const auto &It : BG.Batches)
           for (u16 I = 0; I < It.getCount(); ++I)
-            DCHECK_EQ(compactPtrGroup(It.get(I)), BG.GroupId);
+            DCHECK_EQ(compactPtrGroup(It.get(I)), BG.CompactPtrGroupBeg);
 
-        const uptr From = batchGroupBase(BG.GroupId);
-        const uptr To = batchGroupBase(BG.GroupId) + AllocatedGroupSize;
+        const uptr From = decompactGroupBase(BG.CompactPtrGroupBeg);
+        const uptr To = From + AllocatedGroupSize;
         Context.markRangeAsAllCounted(From, To, Base);
       }
     }
 
     if (!Context.hasBlockMarked())
       return 0;
 
     auto SkipRegion = [this, First, ClassId](uptr RegionIndex) {

compiler-rt/lib/scudo/standalone/primary64.h

 // The memory used by this allocator is never unmapped, but can be partially
 // released if the platform allows for it.
 
 template <typename Config> class SizeClassAllocator64 {
 public:
   typedef typename Config::PrimaryCompactPtrT CompactPtrT;
   static const uptr CompactPtrScale = Config::PrimaryCompactPtrScale;
   static const uptr GroupSizeLog = Config::PrimaryGroupSizeLog;
+  static const uptr GroupScale = GroupSizeLog - CompactPtrScale;
   typedef typename Config::SizeClassMap SizeClassMap;
   typedef SizeClassAllocator64<Config> ThisT;
   typedef SizeClassAllocatorLocalCache<ThisT> CacheT;
   typedef typename CacheT::TransferBatch TransferBatch;
   typedef typename CacheT::BatchGroup BatchGroup;
 
   static uptr getSizeByClassId(uptr ClassId) {
     return (ClassId == SizeClassMap::BatchClassId)
public:
 
   const char *getRegionInfoArrayAddress() const {
     return reinterpret_cast<const char *>(RegionInfoArray);
   }
 
   static uptr getRegionInfoArraySize() { return sizeof(RegionInfoArray); }
 
   uptr getCompactPtrBaseByClassId(uptr ClassId) {
-    // If we are not compacting pointers, base everything off of 0.
-    if (sizeof(CompactPtrT) == sizeof(uptr) && CompactPtrScale == 0)
-      return 0;
     return getRegionInfo(ClassId)->RegionBeg;
   }
 
   CompactPtrT compactPtr(uptr ClassId, uptr Ptr) {
     DCHECK_LE(ClassId, SizeClassMap::LargestClassId);
     return compactPtrInternal(getCompactPtrBaseByClassId(ClassId), Ptr);
   }
 
static CompactPtrT compactPtrInternal(uptr Base, uptr Ptr) {
     return static_cast<CompactPtrT>((Ptr - Base) >> CompactPtrScale);
   }
 
   static uptr decompactPtrInternal(uptr Base, CompactPtrT CompactPtr) {
     return Base + (static_cast<uptr>(CompactPtr) << CompactPtrScale);
   }
 
   static uptr compactPtrGroup(CompactPtrT CompactPtr) {
-    return static_cast<uptr>(CompactPtr) >> (GroupSizeLog - CompactPtrScale);
+    const uptr Mask = (static_cast<uptr>(1) << GroupScale) - 1;
+    return static_cast<uptr>(CompactPtr) & ~Mask;
   }
-  static uptr batchGroupBase(uptr Base, uptr GroupId) {
-    return (GroupId << GroupSizeLog) + Base;
+  static uptr decompactGroupBase(uptr Base, uptr CompactPtrGroupBeg) {
+    DCHECK_EQ(CompactPtrGroupBeg % (static_cast<uptr>(1) << (GroupScale)), 0U);
+    return Base + (CompactPtrGroupBeg << CompactPtrScale);
   }
 
   // Push the blocks to their batch group. The layout will be like,
   //
   // FreeList - > BG -> BG -> BG
   //              |     |     |
   //              v     v     v
   //              TB    TB    TB
private:
   // same group then we will skip checking the group id of each block.
   //
   // The region mutex needs to be held while calling this method.
   void pushBlocksImpl(CacheT *C, uptr ClassId, RegionInfo *Region,
                       CompactPtrT *Array, u32 Size, bool SameGroup = false)
       REQUIRES(Region->Mutex) {
     DCHECK_GT(Size, 0U);
 
-    auto CreateGroup = [&](uptr GroupId) {
+    auto CreateGroup = [&](uptr CompactPtrGroupBeg) {
       BatchGroup *BG = nullptr;
       TransferBatch *TB = nullptr;
       if (ClassId == SizeClassMap::BatchClassId) {
         DCHECK_GE(Size, 2U);
 
         // Free blocks are recorded by TransferBatch in freelist, blocks of
         // BatchClassId are included. In order not to use additional memory to
         // record blocks of BatchClassId, they are self-contained. I.e., A
auto CreateGroup = [&](uptr CompactPtrGroupBeg) {
       } else {
         BG = C->createGroup();
         BG->Batches.clear();
 
         TB = C->createBatch(ClassId, nullptr);
         TB->clear();
       }
 
-      BG->GroupId = GroupId;
+      BG->CompactPtrGroupBeg = CompactPtrGroupBeg;
       // TODO(chiahungduan): Avoid the use of push_back() in `Batches`.
       BG->Batches.push_front(TB);
       BG->PushedBlocks = 0;
       BG->PushedBlocksAtLastCheckpoint = 0;
       BG->MaxCachedPerBatch =
           TransferBatch::getMaxCached(getSizeByClassId(ClassId));
 
       return BG;
auto InsertBlocks = [&](BatchGroup *BG, CompactPtrT *Array, u32 Size) {
       BG->PushedBlocks += Size;
     };
 
     BatchGroup *Cur = Region->FreeList.front();
 
     if (ClassId == SizeClassMap::BatchClassId) {
       if (Cur == nullptr) {
         // Don't need to classify BatchClassId.
-        Cur = CreateGroup(/*GroupId=*/0);
+        Cur = CreateGroup(/*CompactPtrGroupBeg=*/0);
         Region->FreeList.push_front(Cur);
       }
       InsertBlocks(Cur, Array, Size);
       return;
     }
 
     // In the following, `Cur` always points to the BatchGroup for blocks that
     // will be pushed next. `Prev` is the element right before `Cur`.
     BatchGroup *Prev = nullptr;
 
-    while (Cur != nullptr && compactPtrGroup(Array[0]) > Cur->GroupId) {
+    while (Cur != nullptr &&
+           compactPtrGroup(Array[0]) > Cur->CompactPtrGroupBeg) {
       Prev = Cur;
       Cur = Cur->Next;
     }
 
-    if (Cur == nullptr || compactPtrGroup(Array[0]) != Cur->GroupId) {
+    if (Cur == nullptr ||
+        compactPtrGroup(Array[0]) != Cur->CompactPtrGroupBeg) {
       Cur = CreateGroup(compactPtrGroup(Array[0]));
       if (Prev == nullptr)
         Region->FreeList.push_front(Cur);
       else
         Region->FreeList.insert(Prev, Cur);
     }
 
     // All the blocks are from the same group, just push without checking group
     // id.
     if (SameGroup) {
       for (u32 I = 0; I < Size; ++I)
-        DCHECK_EQ(compactPtrGroup(Array[I]), Cur->GroupId);
+        DCHECK_EQ(compactPtrGroup(Array[I]), Cur->CompactPtrGroupBeg);
 
       InsertBlocks(Cur, Array, Size);
       return;
     }
 
     // The blocks are sorted by group id. Determine the segment of group and
     // push them to their group together.
     u32 Count = 1;
     for (u32 I = 1; I < Size; ++I) {
       if (compactPtrGroup(Array[I - 1]) != compactPtrGroup(Array[I])) {
-        DCHECK_EQ(compactPtrGroup(Array[I - 1]), Cur->GroupId);
+        DCHECK_EQ(compactPtrGroup(Array[I - 1]), Cur->CompactPtrGroupBeg);
         InsertBlocks(Cur, Array + I - Count, Count);
 
-        while (Cur != nullptr && compactPtrGroup(Array[I]) > Cur->GroupId) {
+        while (Cur != nullptr &&
+               compactPtrGroup(Array[I]) > Cur->CompactPtrGroupBeg) {
           Prev = Cur;
           Cur = Cur->Next;
         }
 
-        if (Cur == nullptr || compactPtrGroup(Array[I]) != Cur->GroupId) {
+        if (Cur == nullptr ||
+            compactPtrGroup(Array[I]) != Cur->CompactPtrGroupBeg) {
           Cur = CreateGroup(compactPtrGroup(Array[I]));
           DCHECK_NE(Prev, nullptr);
           Region->FreeList.insert(Prev, Cur);
         }
 
         Count = 1;
       } else {
         ++Count;
NOINLINE uptr releaseToOSMaybe(RegionInfo *Region, uptr ClassId,
     for (BatchGroup *BG = Region->FreeList.front(); BG != nullptr;) {
       const uptr PushedBytesDelta =
           BG->PushedBlocks - BG->PushedBlocksAtLastCheckpoint;
       if (PushedBytesDelta * BlockSize < PageSize) {
         Prev = BG, BG = BG->Next;
         continue;
       }
 
-      // Group boundary does not necessarily have the same alignment as Region.
-      // It may sit across a Region boundary. Which means that we may have the
-      // following two cases,
-      //
-      // 1. Group boundary sits before RegionBeg.
+      // Group boundary is always GroupSize-aligned from CompactPtr base. The
+      // layout of memory groups is like,
       //
-      //                (BatchGroupBeg)
-      // batchGroupBase  RegionBeg       BatchGroupEnd
-      //        |            |                |
-      //        v            v                v
-      //        +------------+----------------+
-      //         \                           /
-      //          ------   GroupSize   ------
-      //
-      // 2. Group boundary sits after RegionBeg.
+      //     (CompactPtrBase)
+      // #1 CompactPtrGroupBeg   #2 CompactPtrGroupBeg            ...
+      //           |                       |                       |
+      //           v                       v                       v
+      //           +-----------------------+-----------------------+
+      //            \                     / \                     /
+      //             ---   GroupSize   ---   ---   GroupSize   ---
       //
-      //               (BatchGroupBeg)
-      //    RegionBeg  batchGroupBase               BatchGroupEnd
-      //        |           |                             |
-      //        v           v                             v
-      //        +-----------+-----------------------------+
-      //                     \                           /
-      //                      ------   GroupSize   ------
-      //
-      // Note that in the first case, the group range before RegionBeg is never
-      // used. Therefore, while calculating the used group size, we should
-      // exclude that part to get the correct size.
+      // After decompacting the CompactPtrGroupBeg, we expect the alignment
+      // property is held as well.
       const uptr BatchGroupBeg =
-          Max(batchGroupBase(CompactPtrBase, BG->GroupId), Region->RegionBeg);
+          decompactGroupBase(CompactPtrBase, BG->CompactPtrGroupBeg);
+      DCHECK_LE(Region->RegionBeg, BatchGroupBeg);
       DCHECK_GE(AllocatedUserEnd, BatchGroupBeg);
-      const uptr BatchGroupEnd =
-          batchGroupBase(CompactPtrBase, BG->GroupId) + GroupSize;
+      DCHECK_EQ((Region->RegionBeg - BatchGroupBeg) % GroupSize, 0U);
+      const uptr BatchGroupEnd = BatchGroupBeg + GroupSize;
       const uptr AllocatedGroupSize = AllocatedUserEnd >= BatchGroupEnd
-                                          ? BatchGroupEnd - BatchGroupBeg
+                                          ? GroupSize
                                           : AllocatedUserEnd - BatchGroupBeg;
       if (AllocatedGroupSize == 0) {
         Prev = BG, BG = BG->Next;
         continue;
       }
 
       // TransferBatches are pushed in front of BG.Batches. The first one may
       // not have all caches used.
for (BatchGroup *BG = Region->FreeList.front(); BG != nullptr;) {
       else
         Region->FreeList.pop_front();
       GroupToRelease.push_back(Cur);
     }
 
     if (GroupToRelease.empty())
       return 0;
 
-    const uptr ReleaseBeg =
-        Max(batchGroupBase(CompactPtrBase, GroupToRelease.front()->GroupId),
-            Region->RegionBeg);
+    const uptr ReleaseBeg = decompactGroupBase(
+        CompactPtrBase, GroupToRelease.front()->CompactPtrGroupBeg);
     const uptr LastGroupEnd =
-        Min(batchGroupBase(CompactPtrBase, GroupToRelease.back()->GroupId) +
+        Min(decompactGroupBase(CompactPtrBase,
+                               GroupToRelease.back()->CompactPtrGroupBeg) +
                 GroupSize,
             AllocatedUserEnd);
     // The last block may straddle the group boundary. Rounding up to BlockSize
     // to get the exact range.
     const uptr ReleaseEnd =
         roundUpToSlow(LastGroupEnd - Region->RegionBeg, BlockSize) +
         Region->RegionBeg;
     const uptr ReleaseRangeSize = ReleaseEnd - ReleaseBeg;
 
     ReleaseRecorder Recorder(ReleaseBeg, &Region->Data);
     PageReleaseContext Context(
         BlockSize, Region->AllocatedUser, /*NumberOfRegions=*/1U,
         ReleaseRangeSize, /*ReleaseOffset=*/ReleaseBeg - Region->RegionBeg);
 
     for (BatchGroup &BG : GroupToRelease) {
       BG.PushedBlocksAtLastCheckpoint = BG.PushedBlocks;
 
-      // TODO(chiahungduan): Replace GroupId with BatchGroupBeg to simplify the
-      // calculation here and above (where we determine the set of groups to
-      // release).
       const uptr BatchGroupBeg =
-          Max(batchGroupBase(CompactPtrBase, BG.GroupId), Region->RegionBeg);
-      const uptr BatchGroupEnd =
-          batchGroupBase(CompactPtrBase, BG.GroupId) + GroupSize;
+          decompactGroupBase(CompactPtrBase, BG.CompactPtrGroupBeg);
+      const uptr BatchGroupEnd = BatchGroupBeg + GroupSize;
       const uptr AllocatedGroupSize = AllocatedUserEnd >= BatchGroupEnd
-                                          ? BatchGroupEnd - BatchGroupBeg
+                                          ? GroupSize
                                           : AllocatedUserEnd - BatchGroupBeg;
       const uptr BatchGroupUsedEnd = BatchGroupBeg + AllocatedGroupSize;
       const bool BlockAlignedWithUsedEnd =
           (BatchGroupUsedEnd - Region->RegionBeg) % BlockSize == 0;
 
       uptr MaxContainedBlocks = AllocatedGroupSize / BlockSize;
       if (!BlockAlignedWithUsedEnd)
         ++MaxContainedBlocks;
 
       const uptr NumBlocks = (BG.Batches.size() - 1) * BG.MaxCachedPerBatch +
                              BG.Batches.front()->getCount();
 
       if (NumBlocks != MaxContainedBlocks) {
         DCHECK_LT(NumBlocks, MaxContainedBlocks);
         // Note that we don't always visit blocks in each BatchGroup so that we
         // may miss the chance of releasing certain pages that cross
         // BatchGroups.
         Context.markFreeBlocks(BG.Batches, DecompactPtr, Region->RegionBeg);
       } else {
         for (const auto &It : BG.Batches)
           for (u16 I = 0; I < It.getCount(); ++I)
-            DCHECK_EQ(compactPtrGroup(It.get(I)), BG.GroupId);
+            DCHECK_EQ(compactPtrGroup(It.get(I)), BG.CompactPtrGroupBeg);
 
         Context.markRangeAsAllCounted(BatchGroupBeg, BatchGroupUsedEnd,
                                       Region->RegionBeg);
       }
     }
 
     DCHECK(Context.hasBlockMarked());
 
NOINLINE uptr releaseToOSMaybe(RegionInfo *Region, uptr ClassId,
     // `Region->FreeList` and `GroupToRelease` are sorted.
     for (BatchGroup *BG = Region->FreeList.front();; BG = BG->Next) {
       if (BG == nullptr || GroupToRelease.empty()) {
         if (!GroupToRelease.empty())
           Region->FreeList.append_back(&GroupToRelease);
         break;
       }
 
-      DCHECK_NE(BG->GroupId, GroupToRelease.front()->GroupId);
+      DCHECK_NE(BG->CompactPtrGroupBeg,
+                GroupToRelease.front()->CompactPtrGroupBeg);
 
-      if (BG->GroupId < GroupToRelease.front()->GroupId)
+      if (BG->CompactPtrGroupBeg < GroupToRelease.front()->CompactPtrGroupBeg)
         continue;
 
       BatchGroup *Cur = GroupToRelease.front();
       GroupToRelease.pop_front();
       Region->FreeList.insert(BG, Cur);
     }
 
     DCHECK_EQ(Region->FreeList.size(), NumberOfBatchGroups);