MMgc profiler: Minor code cleanup

VERIFIED FIXED

Status

VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: rishah, Unassigned)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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
(Reporter)

Updated

10 years ago
Blocks: 486409
(Reporter)

Comment 1

10 years ago
Created attachment 371312 [details] [diff] [review]
[v1] patch

Fixes for items 1-6 listed under description.
Attachment #371312 - Flags: review?(treilly)
Attachment #371312 - Flags: review?(lhansen)

Comment 2

10 years ago
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!!!
Attachment #371312 - Flags: review?(treilly) → review-
(Reporter)

Comment 3

10 years ago
(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?
(Reporter)

Comment 4

10 years ago
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

10 years ago
Comment on attachment 371312 [details] [diff] [review]
[v1] patch

Deal!
Attachment #371312 - Flags: review- → review+

Updated

10 years ago
Attachment #371312 - Flags: review?(lhansen) → review+
(Reporter)

Comment 6

10 years ago
changeset 1731	a49a40e9017e
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.