[PATCH] D87120: [HeapProf] Heap profiling runtime support

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks for the comments @vitalybuka and @MaskRay. Starting with Vitaly's comments. Responses and follow up questions below.



================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.cpp:78-95
+struct ChunkHeader {
+  // 1-st 4 bytes.
+  u32 from_memalign : 1;
+  // This field is used for small sizes. For large sizes it is equal to
+  // SizeClassMap::kMaxSize and the actual size is stored in the
+  // SecondaryAllocator's metadata.
+  u32 user_requested_size : 29;
----------------
vitalybuka wrote:
> if you have dummy you can use them for user_requested_size and avoid special case for SizeClassMap::kMaxSize
> 
> you probably can steal 32bit from alloc_context_id and use it as offset
> this way you don't need metadata
> so if it's 0 then chunk starts with header, otherwise += offset bytes.
> 
> If I understand offset is a function of (alloc_beg, alignment)
> if alignment is 2^x then just one byte for  alignmentLog2 is enough
> 
> 
Thanks for the suggestions. You are right, I can increase the allocation size here given the extra space I currently have. Question on that further below.

And good suggestion about alloc_context_id, I see StackDepotPut returns a u32 anyway. I'm not sure why I changed this to u64 here - looks like it has been u32 in asan.


================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.cpp:62-113
+// The memory chunk allocated from the underlying allocator looks like this:
+// H H U U U U U U
+//   H -- ChunkHeader (32 bytes)
+//   U -- user memory.
+
+// If there is left padding before the ChunkHeader (due to use of memalign),
+// we store a magic value in the first uptr word of the memory block and
----------------
vitalybuka wrote:
> tejohnson wrote:
> > vitalybuka wrote:
> > > All this ChunkHeader stuff is Asan optimization hack to reuse redzones.
> > > There is metatada support in underlying allocators.
> > > Please take a look at hwasan_allocator
> > Note that this serves a specific purpose in the heap allocator as well (and in fact having a 32-byte header was part of the original design). It allows us to support a 64-byte shadow counter granularity with only a 32-byte alignment, so less overhead to avoid counter aliasing.
> As I understand it's just an optimization. This trick lets Asan to put Header into redzone (in your case in alignment)
> I am not sure how important this for asan and why @eugenis decided not use it and just use GetMetadata D40935
> HWAsan uses 64byte alignment as well.
Right, it's an optimization to reduce the memory overhead. Since it was straightforward to implement, I think it makes sense to use this support to enable reduced alignment requirements.


================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.cpp:561
+    }
+    if (using_primary_allocator) {
+      CHECK(size);
----------------
vitalybuka wrote:
> tejohnson wrote:
> > vitalybuka wrote:
> > > there is also asan specific inconsistency
> > > primiary/secondary difference is because primiary is reuse redzones and secondary not
> > > unfortunately asan is the worst example. 
> > > msan, hwasan and lsan have less unneeded stuff
> > Can you clarify why this isn't needed here? I do see references to the primary/secondary allocator in both hwasan and lsan. In fact lsan has code that conditionally memsets to 0 if it is from primary (similar to what is further below here in asan).
> primary vs secondary for asan are result of:
> 1. right redzone is needed for the secondary - irrelevant here
> 2. meta[1] contains chunk_beg, but I already removing this part from asan D86932, and explained above how to avoid to use GetMetaData here as well
BTW I found that the  "meta[0] = size" was removed in D86922, you might want to mark that as a parent to D86932. Question on that though. In D86922 you are using a total of at most 45 bits for holding the size. What is in fact the max size we need to represent?


================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.h:67
+  static const u64 kSpaceSize = SANITIZER_MMAP_RANGE_SIZE;
+  static const uptr kMetadataSize = 16;
+  typedef __heapprof::SizeClassMap SizeClassMap;
----------------
vitalybuka wrote:
> it was a typo in asan, neither asan nor heapprof use metadata with any primary allocator
> but I believe you don't need AP32 at all if 32bit is not a target
Will fix


================
Comment at: compiler-rt/lib/heapprof/heapprof_allocator.h:63-76
+typedef CompactSizeClassMap SizeClassMap;
+template <typename AddressSpaceViewTy> struct AP32 {
+  static const uptr kSpaceBeg = 0;
+  static const u64 kSpaceSize = SANITIZER_MMAP_RANGE_SIZE;
+  static const uptr kMetadataSize = 16;
+  typedef __heapprof::SizeClassMap SizeClassMap;
+  static const uptr kRegionSizeLog = 20;
----------------
vitalybuka wrote:
> if it's targeted to _x86_64__ you don't need AP32
Will fix


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:752
+
 int internal_dlinfo(void *handle, int request, void *p) {
 #if SANITIZER_FREEBSD
----------------
vitalybuka wrote:
> would you like to extract sanitizer_common into a separate patch?
> To my taste I'd cut this patch in a dozen of smaller ones.
> 
> 
> 
Let me try doing that, it will need to be a child revision of this so it can be used/tested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87120/new/

https://reviews.llvm.org/D87120