[PATCH] D86295: [analyzer] Reorder the the layout of MemRegion and cache by hand for optimal size

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: NoQ, xazax.hun, Szelethus, vsavchenko, ASDenysPetrov.
Herald added subscribers: cfe-commits, martong, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity.
Herald added a project: clang.
steakhal requested review of this revision.

By the time working on the gdb pretty-printers for the Analyzer classes,
I observed that the `MemRegion` class has an inefficient memory layout.

Each and every object which's has a `MemRegion` as a subobject would benefit from this change.
We could spare 8 bytes for each case.

As a downside, I had to substitute the `llvm::Optional`, since that alone consumes 16 bytes (even if the wrapped object would consume 8 bytes :| ).

Before the patch: `sizeof(MemRegion) == 48`

  (gdb) ptype /o class MemRegion
  /* offset    |  size */  type = class clang::ento::MemRegion : public llvm::FoldingSetBase::Node {
                           private:
  /*   16      |     4 */    const clang::ento::MemRegion::Kind kind;
  /* XXX  4-byte hole  */
  /*   24      |    24 */    class llvm::Optional<clang::ento::RegionOffset> [with T = clang::ento::RegionOffset] {
                               private:
  /*   24      |    24 */        class llvm::optional_detail::OptionalStorage<T, true> [with T = T] {
                                   private:
  /*   24      |    16 */            union {
  /*                 1 */                char empty;
  /*                16 */                T value;
  
                                         /* total size (bytes):   16 */
                                     };
  /*   40      |     1 */            bool hasVal;
  
                                     /* total size (bytes):   24 */
                                 } Storage;
  
                                 /* total size (bytes):   24 */
                             } cachedOffset;
  
                             /* total size (bytes):   48 */
                           }

After the patch: `sizeof(MemRegion) == 40`

  (gdb) ptype /o MemRegion
  /* offset    |  size */  type = class clang::ento::MemRegion : public llvm::FoldingSetBase::Node {
                           private:
  /*   16      |    16 */    class clang::ento::RegionOffset {
                               private:
  /*   16      |     8 */        const clang::ento::MemRegion *R;
  /*   24      |     8 */        int64_t Offset;
                               public:
                                 static const int64_t Symbolic;
  
                                 /* total size (bytes):   16 */
                             } cachedOffset;
  /*   32      |     4 */    const clang::ento::MemRegion::Kind kind;
  /*   36      |     1 */    bool hasCachedOffset;
  
                             /* total size (bytes):   40 */
                           }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86295

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp


Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1579,9 +1579,11 @@
 }
 
 RegionOffset MemRegion::getAsOffset() const {
-  if (!cachedOffset)
+  if (!hasCachedOffset) {
     cachedOffset = calculateOffset(this);
-  return *cachedOffset;
+    hasCachedOffset = true;
+  }
+  return cachedOffset;
 }
 
 //===----------------------------------------------------------------------===//
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -100,8 +100,9 @@
   };
 
 private:
+  mutable RegionOffset cachedOffset;
   const Kind kind;
-  mutable Optional<RegionOffset> cachedOffset;
+  mutable bool hasCachedOffset = false;
 
 protected:
   MemRegion(Kind k) : kind(k) {}


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D86295.286825.patch
Type: text/x-patch
Size: 1066 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200820/10e2353f/attachment.bin>