487102 - MMgc profiler: Minor code cleanup

Status: VERIFIED FIXED

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

Description

[Rishit Shah]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.0; SLCC1; .NET CLR 2.0.50727; Media Center PC 5.0; .NET CLR 3.5.30729; .NET CLR 3.0.30618)
Build Identifier: 

1. StrackTrace ctor contains 
VMPI_memcpy(ips, trace, kMaxStackTrace * sizeof(void*));
However, trace is defined as uintptr_t.  sizeof(void*) == sizeof(uintptr_t)
would be the case but better to be consisent.

2. Cleanup forward declarations of GetStackTrace() and DumpStackTrace() in
GCMemoryProfiler.cpp as there are no such functions.

3. Inline GCHeap::HooksEnabled()

4. In MemoryProfiler::Alloc, setting memtype, memtag to NULL outside of "if"
condition is not necessary.

5. Renaming GCMemoryProfiler::Alloc/Free to RecordAllocation/RecordDeallocation respectively.

6. In GCMemoryProfiler::GetStackTrace, framesToSkip argument should be 3 instead of 2 given that VMPI_captureStackTrace is an API call which adds an additional function frame to ignore.

Reproducible: Always

Comment 1

[Rishit Shah]

Attachment: [v1] patch

Fixes for items 1-6 listed under description.

Comment 2

[Tommy Reilly]

Comment on attachment 371312 [details] [diff] [review]
[v1] patch

GetStackTrace wants to be used by GCHeap to record the alloc/free trace of each block no?   Also can you look at the MMgc in the DownwardlyMobile branch, if you are gonna change this code you might as well incorporate the latest (in particular I'm thinking of the askSize and internal fragmentation stuff).  Thank you!!!

Comment 3

[Rishit Shah]

(In reply to comment #2)
> (From update of attachment 371312 [details] [diff] [review])
> GetStackTrace wants to be used by GCHeap to record the alloc/free trace of each
> block no?   
GetStackTrace() is already defined as a member function of MemoryProfiler.  I just removed the forward declaration of a standalone GetStackTrace function that was not defined anywhere.

>Also can you look at the MMgc in the DownwardlyMobile branch, if
> you are gonna change this code you might as well incorporate the latest (in
> particular I'm thinking of the askSize and internal fragmentation stuff). 
> Thank you!!!
I will do that but not sure if we wanna mix those changes with this patch.  Do we?

Comment 4

[Rishit Shah]

I've logged a separate bug for moving over changes from DownwardlyMobile branch to redux - https://bugzilla.mozilla.org/show_bug.cgi?id=487452

Given that and my reply to your comment on GetStackTrace() above, could you let me know if you approve the existing patch?

Comment 5

[Tommy Reilly]

Comment on attachment 371312 [details] [diff] [review]
[v1] patch

Deal!

Comment 6

[Rishit Shah]

changeset 1731	a49a40e9017e