Random fixes

Author: pablogsal

Doc/library/profiling.sampling.rst

@@ -1041,7 +1041,7 @@ Some call paths may disappear entirely between profiles. These are called
 **elided stacks** and occur when optimizations eliminate code paths or certain
 branches stop executing. If elided stacks are present, an elided toggle appears
 allowing you to switch between the main differential view and an elided-only
-view that shows just the removed paths (colored deep red).
+view that shows just the removed paths (colored purple).
 
 
 Gecko format

Lib/profiling/sampling/_flamegraph_assets/flamegraph.css

@@ -28,6 +28,23 @@
   --diff-elided: #b39ddb;
 }
 
+/* Dark mode differential colors - adjusted for contrast against dark backgrounds */
+[data-theme="dark"] {
+  --diff-regression-deep: #ef5350;
+  --diff-regression-medium: #e57373;
+  --diff-regression-light: #ef9a9a;
+  --diff-regression-verylight: #ffcdd2;
+
+  --diff-improvement-deep: #42a5f5;
+  --diff-improvement-medium: #64b5f6;
+  --diff-improvement-light: #90caf9;
+  --diff-improvement-verylight: #bbdefb;
+
+  --diff-neutral: #757575;
+  --diff-new: #b39ddb;
+  --diff-elided: #ce93d8;
+}
+
 /* --------------------------------------------------------------------------
    Layout Overrides (Flamegraph-specific)
    -------------------------------------------------------------------------- */

Lib/profiling/sampling/_flamegraph_assets/flamegraph.js

@@ -986,7 +986,7 @@ function setupElidedToggle(data) {
     return;
   }
 
-  elidedFlamegraphData = resolveStringIndices(elidedFlamegraph, elidedFlamegraph.baseline_strings);
+  elidedFlamegraphData = resolveStringIndices(elidedFlamegraph, elidedFlamegraph.strings);
 
   const toggleElided = document.getElementById('toggle-elided');
   if (toggleElided) {

Lib/profiling/sampling/cli.py

@@ -59,8 +59,8 @@ class CustomFormatter(
 class DiffFlamegraphAction(argparse.Action):
     """Custom action for --diff-flamegraph that sets both format and baseline path."""
     def __call__(self, parser, namespace, values, option_string=None):
-        setattr(namespace, 'format', 'diff_flamegraph')
-        setattr(namespace, 'diff_baseline', values)
+        namespace.format = 'diff_flamegraph'
+        namespace.diff_baseline = values
 
 
 _HELP_DESCRIPTION = """Sample a process's stack frames and generate profiling data.
@@ -490,7 +490,7 @@ def _add_format_options(parser, include_compression=True, include_binary=True):
             dest="format",
             help="Generate high-performance binary format (use 'replay' command to convert)",
         )
-    parser.set_defaults(format="pstats")
+    parser.set_defaults(format="pstats", diff_baseline=None)
 
     if include_compression:
         output_group.add_argument(
@@ -564,7 +564,7 @@ def _create_collector(format_type, sample_interval_usec, skip_idle, opcodes=Fals
     """Create the appropriate collector based on format type.
 
     Args:
-        format_type: The output format ('pstats', 'collapsed', 'flamegraph', 'gecko', 'heatmap', 'binary')
+        format_type: The output format ('pstats', 'collapsed', 'flamegraph', 'gecko', 'heatmap', 'binary', 'diff_flamegraph')
         sample_interval_usec: Sampling interval in microseconds
         skip_idle: Whether to skip idle samples
         opcodes: Whether to collect opcode information (only used by gecko format
@@ -981,7 +981,7 @@ def _handle_attach(args):
         args.format, args.sample_interval_usec, skip_idle, args.opcodes,
         output_file=output_file,
         compression=getattr(args, 'compression', 'auto'),
-        diff_baseline=getattr(args, 'diff_baseline', None)
+        diff_baseline=args.diff_baseline
     )
 
     with _get_child_monitor_context(args, args.pid):
@@ -1060,7 +1060,7 @@ def _handle_run(args):
         args.format, args.sample_interval_usec, skip_idle, args.opcodes,
         output_file=output_file,
         compression=getattr(args, 'compression', 'auto'),
-        diff_baseline=getattr(args, 'diff_baseline', None)
+        diff_baseline=args.diff_baseline
     )
 
     with _get_child_monitor_context(args, process.pid):
@@ -1211,7 +1211,7 @@ def _handle_replay(args):
 
         collector = _create_collector(
             args.format, interval, skip_idle=False,
-            diff_baseline=getattr(args, 'diff_baseline', None)
+            diff_baseline=args.diff_baseline
         )
 
         def progress_callback(current, total):

Lib/profiling/sampling/stack_collector.py

@@ -449,6 +449,8 @@ class DiffFlamegraphCollector(FlamegraphCollector):
 
     def __init__(self, sample_interval_usec, *, baseline_binary_path, skip_idle=False):
         super().__init__(sample_interval_usec, skip_idle=skip_idle)
+        if not os.path.exists(baseline_binary_path):
+            raise ValueError(f"Baseline file not found: {baseline_binary_path}")
         self.baseline_binary_path = baseline_binary_path
         self._baseline_collector = None
         self._elided_paths = set()
@@ -477,7 +479,8 @@ def _aggregate_path_samples(self, root_node, path=None):
         stats = {}
 
         for func, node in root_node["children"].items():
-            func_key = (func[0], func[2])
+            filename, _lineno, funcname = func
+            func_key = (filename, funcname)
             path_key = path + (func_key,)
 
             total_samples = node.get("samples", 0)
@@ -508,15 +511,22 @@ def _convert_to_flamegraph_format(self):
             self._load_baseline()
 
         current_flamegraph = super()._convert_to_flamegraph_format()
-        if self._total_samples == 0:
-            return current_flamegraph
 
         current_stats = self._aggregate_path_samples(self._root)
         baseline_stats = self._aggregate_path_samples(self._baseline_collector._root)
 
-        # Scale baseline values to make them comparable when sample counts differ
-        scale = (self._total_samples / self._baseline_collector._total_samples
-                if self._baseline_collector._total_samples > 0 else 1.0)
+        # Scale baseline values to make them comparable, accounting for both
+        # sample count differences and sample interval differences.
+        baseline_total = self._baseline_collector._total_samples
+        if baseline_total > 0 and self._total_samples > 0:
+            current_time = self._total_samples * self.sample_interval_usec
+            baseline_time = baseline_total * self._baseline_collector.sample_interval_usec
+            scale = current_time / baseline_time
+        elif baseline_total > 0:
+            # Current profile is empty - use interval-based scale for elided display
+            scale = self.sample_interval_usec / self._baseline_collector.sample_interval_usec
+        else:
+            scale = 1.0
 
         self._annotate_nodes_with_diff(current_flamegraph, current_stats, baseline_stats, scale)
         self._add_elided_flamegraph(current_flamegraph, current_stats, baseline_stats, scale)
@@ -575,7 +585,7 @@ def _is_promoted_root(self, data):
 
     def _add_elided_flamegraph(self, current_flamegraph, current_stats, baseline_stats, scale):
         """Calculate elided paths and add elided flamegraph to stats."""
-        self._elided_paths = set(baseline_stats.keys()) - set(current_stats.keys())
+        self._elided_paths = baseline_stats.keys() - current_stats.keys()
 
         current_flamegraph["stats"]["elided_count"] = len(self._elided_paths)
 
@@ -585,23 +595,39 @@ def _add_elided_flamegraph(self, current_flamegraph, current_stats, baseline_sta
                 current_flamegraph["stats"]["elided_flamegraph"] = elided_flamegraph
 
     def _build_elided_flamegraph(self, baseline_stats, scale):
-        """Build flamegraph containing only elided paths from baseline."""
+        """Build flamegraph containing only elided paths from baseline.
+
+        This re-runs the base conversion pipeline on the baseline collector
+        to produce a complete formatted flamegraph, then prunes it to keep
+        only elided paths.
+        """
         if not self._baseline_collector or not self._elided_paths:
             return None
 
-        baseline_data = self._baseline_collector._convert_to_flamegraph_format()
+        # Suppress source line collection for elided nodes - these functions
+        # no longer exist in the current profile, so source lines from the
+        # current machine's filesystem would be misleading or unavailable.
+        orig_get_source = self._baseline_collector._get_source_lines
+        self._baseline_collector._get_source_lines = lambda func: None
+        try:
+            baseline_data = self._baseline_collector._convert_to_flamegraph_format()
+        finally:
+            self._baseline_collector._get_source_lines = orig_get_source
 
         # Remove non-elided nodes and recalculate values
         if not self._extract_elided_nodes(baseline_data, path=()):
             return None
 
         self._add_elided_metadata(baseline_data, baseline_stats, scale, path=())
 
-        baseline_data["stats"].update(self.stats)
+        # Merge only profiling metadata, not thread-level stats
+        for key in ("sample_interval_usec", "duration_sec", "sample_rate",
+                    "error_rate", "missed_samples", "mode"):
+            if key in self.stats:
+                baseline_data["stats"][key] = self.stats[key]
         baseline_data["stats"]["is_differential"] = True
         baseline_data["stats"]["baseline_samples"] = self._baseline_collector._total_samples
         baseline_data["stats"]["current_samples"] = self._total_samples
-        baseline_data["baseline_strings"] = self._baseline_collector._string_table.get_strings()
 
         return baseline_data
 
@@ -625,8 +651,9 @@ def _extract_elided_nodes(self, node, path):
                     total_value += child.get("value", 0)
             node["children"] = elided_children
 
-            # Recalculate value based on remaining children
-            if elided_children:
+            # Recalculate value for structural (non-elided) ancestor nodes;
+            # elided nodes keep their original value to preserve self-samples
+            if elided_children and not is_elided:
                 node["value"] = total_value
 
         # Keep this node if it's elided or has elided descendants
@@ -654,7 +681,14 @@ def _add_elided_metadata(self, node, baseline_stats, scale, path):
             node["diff"] = 0
 
         node["self_time"] = 0
-        node["diff_pct"] = -100.0
+        # Elided paths have zero current self-time, so the change is always
+        # -100% when there was actual baseline self-time to lose.
+        # For internal nodes with no baseline self-time, use 0% to avoid
+        # misleading tooltips.
+        if baseline_self > 0:
+            node["diff_pct"] = -100.0
+        else:
+            node["diff_pct"] = 0.0
 
         if "children" in node and node["children"]:
             for child in node["children"]:

Lib/test/test_profiling/test_sampling_profiler/mocks.py

@@ -105,6 +105,8 @@ def make_diff_collector_with_mock_baseline(baseline_samples):
     for sample in baseline_samples:
         baseline.collect(sample)
 
-    diff = DiffFlamegraphCollector(1000, baseline_binary_path="baseline.bin")
+    # Path is unused since we inject _baseline_collector directly;
+    # use __file__ as a dummy path that passes the existence check.
+    diff = DiffFlamegraphCollector(1000, baseline_binary_path=__file__)
     diff._baseline_collector = baseline
     return diff

Lib/test/test_profiling/test_sampling_profiler/test_collectors.py

@@ -1386,7 +1386,7 @@ def test_diff_flamegraph_elided_stacks(self):
         self.assertIn("elided_flamegraph", data["stats"])
         elided = data["stats"]["elided_flamegraph"]
         self.assertTrue(elided["stats"]["is_differential"])
-        self.assertIn("baseline_strings", elided)
+        self.assertIn("strings", elided)
 
         elided_strings = elided.get("strings", [])
         children = elided.get("children", [])
@@ -1433,7 +1433,7 @@ def test_diff_flamegraph_function_matched_despite_line_change(self):
         self.assertAlmostEqual(child["diff_pct"], 0.0, places=1)
 
     def test_diff_flamegraph_empty_current(self):
-        """Empty current profile returns early without crashing."""
+        """Empty current profile still produces differential metadata and elided paths."""
         baseline_frames = [
             MockInterpreterInfo(0, [
                 MockThreadInfo(1, [MockFrameInfo("file.py", 10, "func1")])
@@ -1446,6 +1446,10 @@ def test_diff_flamegraph_empty_current(self):
         data = diff._convert_to_flamegraph_format()
         self.assertIn("name", data)
         self.assertEqual(data["value"], 0)
+        # Differential metadata should still be populated
+        self.assertTrue(data["stats"]["is_differential"])
+        # All baseline paths should be elided since current is empty
+        self.assertGreater(data["stats"]["elided_count"], 0)
 
     def test_diff_flamegraph_empty_baseline(self):
         """Empty baseline with non-empty current uses scale=1.0 fallback."""
@@ -1585,7 +1589,7 @@ def test_diff_flamegraph_elided_preserves_metadata(self):
         self.assertIn("per_thread_stats", elided["stats"])
         self.assertIn("baseline_samples", elided["stats"])
         self.assertIn("current_samples", elided["stats"])
-        self.assertIn("baseline_strings", elided)
+        self.assertIn("strings", elided)
 
         elided_strings = elided.get("strings", [])
         children = elided.get("children", [])