486409 - MMgc Memory Profiler cleanup items

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Profiler, defect)

Description

[Rishit Shah]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8) Gecko/2009032608 Firefox/3.0.8
Build Identifier: 

Creating this bug to track items in profiler code that needs modifications as well as to gather thoughts and feedback.

Individual items identified from the discussion will be logged as separate bugs and dangled off this primary bug.

Reproducible: Always

Comment 1

[Rishit Shah]

Should I attach this one to the MMgc cleanup items bug to have a single tracking point?

Comment 2

[Tommy Reilly]

I think profiler specific is good here's my wish list:

- get rid of MemoryProfiler configuration and instead have a script to toggle the omit frame pointer optimization switch
- port to mac
- port to linux
- port to winmo
- have QE develop some tests for the memory profiler, I think Dan S might have some back log items/thoughts on this

Comment 3

[Tommy Reilly]

- print detailed size class info for GCAlloc and FixedMalloc (code is there for GCAlloc just needs to be enabled)
- include internal fragmentation #'s by tracking askSize and gotSize in profiler alloc hooks

Comment 4

[Rishit Shah]

List of minor code cleanup -

1. GCHeap::autoGCStats is used only for MMGC_MEMORY_PROFILER but its declared for all cases.  Move it under the define as well?  New api on GCHeap might be required to enable it from shell.

2. Ditto for GCHeap::signal. Btw, can't find usage of GCHeap::signal.  Where/how is it set or initialized?

3. HeapBlock::allocTrace and HeapBlock::freeTrace are unused, should be removed.

4. 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.

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

6. Inline GCHeap::HooksEnabled()

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

Comment 5

[Rishit Shah]

Nomenclature:

MemoryProfiler::Alloc doesn't sound right.  Shouldn't be it something like RecordAllocation or CaptureAllocTrace?  Similarly, ReleaseAllocTrace or RecordDeallocation instead of MemoryProfiler::Free.

Comment 6

[Rishit Shah]

GCStackTraceHashtable::hash relies on simple addition to come up with the hash value.  Are we sure this would not lead to collisions thereby recording incorrect stack trace when called from GetStackTrace()?  

Why not use the out_param of WINAPI to get the hash value? Is their hash function slower and could affect performance?

Comment 7

[Rishit Shah]

Now that VMPI_captureStackTrace is an API call, I think framesToSkip should be 3 instead of 2.

Comment 8

[Rishit Shah]

Porting Issues:

1. GCThreadLocal uses Win/Posix functions directly.  They need to be VMPI-ized.

2. All GCHeap::DumpXX functions are using libc routines instead of VMPI ones.

3. spyFile is declared as FILE which does not conform to VMPI rules.

Comment 9

[Rishit Shah]

spyFile and related APIs:

Seems like spyFile and APIs like VMPI_openAndConnectToNamedPipe/VMPI_HandleToStream/VMPI_CloseNamedPipe are -
1. Specific to Windows
2. Could you replaced by more generic APIs that switch the outputstream to temporarily redirect log traffic.

Currently, spyFile is switched only for DumpMemoryInfo call in GCHeap::AllocHook.

Comment 10

[Rishit Shah]

There is a FIXME comment in MemoryProfiler::Free.  Address it or remove the comment.

Comment 11

[Tommy Reilly]

autoGCStats is used from avmshell and works outside memory profiler

signal is used for the pyspy trigger mechanism

I like RecordAllocation and RecordDeallocation

HeapBlock allocBlock and freeBlock should be wired up as StackTrace *'s from GCHeap::Alloc and GCHeap::Free when MMGC_MEMORY_INFO is defined they are useful to have for debugging (ie you know where a block was allocated/freed).   The decls for these fields shouldn't be MMGC_MEMORY_PROFILER either (don't want them in Release builds).  Maybe they should be:  defined(MMGC_MEMORY_INFO) && defined(MMGC_MEMORY_PROFILER)

> Are we sure this would not lead to collisions thereby recording incorrect stack trace when called from GetStackTrace()?  

hash key collisions are fine, that's what equals is for.

Comment 12

[Steven Johnson]

(In reply to comment #9)
> VMPI_openAndConnectToNamedPipe/VMPI_HandleToStream/VMPI_CloseNamedPipe are -

super-nit: could we standardize on a camelcase convention for VMPI?

i.e., either

VMPI_InitialCap()

or

VMPI_initialLowercase()

?

Comment 13

[Rishit Shah]

camelCase is the intendend convention for VMPI functions.

HandleToStream/CloseNamedPipe were result of an oversight.  I will change them.

Comment 14

[Lars T Hansen]

(In reply to comment #11)
> I like RecordAllocation and RecordDeallocation

I do too, though I note that that except for mmgc (which is not 100% consistent but almost so) the vm uses javaCapitalizationStyle not WindowsCapitalizationStyle.

Ah, I now see Steven made the same observation about VMPI, and there I do want to be hard-nosed.  I concur with Rishit.

Note also VMPI_Log.

Comment 15

[Rishit Shah]

Re allocTrace/freeTrace, I don't see them getting set anywhere.

Comment 16

[Tommy Reilly]

right, code needs to be added, I took it out when the new profiler landed.  MemoryProfiler::GetStackTrace might need to be exposed publicly to do it.

Comment 17

[Rishit Shah]

Items from detailed review of profiler -

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 18

[Dan Schaffer]

from Rishit:

The only reason the bug is open is due to its dependency on https://bugzilla.mozilla.org/show_bug.cgi?id=491776. I had made those changes as a part of related change but that changed was not considered required at this point.
 
I can submit a patch for this bug or it could be deferred.

Comment 19

[Trevor Baker]

If 491776 is targeted to future this one must be also.