[Kineto] Add lookback window and sampling interval as config options - #1561
Conversation
| std::optional<nanoseconds> parseMilliseconds(std::string_view text) noexcept { | ||
| double count = 0; | ||
| const auto [parseEnd, error] = | ||
| std::from_chars(text.begin(), text.end(), count); |
There was a problem hiding this comment.
from_chars needs raw pointers. string_view::begin()/end() are not raw pointers on MSVC, so this breaks the Windows build. Please use text.data() and text.data() + text.size().
| counterDataSize.pMetricNames = metricNamePtrs_.data(); | ||
| counterDataSize.numMetrics = metricNamePtrs_.size(); | ||
| counterDataSize.maxSamples = kMaxSamplesPerDecode; | ||
| counterDataSize.maxSamples = static_cast<uint32_t>( |
There was a problem hiding this comment.
maxSamples is a uint32_t, but this calculation can be much larger. A large lookback or tiny interval can wrap, possibly to zero, or request a huge buffer. Can we validate it before casting?
| counterDataSize.numMetrics = metricNamePtrs_.size(); | ||
| counterDataSize.maxSamples = kMaxSamplesPerDecode; | ||
| counterDataSize.maxSamples = static_cast<uint32_t>( | ||
| std::max<int64_t>(1, config_.lookbackWindow / config_.samplingInterval)); |
There was a problem hiding this comment.
GA100 uses the fixed 1M-cycle interval, but this sizes the buffer using the requested nanosecond interval, which GA100 ignores. This makes the lookback inaccurate, and zero causes division by zero. Can we use the effective GA100 cadence or reject this setting?
| int32_t performanceMetricsDeviceId_{-1}; | ||
| std::chrono::nanoseconds performanceMetricsSamplingInterval_{ | ||
| std::chrono::milliseconds{1}}; | ||
| std::chrono::nanoseconds performanceMetricsLookbackWindow_{ |
There was a problem hiding this comment.
nit: With the 1ms interval above, this 10-second default increases capacity from 1,024 to 10,000 samples. Do we have memory and decode-cost measurements for this increase?
There was a problem hiding this comment.
thanks, updated. This was being used as the default for cuspy but we shouldn't need such a big buffer here.
Last one! Includes the following commits: - [Kineto] Exclude test on windows (pytorch/kineto#1575) 638a3ef - Opt-in per-thread activity buffers on the ROCm HIP path (pytorch/kineto#1573) 5dc9333 - Parse DISABLE_CUPTI_LAZY_REINIT by value (pytorch/kineto#1568) f140a13 - [Kineto] Add lookback window and sampling interval as config options (pytorch/kineto#1561) fcda441 - [xpupti] Don't emit ac2g flow endpoints on XPU_DRIVER records (pytorch/kineto#1549) f0df071 Pull Request resolved: #198560 Approved by: https://github.com/sanrise Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
These were introduced as inputs in pytorch/pytorch#196065, but are not yet being parsed by Kineto (only cupti monitor/cuspy).
Lookback window is a proxy for buffer capacity, which we previously fixed as
maxSamplesForDecodebut is now calculated asmax(1, lookbackWindow / samplingInterval).