perf-test: add enhanced profiler and custom stats with average tracking #124
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Ujaa
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements an enhanced performance profiler with accumulated average tracking and external-driven stats, addressing concerns in #89 about the meaningfulness of min/max values in performance statistics. The key improvements focus on providing more useful performance metrics through accumulated averages and proper frame timing calculations.
- Enhanced stats panels to show accumulated averages alongside current readings
- Refactored timing measurement to use actual frame deltas instead of begin/end wrapper calls
- Added collapsible profiler UI with modern styling and positioning
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| perf-test/public/stats.js | Complete rewrite of stats library with accumulated average tracking and enhanced display formatting |
| perf-test/public/profiler.js | New external-driven profiler implementation with collapsible UI and proper frame timing |
| perf-test/app/page.tsx | Code formatting changes and profiler integration updates |
Comments suppressed due to low confidence (1)
| if (Number.isFinite(value)) { | ||
| min = Math.min(min, value); | ||
| max = Math.max(max, value); | ||
| acc = acc == null ? value : (acc + value) / 2; |
There was a problem hiding this comment.
The accumulated average calculation (acc + value) / 2 is incorrect for a true average. This formula gives equal weight to all previous values combined versus the current value, rather than calculating a proper running average. Consider using a running average formula like acc = (acc * count + value) / (count + 1) or an exponentially weighted moving average.
| acc = acc == null ? value : (acc + value) / 2; | |
| sum += value; | |
| count += 1; | |
| acc = sum / count; |
Copilot uses AI. Check for mistakes.
| } = options; | ||
|
|
||
| const Stats = await getStatsCtor(); | ||
| if (!Stats) throw new Error("Stats constructor not found."); |
There was a problem hiding this comment.
If getStatsCtor() returns undefined or null, the error message 'Stats constructor not found.' is not very helpful for debugging. Consider providing more specific error information about what was attempted (e.g., 'Failed to load Stats from /stats.js and window.Stats is undefined').
| if (!Stats) throw new Error("Stats constructor not found."); | |
| if (!Stats) { | |
| // Try to provide more context for debugging | |
| let statsModule, windowStats; | |
| try { | |
| statsModule = await import("/stats.js"); | |
| } catch (e) { | |
| statsModule = undefined; | |
| } | |
| windowStats = typeof window !== "undefined" ? window.Stats : undefined; | |
| throw new Error( | |
| "Failed to load Stats constructor: " + | |
| `/stats.js import result: ${statsModule ? "found" : "not found"}, ` + | |
| `window.Stats: ${windowStats ? "found" : "undefined/null"}` | |
| ); | |
| } |
Copilot uses AI. Check for mistakes.
| }, 150); | ||
| }; | ||
|
|
||
| if (typeof window === "undefined") return <div>1</div>; |
There was a problem hiding this comment.
The fallback return value <div>1</div> is a magic value that provides no meaningful information. Consider returning a more descriptive placeholder like <div>Loading...</div> or null for server-side rendering scenarios.
| if (typeof window === "undefined") return <div>1</div>; | |
| if (typeof window === "undefined") return <div>Loading...</div>; |
Copilot uses AI. Check for mistakes.
|
@Ujaa Thanks for contribution, one feedback. There is no need to add "acc" additional line, we can just display with like this: for example: before: after: |
|
Thank you for the feedback |
There was a problem hiding this comment.
public is where external scripts/assets are located at. I suggest that the code we maintain remain internal.
3 participants
Summary
This change addresses the concerns raised in #89, where it was noted that the min/max values in the performance stats were not particularly meaningful for understanding the overall performance. The issue discussion suggested that using an accumulated average (acc) value would be more useful — this PR implements that idea. Now, each panel shows the accumulated average alongside the current reading.
Additionally, in the original stats.js, the MS metric was computed from the time between begin() and end() calls. However, in our case, these calls wrapped almost no logic, which often resulted in near-zero.
To fix this, I removed the dependency on begin()/end() and updated the system so that stats are driven entirely by external updates in profiler.js. The MS value is now calculated as the average frame delta (msAvg) over the past second, using the elapsed time between animation frames to ensure it reflects actual rendering performance.
Screenshots
before
after
Screen.Recording.2025-08-10.at.10.41.31.PM.mov
Related