Io report caller interval#60
Conversation
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
|
Thanks for the PR. I agree with the drift issue: the current internal sampling can accumulate extra delay from IOReport calls, and that part is worth fixing. I want to preserve the public The duration-based sampling and internal smoothing are intentional. I tuned this path so short/cyclic loads produce graphs close to Activity Monitor, so replacing it with a fully caller-owned loop is not a direct swap. For the manual sampling API, the tricky part is stale windows: if the caller stops polling for a while, “metrics since last call” can become an average over a long gap instead of current metrics. I’ll keep that in mind while reworking this PR so we can support manual sampling without breaking the existing behavior. |
|
I think the manual API should be added, but without replacing the existing My preferred shape is: sampler.get_metrics(duration_ms) -> Result<Metrics>
sampler.get_metrics_now(stale_after_ms) -> Result<Option<Metrics>>
This keeps the public API compatible, avoids a separate |
|
Final update before merge: I reworked this PR along the lines discussed above. What I am taking:
What I am not taking:
Thanks again for the PR. I am going to merge this version. |
Not sure how Activity Monitor is related since it doesn't show such metrics. 4x smoothing was added after my report of the metrics are not accurate, this is correct. But the real cause of this incorrectness was that the older version collects data for only 80 ms window. So the proper fix is not 4x smoothing, the real fix is make the whole interval a metric window which is already happens. I have described this in the pr description. |
This PR is a port of the timer-related changes from #59.
Sampling and calculation fixes
Sampling now matches the requested interval
Previously, a requested
1 sinterval was internally handled as4 x 250 mssamples plus an extra averaging pass, with the sampling loop implemented inIOReport::get_sample.In this branch, the same
1 sinterval is sampled as an actual1 sinterval instead of being split into smaller internal windows.That earlier
4 x 250 msscheme was introduced as a workaround for the behavior described in issue #10. However, the later investigation showed that the original problem had a different cause: the older implementation sampled only a fixed 80 ms window and then slept for the remainder of the interval, whileget_metricsused that short window as the sampling duration.Now that sampling is structured around consecutive real samples and their delta, the reported metrics are derived from the actual elapsed interval itself. Because of that, the extra
4 x 250 mssmoothing loop is no longer necessary. It also allowed theIOReportsampling path to become substantially simpler by removing the internal multi-sample orchestration that used to live inIOReport::get_sample.Interval management is now fully owned by the caller
Another related change is that sampling no longer sleeps internally.
Previously,
IOReportitself managed timing by sleeping between internal samples inIOReport::get_sample. In this branch, that responsibility is moved out ofIOReport::get_sample, which now just computes the delta between the previous sample and the current one. It does not block, does not sleep, and does not decide how often sampling should happen.This is a better separation of responsibilities:
IOReportis responsible only for sample-to-sample delta calculation;Reduced interval drift
The measurements and timings were changed since #59 to account for the 100ms minimum interval.
The same change also reduces drift across repeated short samples.
Startup time, which is not changed in this version:
Excluding the 100ms interval, it is about
2.925s - 100ms = 2.825s.With
-i 100 -s 100, the requested sampling time is100 x 100ms = 10s:The total runtime excluding startup dropped from about
21.015s - 2.825s = 18.19sto about12.821s - 2.825s = 9.996s, which perfectly matches the requested 10s. In the old versions, there was a large 8s drift due to ignoring theIOReportCreateSamplestime itself multiplied by 4.