Trac #9579: guide user to link with `-rtsopts` when they are not available
ClosedPublic

Authored by Javran on Mar 29 2015, 4:04 AM.

Details

Summary

This patch provides user with a better hint when most RTS options
are not available (not compiled with -rtsopts).

A new field "rtsOptsEnabled" is added into RtsFlags.MiscFlags to
tell the availablity of RTS options.

Some concerns:

  • Unlike other flag fields in "libraries/base/GHC/RTS/Flags.hsc",

"RtsOptsEnabled" is defined in "includes/RtsAPI.h" and lacks constant macros.
Therefore In "GHC.RTS", "RtsOptsEnabled" simply derives Enum instance and reads as of type "CInt".
I don't know if this is a proper way to deal with it.

  • There are other ways to change RTS options (e.g. -with-rtsopts),

but it might be too verbose to mention.

  • I'm not sure how to trigger the error message about "maximum number of censuses reached",

therefore no testcase for that particular case.

  • In the ticket @rwbarton proposed about addinng an option "-no-rtsopts-suggestions",

which is not implemented in this patch.

Test Plan

validate

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.
Javran updated this revision to Diff 2555.Mar 29 2015, 4:04 AM
Javran retitled this revision from to Fix Trac #9579.
Javran updated this object.
Javran edited the test plan for this revision. (Show Details)
Javran added reviewers: austin, thomie.
Javran updated the Trac tickets for this revision.
Javran added a subscriber: rwbarton.
thomie requested changes to this revision.Mar 29 2015, 5:57 AM
thomie edited edge metadata.

Looks good. I like the thorough testing you did, and that you thought of updating base as well (although a test is missing here, but I think that's ok).

Just some minor issues about the tests:

  • empty stdout files don't need to be added
testsuite/tests/rts/T9579/OutOfHeap.hs
5

What is this magic number? Please add your calculation in a comment.
Will it also fail on a 32bit architecture?

This revision now requires changes to proceed.Mar 29 2015, 5:57 AM

And please give this Diff a better title. You can do so at the top of this page: 'Edit Revision'. The title of a Diff is what ends up as the commit message subject.

Javran retitled this revision from Fix Trac #9579 to Trac #9579: guide user to link with `-rtsopts` when they are not available.Mar 29 2015, 11:20 AM
Javran edited edge metadata.
Javran updated this revision to Diff 2558.Mar 29 2015, 12:10 PM
Javran edited edge metadata.
  • rewrite test program, comments for clarification
  • remove empty stdout files in the testsuite

Thanks for the comment, the patch has been updated accordingly.
Also I can confirm running make CLEANUP=1 will leave no untracked files without use of extra_clean.

thomie accepted this revision.Mar 30 2015, 2:28 AM
thomie edited edge metadata.

LGTM

Note for other reviewers: I'll fixup the commit message before pushing.

This revision is now accepted and ready to land.Mar 30 2015, 2:28 AM
Javran updated this revision to Diff 2662.Apr 5 2015, 10:28 PM
Javran marked an inline comment as done.
Javran edited edge metadata.
  • adding rtsOptsSuggestions
  • change error message
  • change GHC.RTS.Flags accordingly
  • update docs
  • add testcases
thomie added a comment.Apr 7 2015, 5:36 AM

Javran: I think you mistakenly included the changes for D809 here as well. Could you try fixing it. Or, if you want all changes to go in in one commit, then please abandon one of the two Diffs. You can do so at the bottom of the page from the dropdown menu.

I know Phabricator can be confusing when working with multiple Diffs at the same time.

Things to remember:

  • when you run arc diff, arc will try to find a line Differential Revision: https://phabricator.haskell.org/Dxxx in the commit message of the commit you are currently on (HEAD). If it finds it, it will update that Diff, otherwise it creates a new Diff.
  • what helps is telling arc explicitly how many commits you want to be included in the Diff. arc diff HEAD^ for only the last commit, arc diff HEAD^^ for the last two commits, etc.
Javran abandoned this revision.Apr 7 2015, 11:12 AM

Oh, sorry about that. I intended to split these two patches.

Javran updated this revision to Diff 2685.Apr 7 2015, 11:21 AM
  • adding rtsOptsSuggestions
  • change error message
  • change GHC.RTS.Flags accordingly
  • update docs
  • add testcases
This revision is now accepted and ready to land.Apr 7 2015, 11:21 AM

Phabricator seems to be confused here, while it states the base of your patch is 5aa57d0, the diff displayed is clearly applied to a version that predates a7ab16160, which is nearly a year old. That is probably why your build is failing.

The only thing I note is that a7ab16160 is D8 which is the most recent differential to touch rts/RtsFlags.c, but that really doesn't explain what is going on since D8 was closed long ago.

Javran updated this revision to Diff 2722.Apr 10 2015, 8:37 PM
  • rewrite test program, comments for clarification
  • remove empty stdout files in the testsuite
  • rebase -- hope this solve the failed build status
Javran updated this revision to Diff 2723.Apr 10 2015, 10:53 PM
  • adjust patch according to the latest changes
Javran updated this revision to Diff 2724.Apr 10 2015, 10:55 PM
  • spaces only
simonmar added inline comments.Apr 11 2015, 2:31 AM
includes/rts/Flags.h
181 ↗(On Diff #2724)

This is already available as rtsConfig.rts_opts_enabled, we probably don't need to add it here.

rts/ProfHeap.c
284

*censuses

Thanks for the comment @simonmar , I just get started working on this, could you reply those inline comments?
Still new to ghc codebase so might need some guides...

includes/rts/Flags.h
181 ↗(On Diff #2724)

Yeah, I think there are some duplicates but as I get started working on it, I'm not sure how to solve this: for hooks in rts/hooks, rtsConfig is reported as an undeclared identifier. One way would be to make this visible, but when I explore "rts/RtsFlags.h", I find rtsConfig is hidden intentionally. Does it sound good to make it visible?

rts/ProfHeap.c
284

sorry I'm not sure what you meant here... could you be more specific?

Javran updated this revision to Diff 2762.Apr 14 2015, 1:29 PM
  • drop RtsOptsEnabledEnum and use rtsConfig fields
Javran marked 2 inline comments as done.Apr 14 2015, 1:48 PM

Turns out we can make rtsConfig visible by simply including "RtsFlags.h". Not sure whether including it is fine.

This revision was automatically updated to reflect the committed changes.
ezyang added a subscriber: ezyang.Apr 17 2015, 5:17 AM

Heads up to reviewers, make sure you look at the test code too. I recently fixed two errors in this commit:

commit 619a324ed6a35708e4a3746f48c23b4f294e82df
Author: Edward Z. Yang <ezyang@cs.stanford.edu>
Date:   Fri Apr 17 03:14:41 2015 -0700

    Make T9579 parallel-safe and add build outputs to .gitignore
    
    Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>