Fix uninitialized aggregate_sampling_time_ms in Stats struct #15820
Conversation
seyeong-han
requested review from
cccclai,
jackzhxng,
larryliu0820 and
mergennachin
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15820
Note: Links to docs will display an error until the docs builds have been completed. ❌ 5 New FailuresAs of commit 74639c6 with merge base c247604 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
This PR needs a
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug where the aggregate_sampling_time_ms member variable in the Stats struct was uninitialized, causing it to accumulate garbage values from uninitialized memory. The fix initializes the variable to 0 at declaration in both affected files.
- Initializes
aggregate_sampling_time_ms = 0in the mainStatsstruct definition - Applies the same fix to a duplicate
Statsstruct in the Qualcomm example code
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| extension/llm/runner/stats.h | Initialize aggregate_sampling_time_ms to 0 in the main Stats struct |
| examples/qualcomm/qaihub_scripts/llama/runner/runner.h | Initialize aggregate_sampling_time_ms to 0 in the Qualcomm-specific Stats struct |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…#15820) ## Summary Fixes a critical bug where `aggregate_sampling_time_ms` in the `Stats` struct was not initialized, causing it to contain garbage data from uninitialized memory. ## Problem The `aggregate_sampling_time_ms` member variable was declared without initialization: ```cpp long aggregate_sampling_time_ms; // uninitialized! ``` This resulted in absurd sampling time reports like: ``` Sampling time over 68 tokens: 8433599.048000 (seconds) // ~97.5 days! ``` The actual sampling time should have been milliseconds, not millions of seconds. Since the code accumulates timing data onto this variable (`stats_.aggregate_sampling_time_ms += ...`), the garbage initial value propagated through all calculations. ## Solution Initialize the variable to zero in both locations: `long aggregate_sampling_time_ms = 0;` ## Impact After this fix, sampling time metrics will report realistic values (e.g., 0.010-0.100 seconds for typical token generation) instead of garbage values.
2 participants
Summary
Fixes a critical bug where
aggregate_sampling_time_msin theStatsstruct was not initialized, causing it to contain garbage data from uninitialized memory.Problem
The
aggregate_sampling_time_msmember variable was declared without initialization:This resulted in absurd sampling time reports like:
The actual sampling time should have been milliseconds, not millions of seconds. Since the code accumulates timing data onto this variable (
stats_.aggregate_sampling_time_ms += ...), the garbage initial value propagated through all calculations.Solution
Initialize the variable to zero in both locations:
long aggregate_sampling_time_ms = 0;Impact
After this fix, sampling time metrics will report realistic values (e.g., 0.010-0.100 seconds for typical token generation) instead of garbage values.