Log heap profiler samples to event log
ClosedPublic

Authored by bgamari on Dec 30 2015, 3:39 PM.

Details

Summary

This adds support for emitting heap census data to the eventlog, allowing heap behavior to be correlated with other tracing events. Currently we still emit hp records even when eventlog output is enabled, although we may wish to revisit this eventually with an eye towards deprecating the hp format.

Test Plan

Try it

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bgamari updated this revision to Diff 6041.Dec 30 2015, 3:39 PM
bgamari retitled this revision from to Log heap profiler samples to event log.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
bgamari added a reviewer: hvr.
bgamari updated the Trac tickets for this revision.
bgamari updated this revision to Diff 6044.Dec 31 2015, 7:47 AM
bgamari edited edge metadata.

Make it work

bgamari updated this revision to Diff 6045.Dec 31 2015, 8:03 AM
bgamari edited edge metadata.

Iterate

bgamari updated this revision to Diff 6445.Jan 26 2016, 7:36 AM
bgamari edited edge metadata.
  • Log heap profiler samples to event log
  • Allow prof, eventlog, and threaded ways to coexist
  • things
  • Documentation
bgamari updated this revision to Diff 7057.Mar 25 2016, 8:01 PM
bgamari edited edge metadata.
  • Remove profiling, non-eventlog way
bgamari updated this revision to Diff 7062.Mar 25 2016, 8:06 PM
bgamari edited edge metadata.

wibbles

bgamari updated this revision to Diff 7710.May 24 2016, 5:46 AM
bgamari edited edge metadata.
  • Rebase
simonmar edited edge metadata.May 24 2016, 8:18 AM

This looks pretty sensible, modulo a few comments below.

docs/users_guide/profiling.rst
1400

I don't think we describe the binary format of events elsewhere in the User Guide, so this looks a bit strange. I'd think a comment in EventLogFormat.h would be better, no?

includes/rts/EventLogFormat.h
165–166

Delete this

174

Reserve a tag range for profiling events, and I suggest documenting the format here.

mk/config.mk.in
263

We might as well roll eventlog into the prof way, to avoid way explosion. We already do this for debug.

rts/ProfHeap.c
380

Do you really want to dump the cost centres for each sample? It would be enough to dump them at the start of the program, and when dynamically loading code.

bgamari updated this revision to Diff 7724.May 24 2016, 12:10 PM
bgamari edited edge metadata.
  • Address Simon's comments
bgamari marked 2 inline comments as done.May 24 2016, 12:11 PM
bgamari added inline comments.
docs/users_guide/profiling.rst
1400

How about moving it to an appendix? IMHO this is part of the user-visible interface that the compiler provides and consequently should be documented in the user guide. Moreover, it's a bit more readable with proper mark-up.

mk/config.mk.in
263

Wait, isn't that what I do? Note how there is no longer a p way, only p_l.

rts/ProfHeap.c
380

I'm confused. dumpCostCentresToEventLog is only called from initHeapProfiling, which should be called precisely once, no?

simonmar added inline comments.May 26 2016, 10:47 AM
mk/config.mk.in
263

Oh I see. But that's different from the way we handle debug, which is that debug is assumed to include l. So debug_p_l is weird, and so is thr_debug_p_l. Also, getting rid of "p" will be surprising, so I propose we just make "p_l" == "p".

See https://phabricator.haskell.org/diffusion/GHC/browse/master/compiler/main/Packages.hs;5020bc8fd2a99a557f45ea5abf8240ac995cc03d$1235-1239

rts/ProfHeap.c
380

Ah sorry, I misread the code.

@simonmar, quick question inline.

mk/ways.mk
65

@simonmar, I'm confused. If the debug way implies eventlog support shouldn't -eventlog appear in the options here?

bgamari added inline comments.Jun 16 2016, 8:53 AM
mk/ways.mk
65

The key here appears to be in includes/rts/Config.h,

/* DEBUG implies TRACING and TICKY_TICKY
 */
#if defined(DEBUG)
#define TRACING
#define TICKY_TICKY
#endif

Now the question is why handle this here instead of simply passing the relevant arguments to ghc while building the RTS.

bgamari updated this revision to Diff 7976.Jun 16 2016, 9:38 AM
bgamari edited edge metadata.
  • Rework handling of ways
bgamari updated this revision to Diff 7990.Jun 18 2016, 4:15 AM
bgamari edited edge metadata.

Address more comments

bgamari updated this revision to Diff 7991.Jun 18 2016, 4:33 AM
bgamari edited edge metadata.

More documentation

bgamari updated this revision to Diff 7992.Jun 18 2016, 4:36 AM
bgamari edited edge metadata.

Add note to release notes

bgamari updated this revision to Diff 7993.Jun 18 2016, 4:39 AM
bgamari marked 3 inline comments as done.
bgamari edited edge metadata.

More comments

bgamari updated this object.Jun 18 2016, 4:42 AM
bgamari edited edge metadata.
simonmar accepted this revision.Jun 20 2016, 10:13 AM
simonmar edited edge metadata.

Looks like this is ready to go now.

compiler/main/DynFlags.hs
2354

You don't actually need this, right?

docs/users_guide/8.2.1-notes.rst
60

allowinng

docs/users_guide/eventlog-formats.rst
2

It's suboptimal that we now have a partial (but detailed) description of the format here and a separate partial description of the format in the header file. I think this is moving in the right direction, but in the meantime maybe a pointer from each to the other would be a good idea?

This revision is now accepted and ready to land.Jun 20 2016, 10:13 AM
This revision was automatically updated to reflect the committed changes.