Skip to content

feat(profiler): add flamegraph and icicle chart visualization - #166

Merged
Oaklight merged 1 commit into
masterfrom
worktree-feature+profiler-flamegraph
Sep 15, 2026
Merged

Oaklight merged 1 commit into
masterfrom
worktree-feature+profiler-flamegraph

Conversation

@Oaklight

Copy link
Copy Markdown
Owner

Summary

  • Stage 2 of feat: profiler module — stdlib-only cProfile wrapper with HTML output #139 — flamegraph and icicle chart HTML visualization styles for the profiler module
  • Call tree extraction via pstats.calc_callees() with DFS tree builder (recursion-safe via frozenset visited tracking)
  • Self-contained HTML output: zoom-on-click, reset zoom, search highlighting with dimming, hover tooltips, dark/light theme toggle
  • style="flamegraph" renders root at bottom (caller→callee stacked upward), style="icicle" renders root at top (callees stacked downward)
  • 19 new tests (5 call tree extraction, 9 flamegraph output, 5 icicle output), total 65

Test plan

  • make test-profiler — 65 tests pass
  • make lint — clean
  • pre-commit run --all-files — all hooks pass
  • Playwright visual testing: rendered both styles in browser, verified frame layout, tooltip, search highlighting, zoom/reset, theme toggle
  • CI lint + test

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.

@milo-oaklight milo-oaklight Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-clock

These 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-internal pstats tuple 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.
  • tottime in each tree node is the function's global tottime (across all callers), not edge-specific, while cumtime is 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.

@elena-oaklight elena-oaklight Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. zoomStack is dead code — onFrameClick pushes {data: FLAME_DATA, total: TOTAL_TIME} onto zoomStack every 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 call render(FLAME_DATA, TOTAL_TIME) directly in reset.

  2. sort_by and limit silently 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:

  • tottime in tree nodes comes from the function's global self-time (tt from stats_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 since cumtime is edge-specific via edge_ct.
  • total_tt CPython-internal attribute — same as Stage 1, consistent usage.

LGTM 👍

@Oaklight
Oaklight merged commit 83403d8 into master Sep 15, 2026
6 checks passed
@Oaklight
Oaklight deleted the worktree-feature+profiler-flamegraph branch September 15, 2026 01:48

@clementine-oaklight clementine-oaklight Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_tree reads stats_any.total_tt while _build_flame_html reads self.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-9 guard path.


Good stuff

  • Self-contained HTML with no external resources — proper zero-dep output.
  • frozenset visited set for cycle detection in _extract_call_tree handles recursive call graphs cleanly.
  • XSS-safe throughout: textContent for frame labels, escH() for tooltip innerHTML, setAttribute for data attrs.
  • 19 well-structured tests covering tree extraction, flamegraph output, icicle output, title escaping, and file write.
  • Existing test_output_html_unknown_style_raises correctly updated from "flamegraph" → "nonexistent".

@Oaklight

Copy link
Copy Markdown
Owner Author

All actionable and non-blocking review feedback addressed in PR #167:

  1. sort_by/limit silently ignored → now raises ValueError with flamegraph/icicle
  2. zoomStack dead code → removed entirely, reset renders from root directly
  3. cumtime_pct vs TOTAL_TIME divergence → unified to use total_tt consistently
  4. Edge tuple format → added CPython version comment
  5. Tooltip ambiguity → "Cumulative" → "Cumulative (path)"
  6. Empty-profile test → added for flamegraph

68 tests pass, pre-commit clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant