Profiteur timing visualisation #3951
Conversation
| , | ||
| ( "Timing" | ||
| , | ||
| [ [ctxt| request*,booster|kore>rewrite*,success|failure|abort|detail |] |
There was a problem hiding this comment.
In order to distinguish requests easily, I've added a new request <id> context. @geo2a @jberthold please let me know if this looks ok. If so, will update the docs and the other levelToContext mappings accordingly.
There was a problem hiding this comment.
I see why you need this ... wanted to suggest adding the ID to the execute, simplify, implies, etc contexts that we already have instead, but that would be inferior and cause issues with the sub-requests we emit.
In the same change, we can also add a CtxStartup and CtxShutdown, Startup grouping the ceil-related messages which are currently info.
There was a problem hiding this comment.
I thought we already have a [ceil] context?? do we want [startup][ceil]? what would be the advantage/difference?
There was a problem hiding this comment.
i just noticed we no longer have the ceil context, @jberthold what was the reason for removing it?
There was a problem hiding this comment.
As mentioned, the ceil had a double meaning, indicating a ceil rule (with ID) and indicating the ceil analysis phase (which could be included in said startup phase).
I was planning to amend that ceil issue in a follow-up but will wait for your change and what we decide.
| , | ||
| ( "Timing" | ||
| , | ||
| [ [ctxt| request*,booster|kore>rewrite*,success|failure|abort|detail |] |
There was a problem hiding this comment.
I see why you need this ... wanted to suggest adding the ID to the execute, simplify, implies, etc contexts that we already have instead, but that would be inferior and cause issues with the sub-requests we emit.
In the same change, we can also add a CtxStartup and CtxShutdown, Startup grouping the ceil-related messages which are currently info.
booster/library/Booster/CLOptions.hs
Outdated
| @@ -289,6 +290,15 @@ levelToContext = | |||
| [ [ctxt| booster>(simplification *|function *)>warning |] | |||
| ] | |||
| ) | |||
| , | |||
| ( "Timing" | |||
There was a problem hiding this comment.
This flag could also include what is currently --print-stats (and should get its own context).
There was a problem hiding this comment.
I decided to remove --print-stats in favour of -l TimingStats as this is more uniform with how we invoke other logging options, but putting this filer in the -l Timing option would IMO be too verbose/unsuitable if we just want the stats but not the success/abort logging needed for the new timing tool
There was a problem hiding this comment.
The code to parse --print-stats was just for backward compatibility. We can scrap it if that's not important.. (not sure who uses it except myself)
There was a problem hiding this comment.
BTW run-with-tarball.sh uses --print-stats, maybe adjust that on the same PR
This PR adds a small utility for turning JSON logs into timing profiles visualised via the https://github.com/jaspervdj/profiteur library