486587 - Rework MMgc profiler APIs for dumping memory info to spy file

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), 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: 

4 APIs to support spy file memory dump seems too many and unnecessarily burdens the platform porting effort (in future).

The immediate byproduct of spy file though is the need to redirect memory info output to another outputstream besides the one used by VMPI_log.  It almost requires VMPI_fprintf() like API which should be avoided as far as possible.

Proposal:
Have a single API that acts a hook to the platform to dump the memory info to whatever output stream it wants to.  The entire onus is on the platform to invoke memory info dump function and handle the redirection of logging data to an alternate destination.

This way we have minimal APIs in core code, and the platform has flexibility to do whatever it wants. 

Reproducible: Always

Comment 1

[Rishit Shah]

Attachment: [v1] patch - replacing spy file apis with newer ones

One standout issue is ChangeLogStream in MMgcProfilerWin.cpp that is declared as extern and defined in avmshellWin.cpp.  Changing the output stream ties in with VMPI_log implementation and that is becoming tricky because of our folder structure for VMPI implementation (as discussed in the bug logged by Steven) as well as dependency on shell (or other clients).

I didn't want to make this as a VMPI API with the fear that other parts of the code might start using it to log messages to other files or output streams.

Comment 2

[Rishit Shah]

Attachment: [v1.1] - patch removing the unnecessary vcproj changes from previous patch

Comment 3

[Rishit Shah]

Attachment: [v1.2] - patch now includes the new file MMgcPortWin.cpp

Apologies for the flood of patches.

Comment 4

[Tommy Reilly]

Comment on attachment 370886 [details] [diff] [review]
[v1.2] - patch now includes the new file MMgcPortWin.cpp

Moving the getenv into shell breaks the player, I'd like to have the same code work for player and shell use cases by isolating it into MMgc.

Comment 5

[Lars T Hansen]

As a general remark I think the less the VM relies on non-portable aspects of its environment the better.  Some platforms don't have (or won't have) environment variables.  Assuming that they do is possible - it leaves it to the platform layer on those platforms to simulate something the VM can access - but it would be better to look for more general solutions first.

Comment 6

[Lars T Hansen]

Comment on attachment 370886 [details] [diff] [review]
[v1.2] - patch now includes the new file MMgcPortWin.cpp

Generally OK with this but I defer to Tommy on requirements etc.

Comment 7

[Rishit Shah]

(In reply to comment #4)
> (From update of attachment 370886 [details] [diff] [review])
> Moving the getenv into shell breaks the player, I'd like to have the same code
> work for player and shell use cases by isolating it into MMgc.

Actually, we could move the getenv call to VMPI_setupProfilerSniffer which
would be carried over to player as well.  That also means that
GCHeapConfig::enableProfiler goes away.

Sound good?

Comment 8

[Rishit Shah]

(In reply to comment #1)
> One standout issue is ChangeLogStream in MMgcProfilerWin.cpp that is declared
> as extern and defined in avmshellWin.cpp.  Changing the output stream ties in
> with VMPI_log implementation and that is becoming tricky because of our folder
> structure for VMPI implementation (as discussed in the bug logged by Steven) as
> well as dependency on shell (or other clients).
> 
> I didn't want to make this as a VMPI API with the fear that other parts of the
> code might start using it to log messages to other files or output streams.

One functionality in VMPI_log that I scaled back on due to telemetry component was to pass logging markers like (LOG_DEFAULT, LOG_SPY etc) in VMPI_log.  Seems like that would be handy in situations like this where we dont want log messages to go to the default output channel.

Since telemetry task has been pushed down the stack, it would help adding this functionality.

Instead having those changes as a part of this bug it would be better to have that as a separate bug.

Comment 9

[Rishit Shah]

Btw, having a log marker for spy file would mean that the pipe would not be closed until shutdown.

Comment 10

[Rishit Shah]

Attachment: [v2] patch

Latest patch with 2 changes -

1. Moving getenv out of avmshell and into VMPI_setupProfilerSniffer() so that it works for player as well and addresses Tommy's feedback from patch v1.2 review.

2. Updating manifest file for MMgc\platform to include MMgcProfilerWin.cpp.

Comment 11

[Lars T Hansen]

Comment on attachment 371301 [details] [diff] [review]
[v2] patch

Again, I'm generally ok with this.

I strongly dislike the name profilerSnifferCallback because it is way too generic; callback for what?  (And what was wrong with the shorter "spy"?)

Comment 12

[Tommy Reilly]

Comment on attachment 371301 [details] [diff] [review]
[v2] patch

 +1 on moving getenv to VMPI.  +1 on bringing back the Spy term, this infrastructure was meant to be generic and not MMgc profiler specific, how about VMPI_setupSpy (which should be safe to call many times should other places in the VM use it). I'd like to player to use it someday, in the future I imagine pyspy taking an argument to suck out info about any subsystem

Comment 13

[Rishit Shah]

I will rename the APIs back to "spy".  So if the spy functionality is meant to be generic and could be called from several places, it should take a string as the identifier in setup as well as callback APIs to determine the spy channel.

Comment 14

[Tommy Reilly]

I'd cross that bridge when we get to it.

Comment 15

[Rishit Shah]

changeset 1734	b1dbc22d560d

Comment 16

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.