performGC before getting RTS/GC stats #177
Conversation
The deprecated getGCStats had some nice documentation: "If you would
like your statistics as recent as possible, first run a performGC"
This is sadly missing from getRTSStats but I believe it still holds.
When regressing allocated over iters, I'd see a believable slope, but an
unbelievable y intercept, something like -300000 when the slope is 150
or so. That's because the first hundred or so measurements were 0 for
bytes allocated, as the RTS didn't bother to run a GC before the RTS
stats were sampled.
Now we do 3 samples:
1. performGC before the first one, to ensure it's up-to-date.
2. Do the second one after the action, without a performGC, so we can
get legit readings on the GC-related stats.
3. performGC and then sample again, so we can get up-to-date readings
on other metrics.
4. Carefully choose whether to diff start stats against the end stats
per- or post-GC.
Also included is a fix to the ToJSON Measurements instance, which
duplicated the mutator cpu seconds where GC cpu seconds should go.
avieth
force-pushed
the
avieth/fix_rts_stats
branch
from
52eaf27 to
6f18c08
Compare
RyanGlScott
left a comment
RyanGlScott
left a comment
There was a problem hiding this comment.
Thanks for noticing this, @avieth. It's unfortunate timing that this requires another breaking change after we just released criterion-1.3.0.0, but I suppose that's just how it is sometimes.
I've left some comments inline.
Criterion/Measurement.hs
Outdated
| -- | Try to get GC statistics, bearing in mind that the GHC runtime | ||
| -- will throw an exception if statistics collection was not enabled | ||
| -- using \"@+RTS -T@\". | ||
| -- If you need guaranteed up-to-date stats, call performGC first. |
There was a problem hiding this comment.
Put singlequotes around performGC (i.e., 'performGC') so that Haddock will link it.
| -- ^ Statistics gathered at the __end__ of a run. | ||
| -- ^ Statistics gathered at the __end__ of a run, post GC. | ||
| -> Maybe GCStatistics | ||
| -- ^ Statistics gathered at the __end__ of a run, pre GC. |
There was a problem hiding this comment.
The order here (first post-GC statistics, then pre-GC statistics) feels all wonky to me. Why not have the first argument be pre-GC statistics, then following by the post-GC statistics? We're going to need a breaking change anyway, so we might as well set things straight in the process.
There was a problem hiding this comment.
My reasoning was that the "later" argument always came first, so I kept with the trend. It doesn't matter so much to me so I'll change it up if you insist.
There was a problem hiding this comment.
Ah, that's a good point I hadn't considered. In that case, your order is fine, since we'd want to preserve the existing convention.
Criterion/Measurement.hs
Outdated
| , measMutatorCpuSeconds = diff endPostGC start gcStatsMutatorCpuSeconds | ||
| , measGcWallSeconds = diff endPreGC start gcStatsGcWallSeconds | ||
| , measGcCpuSeconds = diff endPreGC start gcStatsGcCpuSeconds | ||
| } where diff a b f = f a - f b |
There was a problem hiding this comment.
This b argument will always be start, yes? Why not keep it inline in diff?
Criterion/Measurement.hs
Outdated
| , measMutatorWallSeconds = diff endPostGC start gcStatsMutatorWallSeconds | ||
| , measMutatorCpuSeconds = diff endPostGC start gcStatsMutatorCpuSeconds | ||
| , measGcWallSeconds = diff endPreGC start gcStatsGcWallSeconds | ||
| , measGcCpuSeconds = diff endPreGC start gcStatsGcCpuSeconds |
There was a problem hiding this comment.
In the comments, can you include a justification for which things are diffed with endPreGC, and which things are diffed with endPostGC?
Then again, apparently I'm the only person who uses regressions on the GC stats :) else someone would probably have noticed this by now. |
|
Thanks! |
2 participants
performGC before getting RTS/GC stats
The deprecated getGCStats had some nice documentation: "If you would
like your statistics as recent as possible, first run a performGC"
This is sadly missing from getRTSStats but I believe it still holds.
When regressing allocated over iters, I'd see a believable slope, but an
unbelievable y intercept, something like -300000 when the slope is 150
or so. That's because the first hundred or so measurements were 0 for
bytes allocated, as the RTS didn't bother to run a GC before the RTS
stats were sampled.
Now we do 3 samples:
get legit readings on the GC-related stats.
on other metrics.
per- or post-GC.
Also included is a fix to the ToJSON Measurements instance, which
duplicated the mutator cpu seconds where GC cpu seconds should go.