Rework MMgc profiler APIs for dumping memory info to spy file

VERIFIED FIXED

Status

VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: rishah, Unassigned)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

10 years ago
Blocks: 486409
(Reporter)

Comment 1

10 years ago
Created attachment 370882 [details] [diff] [review]
[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.
Attachment #370882 - Flags: review?(treilly)
(Reporter)

Updated

10 years ago
Attachment #370882 - Flags: review?(lhansen)
(Reporter)

Updated

10 years ago
Attachment #370882 - Attachment is patch: true
Attachment #370882 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 2

10 years ago
Created attachment 370884 [details] [diff] [review]
[v1.1] - patch removing the unnecessary vcproj changes from previous patch
Attachment #370882 - Attachment is obsolete: true
Attachment #370884 - Flags: review?(treilly)
Attachment #370882 - Flags: review?(treilly)
Attachment #370882 - Flags: review?(lhansen)
(Reporter)

Updated

10 years ago
Attachment #370884 - Flags: review?(lhansen)
(Reporter)

Comment 3

10 years ago
Created attachment 370886 [details] [diff] [review]
[v1.2] - patch now includes the new file MMgcPortWin.cpp

Apologies for the flood of patches.
Attachment #370884 - Attachment is obsolete: true
Attachment #370886 - Flags: review?(treilly)
Attachment #370884 - Flags: review?(treilly)
Attachment #370884 - Flags: review?(lhansen)
(Reporter)

Updated

10 years ago
Attachment #370886 - Flags: review?(lhansen)

Updated

10 years ago
Attachment #370886 - Flags: review?(treilly) → review-

Comment 4

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

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

10 years ago
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.
Attachment #370886 - Flags: review?(lhansen) → review+
(Reporter)

Comment 7

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

Comment 8

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

Comment 9

10 years ago
Btw, having a log marker for spy file would mean that the pipe would not be closed until shutdown.
(Reporter)

Comment 10

10 years ago
Created attachment 371301 [details] [diff] [review]
[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.
Attachment #370886 - Attachment is obsolete: true
Attachment #371301 - Flags: review?(treilly)
Attachment #371301 - Flags: review?(lhansen)

Comment 11

10 years ago
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"?)
Attachment #371301 - Flags: review?(lhansen) → review+

Comment 12

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

Comment 13

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

10 years ago
I'd cross that bridge when we get to it.
(Reporter)

Comment 15

10 years ago
changeset 1734	b1dbc22d560d
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 16

9 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.