feat(profiler): add flamegraph and icicle chart visualization - #166
Conversation
Stage 2 of #139 — caller/callee chain extraction via pstats.calc_callees() with DFS tree builder (recursion-safe), and self-contained HTML rendering for both flamegraph (root at bottom) and icicle (root at top) styles. Features: zoom-on-click, reset zoom, search highlighting with dimming, hover tooltips with timing details, dark/light theme toggle. Same inline CSS/JS pattern as the table style. 19 new tests covering call tree extraction, flamegraph output, and icicle output.
There was a problem hiding this comment.
Solid Stage 2 — the call-tree extraction via calc_callees() + DFS with frozenset visited tracking is the right approach for recursion-safe flamegraphs from cProfile data. Self-contained HTML continues the same inline CSS/JS pattern from Stage 1, and the 19 new tests cover the key surface area. CI green.
Actionable (non-blocking)
1. cumtime_pct and visual frame width use different total-time sources
_extract_call_tree() computes each node's cumtime_pct against stats_any.total_tt (pstats' internal sum of all functions' tottime), but _build_flame_html() passes self.total_time (wall-clock elapsed) as the JS TOTAL_TIME variable, which the flatten() function uses for frame width:
# _extract_call_tree
total_tt = stats_any.total_tt or 1e-9
...
"cumtime_pct": node_ct / total_tt * 100,
# _build_flame_html
total_time = self.total_time or 1e-9 # wall-clock
f"var TOTAL_TIME={total_time};\n"// flatten()
var w = parentWidth * (n.cumtime / totalTime); // uses wall-clockThese two values can diverge (e.g. if the profiled code does I/O or sleeps, wall-clock > CPU time). A frame's visual width would reflect its share of wall-clock time, but the tooltip percentage reflects its share of CPU time. Using the same source for both would keep things consistent — total_tt for CPU-centric view, or self.total_time for wall-clock.
2. zoomStack is populated but never consumed
onFrameClick pushes to zoomStack, but nothing ever reads from it:
function onFrameClick(e){
var node=this._node;
zoomStack.push({data:FLAME_DATA,total:TOTAL_TIME}); // always pushes original
render([node],node.cumtime);
}Reset Zoom ignores the stack and just re-renders from the original data:
document.getElementById('reset-zoom').addEventListener('click',function(){
zoomStack=[];
render(FLAME_DATA,TOTAL_TIME);
});If multi-level zoom-out is intended (click Reset to go back one level), the stack would need to be popped and its contents used. If not, zoomStack is dead code and can be removed. Also, each push always saves {data:FLAME_DATA,total:TOTAL_TIME} (the original root), not the current zoom state, so even the data being stored wouldn't support incremental unzoom.
3. sort_by and limit silently ignored for flamegraph/icicle
if style in ("flamegraph", "icicle"):
tree = self._extract_call_tree()
doc = self._build_flame_html(tree, title, inverted=(style == "icicle"))
else:
rows = self._extract_rows(sort_by=sort_by, limit=limit)A caller doing output_html(style="flamegraph", sort_by="tottime", limit=20) gets no feedback that those params are dropped. Worth either raising a ValueError / emitting a warning when incompatible params are passed, or documenting in the docstring that they only apply to style="table".
Minor
- Edge tuple format:
child_ct = edge[3] if len(edge) == 4 else edge[1]handles two CPython-internalpstatstuple shapes. A brief comment explaining the two formats would help future readers, since this is an undocumented internal detail that could shift between Python versions. tottimein each tree node is the function's global tottime (across all callers), not edge-specific, whilecumtimeis edge-specific. Standard for flamegraphs, but the tooltip shows both side-by-side without distinguishing — labeling the tooltip 'Self time (total)' vs 'Cumulative (this path)' would reduce ambiguity.
There was a problem hiding this comment.
CI green across lint + 3.10–3.13. Strong Stage 2 — self-contained flamegraph/icicle visualizations with no external deps, good test coverage (19 new tests), and clean integration with the existing output_html() API. The call tree extraction with frozenset visited tracking handles recursion correctly, and the edge-specific cumtime propagation is well done.
Actionable:
-
zoomStackis dead code —onFrameClickpushes{data: FLAME_DATA, total: TOTAL_TIME}ontozoomStackevery click, but nothing ever pops or reads it. "Reset Zoom" just clears the array and re-renders from root data. Either wire up a "Back" button that pops the stack for zoom-out-one-level, or drop the stack and just callrender(FLAME_DATA, TOTAL_TIME)directly in reset. -
sort_byandlimitsilently ignored for flamegraph/icicle —output_html(style="flamegraph", limit=20)accepts the params without error but the flamegraph branch never calls_extract_rows(). Consider either raising/warning when those params are passed with a non-table style, or documenting that they only apply to tables.
Non-blocking observations:
tottimein tree nodes comes from the function's global self-time (ttfromstats_any.stats[key]), not edge-specific self-time — so if a function is called from multiple parents, every occurrence shows the full tottime. This is a cProfile limitation (pstats edges don't carry self-time), but worth a brief docstring note sincecumtimeis edge-specific viaedge_ct.total_ttCPython-internal attribute — same as Stage 1, consistent usage.
LGTM 👍
There was a problem hiding this comment.
Review — PR #166: feat(profiler): add flamegraph and icicle chart visualization (Stage 2 of #139)
CI: ✅ Green (lint + 3.10–3.13)
Verdict: LGTM — approve with two actionable items below.
Actionable
1. sort_by / limit silently ignored for flamegraph/icicle
if style in ("flamegraph", "icicle"):
tree = self._extract_call_tree()
doc = self._build_flame_html(tree, title, inverted=(style == "icicle"))
else:
rows = self._extract_rows(sort_by=sort_by, limit=limit)
doc = self._build_table_html(rows, title)A caller doing output_html(style="flamegraph", limit=20) gets no error but the limit has no effect. Either raise ValueError when those params are explicitly passed with a non-table style, or document in the docstring that they only apply to style="table".
2. zoomStack is accumulated but never popped — no "zoom out one level"
function onFrameClick(e){
var node=this._node;
zoomStack.push({data:FLAME_DATA,total:TOTAL_TIME});
render([node],node.cumtime);
}Every click pushes {data:FLAME_DATA, total:TOTAL_TIME} (always the global original), and "Reset Zoom" clears the entire stack. There's no way to step back one level — users who click three levels deep must reset all the way to root. The stack itself is dead code since it always stores the same global state.
Fix: either push the current view state and add a "Back" / right-click-to-zoom-out interaction that pops, or drop the stack entirely and just re-render from FLAME_DATA on reset (which is what already happens).
Non-blocking
-
Dual access to
total_tt—_extract_call_treereadsstats_any.total_ttwhile_build_flame_htmlreadsself.total_time. Same underlying value, but the inconsistent access path is a minor readability nit. -
Large profiles → large HTML — the full call tree is serialized to JSON with no depth/node cap. JS filters sub-pixel frames (
w < 0.3) at render time, but the payload itself can get heavy for complex profiles. Worth considering a Python-side cap in a future iteration. -
No empty-profile test for flamegraph — profiling a no-op and rendering as flamegraph would exercise the
total_tt or 1e-9guard path.
Good stuff
- Self-contained HTML with no external resources — proper zero-dep output.
frozensetvisited set for cycle detection in_extract_call_treehandles recursive call graphs cleanly.- XSS-safe throughout:
textContentfor frame labels,escH()for tooltip innerHTML,setAttributefor data attrs. - 19 well-structured tests covering tree extraction, flamegraph output, icicle output, title escaping, and file write.
- Existing
test_output_html_unknown_style_raisescorrectly updated from"flamegraph"→"nonexistent".
|
All actionable and non-blocking review feedback addressed in PR #167:
68 tests pass, pre-commit clean. |
Summary
pstats.calc_callees()with DFS tree builder (recursion-safe viafrozensetvisited tracking)style="flamegraph"renders root at bottom (caller→callee stacked upward),style="icicle"renders root at top (callees stacked downward)Test plan
make test-profiler— 65 tests passmake lint— cleanpre-commit run --all-files— all hooks pass