Unified Compressed Pointer

[guptask]

added ability for compressed pointer to use full 32 bits for addressing in single tier mode and use 31 bits for addressing in multi-tier mode

---

This change is 

[igchor]

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @guptask)

---

cachelib/allocator/memory/CompressedPtr.h line 49 at r1 (raw file):

// the following are for pointer compression for the memory allocator.  We
// compress pointers by storing the tier index, slab index and alloc index
// of the allocation inside the slab. With slab worth kNumSlabBits (22 bits)

why this description is repeated?

---

cachelib/allocator/memory/CompressedPtr.h line 82 at r1 (raw file):

  // maximum adressable memory for pointer compression to work.
  static constexpr size_t getMaxAddressableSize() noexcept {
    return static_cast<size_t>(1) << (kNumSlabIdxBits + Slab::kNumSlabBits + 1);

why +1? we have the same total addressable memory in total - half for each tier, right?
Can you add some test for this?

---

cachelib/allocator/memory/CompressedPtr.h line 139 at r1 (raw file):

      return (slabIdx << kNumAllocIdxBits) + allocIdx;
    }
    XDCHECK_LT(slabIdx, (1u << kNumSlabIdxBits) - 1);

is this assert correct? with tier ID bit, we have less bits for slab, right?

You could also add ASSERT for tid I guess (< 2).

---

cachelib/allocator/memory/CompressedPtr.h line 214 at r1 (raw file):

    }

    bool isMultiTiered = allocators_.size() > 1 ? true : false;

you can drop ? true : false part

---

cachelib/allocator/memory/CompressedPtr.h line 216 at r1 (raw file):

    bool isMultiTiered = allocators_.size() > 1 ? true : false;
    auto cptr = allocators_[tid]->compress(uncompressed, isMultiTiered);
    if (allocators_.size() > 1) { // config has multiple tiers

why not just use isMultiTIered?

---

cachelib/allocator/memory/CompressedPtr.h line 226 at r1 (raw file):

      return nullptr;
    }
    bool isMultiTiered = allocators_.size() > 1 ? true : false;

same here

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 139 at r1 (raw file):

        slabStats = allocator->getAllocationClassStats(0,0,cid);
    }
    ASSERT_GE(slabStats.approxFreePercent,9.5);

why this is removed?

[guptask]

cachelib/allocator/memory/CompressedPtr.h line 49 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

why this description is repeated?

fixed

[guptask]

cachelib/allocator/memory/CompressedPtr.h line 82 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

why +1? we have the same total addressable memory in total - half for each tier, right?
Can you add some test for this?

I changed kNumTierIdxOffset to 31 from 32.

[guptask]

cachelib/allocator/memory/CompressedPtr.h line 82 at r1 (raw file):

Previously, guptask (Sounak Gupta) wrote…

I changed kNumTierIdxOffset to 31 from 32.

kNumSlabIdxBits = kNumTierIdxOffset (31) - kNumAllocIdxBits (16)

[guptask]

cachelib/allocator/memory/CompressedPtr.h line 139 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

is this assert correct? with tier ID bit, we have less bits for slab, right?

You could also add ASSERT for tid I guess (< 2).

In single tier mode, CompressedPtr doesn't reserve the 32nd bit for tier id. In this PR, kNumTierIdxOffset was changed from 32 to 31. So kNumSlabIdxBits = kNumTierIdxOffset - kNumAllocIdxBits = 31-16 = 15.

[guptask]

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 139 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

why this is removed?

moving to a separate commit since this fix is not directly related to CompressedPtr PR

[guptask]

cachelib/allocator/memory/CompressedPtr.h line 214 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

you can drop ? true : false part

done

[guptask]

cachelib/allocator/memory/CompressedPtr.h line 216 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

why not just use isMultiTIered?

done

[guptask]

cachelib/allocator/memory/CompressedPtr.h line 226 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

same here

done

[igchor]

@guptask can you run a hit_ratio leader and follower benchmark to see if there is no regression? I don't expect any but it would be good to make sure everything works as expected.

Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @guptask)

[guptask]

done. regression not observed.

[byrnedj]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @guptask)