accessors to RTS flag values -- #5364
ClosedPublic

Authored by osa1 on Oct 3 2014, 1:50 PM.

Details

Summary

Implementation of Trac #5364. Mostly boilerplate, reading FILE fields is missing.

Test Plan
  • Get some feedback on missing parts. (FILE fields)
  • Get some feedback on module name.
  • Get some feedback on other things.
  • Get code reviewed.
  • Make sure test suite is passing. (I haven't run it myself)

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.
osa1 updated this revision to Diff 772.Oct 3 2014, 1:50 PM
osa1 retitled this revision from to accessors to RTS flag values -- #5364.
osa1 updated this object.
osa1 edited the test plan for this revision. (Show Details)
osa1 added a reviewer: ezyang.
osa1 updated the Trac tickets for this revision.
osa1 updated this revision to Diff 773.Oct 3 2014, 3:53 PM
osa1 edited edge metadata.
  • fix return type
osa1 updated this revision to Diff 774.Oct 3 2014, 4:36 PM
  • fix more warnings
austin requested changes to this revision.Oct 15 2014, 5:39 PM
austin edited edge metadata.

Overall, pretty good, but please look over the concerns below.

@simonmar may also want to review, but let's go for another round first.

includes/rts/Flags.h
21–28

Please add a Note about all of this! In particular, now, any time someone updates the RTS to accept new flags, they also need to reflect those changes inside of base, so we don't forget things. I cannot think of a way to test for this easily, so we should have a big warning with a Note.

I would suggest adding the bulk of the note at the top of the file (in the format of typical GHC Notes) and then referencing it with a comment for every struct, e.g. on this line add a comment saying -- See Note [Synchronization of flags and base APIs] or something.

243–250

Pedantry: align this vertically, please.

libraries/base/GHC/RTS.hsc
1 ↗(On Diff #774)

This module should have documentation, with a basic overview of the information you can retrieve using it.

1 ↗(On Diff #774)

I don't know if we should use up this namespace entirely with this module.

We actually have some foreign imports from the RTS elsewhere. Perhaps it would be best to consolidate these under a GHC.RTS.* namespace, and then re-export them appropriately (rather than having the glue everywhere), with a top level module. For example:

- GHC.RTS         -- top level re-exports lower-hierarchy modules.
  - GHC.RTS.Flags -- this module verbatim
  - GHC.RTS.Mem   -- exports `performGC`; re-exported by `System.Mem`
  - GHC.RTS.Stats -- re-exported by `GHC.Stats`

etc.

This is just a half baked idea, but I think it would be nicer, perhaps.

43 ↗(On Diff #774)

Like I said in the comment below this: use standard Haddock syntax:

-- | @'Time'@ is defined by GHC as @'StgWord64'@ in @stg/Types.h@
46 ↗(On Diff #774)

Use standard Haddock code formatting with source hyperlinks, e.g.

-- | @'Nat'@ is defined by GHC in @rts/Types.h@.

This will give proper text formatting for all backends and appropriately hyperlink things.

71 ↗(On Diff #774)

Docs. You don't need to describe every field if you don't want to, but that would be nice. :)

99 ↗(On Diff #774)

Ditto for this, and the following Flags.

147 ↗(On Diff #774)

Can we put this 0 behind a #define? Why is there no COST_CENTRES_NONE defined in Flags.h?

This revision now requires changes to proceed.Oct 15 2014, 5:39 PM

Also, this should have a comment in the release notes if you don't mind - please add one to the changelog file in base.

ezyang edited edge metadata.Nov 18 2014, 1:45 AM

Hey osa1, feature freeze for 7.10 is in a week; if you want to get this functionality in for 7.10 it would be best to update the patch soon so we can re-CR and land it.

osa1 updated this revision to Diff 1519.Nov 18 2014, 4:26 PM
osa1 edited edge metadata.
  • addressing comments: add comments, fix indentations.
osa1 updated this revision to Diff 1521.Nov 18 2014, 4:34 PM
osa1 edited edge metadata.
  • add COST_CENTRES_NONE constant
ezyang requested changes to this revision.Nov 18 2014, 4:58 PM
ezyang edited edge metadata.
ezyang added inline comments.
libraries/base/GHC/RTS.hsc
3 ↗(On Diff #1521)

Same as austin's previous comments: docs, and I'd feel better putting this in GHC.RTS.Flags

43 ↗(On Diff #1521)

Same as austin's previous comment.

71 ↗(On Diff #1521)

At the very least, a link to the user manual flags documentation!

199 ↗(On Diff #1521)

Why not just 'String'?

This revision now requires changes to proceed.Nov 18 2014, 4:58 PM
osa1 updated this revision to Diff 1538.Nov 19 2014, 2:41 AM
osa1 edited edge metadata.
  • fix an alignment
  • use haddock style docs. use String instead of [Char]
  • move the module to GHC.RTS.Flags
  • add module-level documentation
osa1 added a comment.Nov 19 2014, 2:42 AM

I tried to add some module-level documentation, however I think we don't have a documentation that describes all RTS flags. As far as I can see they're partially documented in https://www.haskell.org/ghc/docs/latest/html/users_guide/runtime-control.html , in +RTS --help and in includes/rts/Flags.h. Am I missing something?

osa1 updated this revision to Diff 1539.Nov 19 2014, 2:47 AM
osa1 edited edge metadata.
  • add GHC.RTS.Flags to base changelog for 4.8.0.0
ekmett added a subscriber: ekmett.Nov 19 2014, 3:37 AM
ezyang accepted this revision.Nov 20 2014, 8:38 PM
ezyang edited edge metadata.

OK.

This revision was automatically updated to reflect the committed changes.