Closed Bug 1274041 Opened 4 years ago Closed 4 years ago

make jprof work better with e10s

Categories

(Core :: General, defect, P4)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox49 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

()

Details

Attachments

(3 files)

Jesup did some work in bug 674384, but some more work needs to be done with current e10s to make jprof usable with e10s.
I've had this in my tree for a while; it just fixes what look like a
silly mistake.

Review commit: https://reviewboard.mozilla.org/r/53696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53696/
Attachment #8754053 - Flags: review?(rjesup)
Attachment #8754054 - Flags: review?(rjesup)
Attachment #8754055 - Flags: review?(rjesup)
This makes a function call in XRE_InitChildProcess for the child process that
currently exists in XREMain::XRE_mainStartup for the parent process.

This allows signals that jprof uses to be sent to a child process to
profile that child process.

Review commit: https://reviewboard.mozilla.org/r/53698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53698/
This makes the child process write its memory map to a different file
name, just like it does for the jprof log.  It tries to share the
variables between the two so that they're both connected in the code and
consistent with each other.

Note that I haven't yet written the patch to make jprof.cpp *read* the
map from that file, so this currently requires manually renaming the
generated map with the numeric suffix to jprof-map in order to run
jprof.  I should probably write that patch eventually, but I haven't
actually needed to.

This at least prevents the child process's map file from being
overwritten by the parent's a fraction of a second later.

Review commit: https://reviewboard.mozilla.org/r/53700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53700/
Comment on attachment 8754055 [details]
MozReview Request: Bug 1274041 - Make child process write its memory map to a different file name.  r?jesup

https://reviewboard.mozilla.org/r/53700/#review50556

::: tools/jprof/stub/libmalloc.cpp:173
(Diff revision 1)
> +  if (gIsSlave)
> +    snprintf(filename, sizeof(filename), "%s-%d", M_MAPFILE, gFilenamePID);

So (probably) jprof.map-pid
I guess we don't have easy access to the executable file's name here.
Attachment #8754055 - Flags: review?(rjesup) → review+
Attachment #8754054 - Flags: review?(rjesup) → review+
Comment on attachment 8754054 [details]
MozReview Request: Bug 1274041 - Initialize jprof in child process.  r?jesup

https://reviewboard.mozilla.org/r/53698/#review50558
Comment on attachment 8754053 [details]
MozReview Request: Bug 1274041 - Actually define JPROF_STATIC as it was meant to be defined.  r?jesup

https://reviewboard.mozilla.org/r/53696/#review50560

sure, though it largely doesn't matter.
Attachment #8754053 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #4)
> So (probably) jprof.map-pid
> I guess we don't have easy access to the executable file's name here.

Was there something you wanted me to change here?
Flags: needinfo?(rjesup)
(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #7)
> (In reply to Randell Jesup [:jesup] from comment #4)
> > So (probably) jprof.map-pid
> > I guess we don't have easy access to the executable file's name here.
> 
> Was there something you wanted me to change here?

No - some tweaks may need to be made to jprof (leaky.cpp) and/or document how to do this in the readme.html.  That can be a separate patch/bug
Flags: needinfo?(rjesup)
tracking-e10s: --- → +
Priority: -- → P4
You need to log in before you can comment on or make changes to this bug.