-
Notifications
You must be signed in to change notification settings - Fork 468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SCP-2804: budget profiling #4024
Conversation
let printMethod = getPrintMethod mode | ||
case outp of | ||
FileOutput file -> writeFile file . Prelude.show . printMethod $ prog | ||
StdOutput -> print . printMethod $ prog | ||
|
||
writeToFileOrStd :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently still a bit wrong, since it overwrites any previous writes. I want to rewrite this to pass around a handle instead.
@@ -1 +1,75 @@ | |||
[entering addInt, exiting addInt] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these tests were actually meant to show the code that is generated so that we can see whether we're putting things in the right place, but instead they were showing the result of evaluating the (not-fully-applied) functions, which wasn't really showing us anything.
ccee19c
to
f049275
Compare
Hmmm, stylish haskell wants to remove a necessary language extension pragma, despite |
Ah, I found the problem: a HLS bug that inserted the pragma in a random place in the file instead of the top, which then confuses stylish. |
4169278
to
bcb87a7
Compare
@@ -160,45 +165,44 @@ runApply (ApplyOptions inputfiles ifmt outp ofmt mode) = do | |||
---------------- Evaluation ---------------- | |||
|
|||
runEval :: EvalOptions -> IO () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function just keeps getting more complicated with all the different branches. I suppose there's not much to be done though, because it has to handle all the different combinations of flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I tried to make it less complicated, by e.g. handling the budget options once rather than repeatedly, but it's a mess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks plausible, although I'd really have to run it to make sure that it's behaving sensibly. We should also run the benchmarks to confirm that it's not affecting the performance in the default mode since there seem to be quite a number of changes in the CEK machine.
Right v -> writeToFileOrStd outputMode (show (getPrintMethod printMode v)) | ||
case budgetMode of | ||
Silent -> pure () | ||
Verbose _ -> printBudgetState t cekModel budget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the budget state is always going to stdout, but the other things can be directed to a file instead. Is there likely to be a case where we really want that to happen, ie where we want to see the budget state on the screen but also want to save other info in a file? If that's not the case, would it not be simpler just to write everything to stdout and use shell redirection if we want it to go into a file (or a pipe if we want to feed it to another command)? That would also deal with the issue of having to pass a handle around. The --of
option is used in some other places, but that's mostly for binary output like flat that you never want to see on your screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a bug. I need to pipe through a handle everywhere so things go to the right place. Maybe yes we should just send everything to stdout by default. We also need better verbosity options so you can get just the bit you want...
@@ -501,17 +503,17 @@ runCekM | |||
-> (forall s. GivenCekReqs uni fun s => CekM uni fun s a) | |||
-> (Either (CekEvaluationException uni fun) a, cost, [Text]) | |||
runCekM (MachineParameters costs runtime) (ExBudgetMode getExBudgetInfo) (EmitterMode getEmitterMode) a = runST $ do | |||
exBudgetMode <- getExBudgetInfo | |||
emitter <- getEmitterMode | |||
ExBudgetInfo{_exBudgetModeSpender, _exBudgetModeGetFinal, _exBudgetModeGetCumulative} <- getExBudgetInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still want the underscores here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ugly but effective and I don't feel like messing with it right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, I don't have any suggestions.
bcb87a7
to
02c82b4
Compare
Pushed a couple more commits:
Putting this together, I just did something like:
I can't quite make this one line because of the dodgy output processing by The last thing I'd like to do is move the executable to |
-- A wrapper around encoding a reocrd. `cassava` insists on including a trailing newline, which is | ||
-- annoying since we're recording the output line-by-line. | ||
encodeRecord :: CSV.ToRecord a => a -> T.Text | ||
encodeRecord a = T.stripEnd $ T.decodeUtf8 $ BSL.toStrict $ BS.toLazyByteString $ CSV.encodeRecord a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a quick look at the documentation to see why we have to go through so many types. It says that encodeRecord
returns Data.Binary.Builder
, but I think BS.toLazyByteString
is expecting Data.ByteString.Builder
. I assume these types are identical, but I couldn't quite see where that was happening. Anyway, should we be using Data.Binary.Builder.toLazyByteString
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
binary
re-exports the Builder
type from bytestring
. So yeah, they're the same. I don't think it's that weird to use the bytestring-exported versions of the fucntions...
modifySTRef logsRef (`DList.snoc` withTime) | ||
pure $ CekEmitterInfo emitter (DList.toList <$> readSTRef logsRef) | ||
|
||
instance CSV.ToField ExCPU where | ||
toField (ExCPU t) = CSV.toField $ toInteger t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of annoying that we don't have unExCPU
for this. I've been having to put in lots of matches to extract the numbers recently.
This PR is a bit of a mess, since it goes across a few things.
The first couple of commits do a few things:
Then I wanted to actually get this conveniently, so I dived into the executables. I ended up doing a fair bit of drive-by refactoring (sorry), but the end result is a workflow like this:
dump-plc
uplc evaluate --if flat-namedDeBruijn -i ... --trace-mode LogsWithBudgets
Then once we have some log processors we can just pipe it into that.
Pre-submit checklist: