491231 - Memory Profiler cleanup times part-II from review of profiler

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Rishit Shah]

1) StackTrace::sweepSize and StackTrace::sweepCount should be removed as they
are no longer used

2) GCHeap::GCBlock::freeTrace should be removed as it is not used

3) heapAlloc has an argument named “internal”. Rename it to avoid any conflict
with a reserve keyword on compilers.

4) In DumpStackTraceHelper do we need the “out” buffer to be 2K in size given
that we flush the data after about 200 bytes are filled up?

5) Get rid of unused method ChangeSizeForObject().

6) RCObject::DumpHistory provides no functionality and can be removed. The
useful code in the function is ifdef’d out.

7) StackTrace::skip need not be size_t. uint8_t should suffice.

8) StackTrace::count/totalCount could be uint32_t instead of size_t

9) In GC::Alloc() the first “if” check (ie. if(size <= 128)) is not required
because the table lookup in the second case essentially does the same. We could
just use the table lookup to obtain allocator index.

10) Minor item: GetPackage()/GetAllocationCategory() called from
MemoryProfiler::DumpFatties(), both invoke GetAllocationName(). We could
refactor code to have GetAllocationName() called only once.

Comment 1

[Tommy Reilly]

1) StackTrace::sweepSize and StackTrace::sweepCount are no longer used

There used to a choice in DumpFatties between "total" "live" and "swept".    total showed the number's ignoring deletes, and swept showed just the deletes, ignoring live data.  Super useful for optimizing away sweeps.

2) GCHeap::GCBlock::freeTrace is not used

Useful debugging aid (block alloc "history")

3) heapAlloc has an argument named “internal”.  Could that conflict with a reserve keyword on some compiler?

+1 on changing it

4) In DumpStackTraceHelper do we need the “out” buffer to be 2K in size given that we flush the data after about 200 bytes are filled up?

Well there could be a lot more than 200 depending on what getFunctionNameFromPC and what getLineAndFileInfo return no?

5) ChangeSizeForObject() is unused.

Nuke it!

6) RCObject::DumpHistory provides no functionality.  The useful code in the function is ifdef’d out.

That code was really useful once upon a time, file a bug, we should re-enable it.
 
7) StackTrace::skip need not be size_t.  uint8_t should suffice.

+1

8) StackTrace::count/totalCount could be uint32_t instead of size_t

I vote for uint64_t

9) In GC::Alloc() the first “if” check (ie. if(size <= 128)) is not required because the table lookup
in the second case essentially does the same.  We could just use the table lookup to obtain
allocator index.

Cool, file a bug/patch.

10) Minor item:
GetPackage()/GetAllocationCategory() called from
MemoryProfiler::DumpFatties(), both invoke GetAllocationName().   We could
refactor code to have
GetAllocationName() called only once.

Why?  Doesn't GetAllocationName cache its result?

Comment 2

[Rishit Shah]

(In reply to comment #1)
> 1) StackTrace::sweepSize and StackTrace::sweepCount are no longer used
> 
> There used to a choice in DumpFatties between "total" "live" and "swept".   
> total showed the number's ignoring deletes, and swept showed just the deletes,
> ignoring live data.  Super useful for optimizing away sweeps.
I will leave it then.

> 2) GCHeap::GCBlock::freeTrace is not used
> 
> Useful debugging aid (block alloc "history")
Will be replaced by the new history tracker.  But will leave this alone until then.

> 4) In DumpStackTraceHelper do we need the “out” buffer to be 2K in size given
> that we flush the data after about 200 bytes are filled up?
> 
> Well there could be a lot more than 200 depending on what getFunctionNameFromPC
> and what getLineAndFileInfo return no?
"out" buffer is different from the one passed to VMPI functions.  Anyways, this seems like an inconsistent code in TR vs DM.  DM dumps out the data after 200 characters where as TR does that after 1500.

 
> 5) ChangeSizeForObject() is unused.
> 
> Nuke it!
> 
> 6) RCObject::DumpHistory provides no functionality.  The useful code in the
> function is ifdef’d out.
> 
> That code was really useful once upon a time, file a bug, we should re-enable
> it.
> 
> 7) StackTrace::skip need not be size_t.  uint8_t should suffice.
> 
> +1
> 
> 8) StackTrace::count/totalCount could be uint32_t instead of size_t
> 
> I vote for uint64_t
uint64_t it is.

> 9) In GC::Alloc() the first “if” check (ie. if(size <= 128)) is not required
> because the table lookup
> in the second case essentially does the same.  We could just use the table
> lookup to obtain
> allocator index.
> 
> Cool, file a bug/patch.
Will do

> 10) Minor item:
> GetPackage()/GetAllocationCategory() called from
> MemoryProfiler::DumpFatties(), both invoke GetAllocationName().   We could
> refactor code to have
> GetAllocationName() called only once.
> 
> Why?  Doesn't GetAllocationName cache its result?
No it doesn't.  It caches the stack trace but calls the VMPI function to get the function name details.

Comment 3

[Rishit Shah]

(In reply to comment #2)
> (In reply to comment #1)

> > 10) Minor item:
> > GetPackage()/GetAllocationCategory() called from
> > MemoryProfiler::DumpFatties(), both invoke GetAllocationName().   We could
> > refactor code to have
> > GetAllocationName() called only once.
> > 
> > Why?  Doesn't GetAllocationName cache its result?
> No it doesn't.  It caches the stack trace but calls the VMPI function to get
> the function name details.
Oops...i know what you mean.  Yes it does.

Comment 4

[Rishit Shah]

Attachment: [v1] patch

Comment 5

[Rishit Shah]

changeset 1832	6eab4335e729