Skip to content

Commit d0b40b5

Browse files
pablogsalmiss-islington
authored andcommitted
gh-154085: Avoid duplicating diff line values (GH-154099)
(cherry picked from commit 53381bc) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
1 parent 747e518 commit d0b40b5

3 files changed

Lines changed: 164 additions & 6 deletions

File tree

Lib/profiling/sampling/stack_collector.py

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -660,9 +660,33 @@ def _add_diff_data_to_node(self, node, path, current_stats, baseline_stats, scal
660660
current_data = current_stats.get(path_key, {"total": 0, "self": 0})
661661
baseline_data = baseline_stats.get(path_key, {"total": 0, "self": 0})
662662

663-
current_self = current_data["self"]
664-
baseline_self = baseline_data["self"] * scale
665-
baseline_total = baseline_data["total"] * scale
663+
current_self = node.get("self", 0)
664+
current_total = node.get("value", 0)
665+
666+
current_nonself = current_total - current_self
667+
aggregate_nonself = current_data["total"] - current_data["self"]
668+
669+
# Allocate self and descendant samples separately. Line-number
670+
# changes can split one function path into several rendered nodes,
671+
# and using independent weights for self and inclusive totals could
672+
# otherwise assign a node more self samples than total samples.
673+
self_weight = self._sample_weight(
674+
current_self,
675+
current_data["self"],
676+
current_total,
677+
current_data["total"],
678+
)
679+
nonself_weight = self._sample_weight(
680+
current_nonself,
681+
aggregate_nonself,
682+
current_total,
683+
current_data["total"],
684+
)
685+
baseline_self = baseline_data["self"] * scale * self_weight
686+
baseline_nonself = (
687+
baseline_data["total"] - baseline_data["self"]
688+
) * scale * nonself_weight
689+
baseline_total = baseline_self + baseline_nonself
666690

667691
diff = current_self - baseline_self
668692
if baseline_self > 0:
@@ -682,6 +706,14 @@ def _add_diff_data_to_node(self, node, path, current_stats, baseline_stats, scal
682706
for child in node["children"]:
683707
self._add_diff_data_to_node(child, path_key, current_stats, baseline_stats, scale)
684708

709+
@staticmethod
710+
def _sample_weight(value, aggregate, fallback_value, fallback_aggregate):
711+
if aggregate > 0:
712+
return value / aggregate
713+
if fallback_aggregate > 0:
714+
return fallback_value / fallback_aggregate
715+
return 0
716+
685717
def _is_promoted_root(self, data):
686718
"""Check if the data represents a promoted root node."""
687719
return "filename" in data and "funcname" in data
@@ -758,6 +790,9 @@ def _extract_elided_nodes(self, node, path):
758790
# elided nodes keep their original value to preserve self-samples
759791
if elided_children and not is_elided:
760792
node["value"] = total_value
793+
node["self"] = 0
794+
node.pop("opcodes", None)
795+
node.pop("thread_opcodes", None)
761796

762797
# Keep this node if it's elided or has elided descendants
763798
return is_elided or bool(node.get("children"))
@@ -773,9 +808,13 @@ def _add_elided_metadata(self, node, baseline_stats, scale, path):
773808
baseline_self = 0
774809
baseline_total = 0
775810
if func_key and current_path in baseline_stats:
776-
baseline_data = baseline_stats[current_path]
777-
baseline_self = baseline_data["self"] * scale
778-
baseline_total = baseline_data["total"] * scale
811+
baseline_total = node.get("value", 0) * scale
812+
813+
# Matched nodes are retained only as structural ancestors. Their
814+
# own samples are still present in the current profile and must
815+
# not be reported as disappeared.
816+
if current_path in self._elided_paths:
817+
baseline_self = node.get("self", 0) * scale
779818

780819
node["baseline"] = baseline_self
781820
node["baseline_total"] = baseline_total

Lib/test/test_profiling/test_sampling_profiler/test_collectors.py

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1748,6 +1748,123 @@ def test_diff_flamegraph_function_matched_despite_line_change(self):
17481748
self.assertAlmostEqual(child["diff"], 0.0, places=1)
17491749
self.assertAlmostEqual(child["diff_pct"], 0.0, places=1)
17501750

1751+
def test_diff_flamegraph_does_not_duplicate_line_values(self):
1752+
"""Function aggregates are apportioned across line nodes."""
1753+
def sample(line):
1754+
return [
1755+
MockInterpreterInfo(0, [
1756+
MockThreadInfo(1, [
1757+
MockFrameInfo("file.py", line, "func"),
1758+
MockFrameInfo("file.py", 1, "caller"),
1759+
])
1760+
])
1761+
]
1762+
1763+
diff = make_diff_collector_with_mock_baseline(
1764+
[sample(10), sample(20)]
1765+
)
1766+
diff.collect(sample(10))
1767+
diff.collect(sample(20))
1768+
1769+
data = diff._convert_to_flamegraph_format()
1770+
children = data["children"]
1771+
self.assertEqual(sum(node["self"] for node in children), 2)
1772+
self.assertEqual(sum(node["self_time"] for node in children), 2)
1773+
self.assertEqual(sum(node["baseline"] for node in children), 2)
1774+
for node in children:
1775+
self.assertEqual(node["self"], 1)
1776+
self.assertEqual(node["self_time"], 1)
1777+
self.assertAlmostEqual(node["baseline"], 1.0)
1778+
self.assertAlmostEqual(node["diff"], 0.0)
1779+
1780+
def test_diff_flamegraph_line_totals_include_allocated_self(self):
1781+
"""A line's baseline self time cannot exceed its inclusive time."""
1782+
def sample(*frames):
1783+
return [
1784+
MockInterpreterInfo(0, [MockThreadInfo(1, list(frames))])
1785+
]
1786+
1787+
target_10 = MockFrameInfo("file.py", 10, "target")
1788+
target_20 = MockFrameInfo("file.py", 20, "target")
1789+
child = MockFrameInfo("file.py", 30, "child")
1790+
1791+
diff = make_diff_collector_with_mock_baseline(
1792+
[sample(target_10)] * 100
1793+
)
1794+
for _ in range(10):
1795+
diff.collect(sample(target_10))
1796+
for _ in range(90):
1797+
diff.collect(sample(child, target_20))
1798+
1799+
data = diff._convert_to_flamegraph_format()
1800+
nodes = data["children"]
1801+
self.assertEqual(sum(node["baseline"] for node in nodes), 100)
1802+
self.assertEqual(sum(node["baseline_total"] for node in nodes), 100)
1803+
for node in nodes:
1804+
self.assertGreaterEqual(node["baseline"], 0)
1805+
self.assertLessEqual(node["baseline"], node["baseline_total"])
1806+
1807+
def test_diff_flamegraph_does_not_duplicate_elided_line_values(self):
1808+
"""Elided metadata uses each rendered line node's samples."""
1809+
def sample(line, funcname="old_func"):
1810+
return [
1811+
MockInterpreterInfo(0, [
1812+
MockThreadInfo(1, [
1813+
MockFrameInfo("file.py", line, funcname),
1814+
MockFrameInfo("file.py", 1, "caller"),
1815+
])
1816+
])
1817+
]
1818+
1819+
diff = make_diff_collector_with_mock_baseline(
1820+
[sample(10), sample(20)]
1821+
)
1822+
diff.collect(sample(30, "new_func"))
1823+
1824+
data = diff._convert_to_flamegraph_format()
1825+
elided = data["stats"]["elided_flamegraph"]
1826+
children = elided["children"]
1827+
scale = data["stats"]["baseline_scale"]
1828+
self.assertEqual(sum(node["self"] for node in children), 2)
1829+
self.assertEqual(sum(node["baseline"] for node in children), 2 * scale)
1830+
for node in children:
1831+
self.assertEqual(node["self"], 1)
1832+
self.assertAlmostEqual(node["baseline"], scale)
1833+
self.assertAlmostEqual(node["diff"], -scale)
1834+
1835+
def test_diff_flamegraph_elided_ancestors_have_no_lost_self_time(self):
1836+
"""Matched ancestors only carry inclusive elided geometry."""
1837+
root = MockFrameInfo("file.py", 10, "root")
1838+
common = MockFrameInfo("file.py", 20, "common", opcode=100)
1839+
old = MockFrameInfo("file.py", 30, "old")
1840+
1841+
common_sample = [
1842+
MockInterpreterInfo(0, [MockThreadInfo(1, [common, root])])
1843+
]
1844+
old_sample = [
1845+
MockInterpreterInfo(0, [MockThreadInfo(1, [old, common, root])])
1846+
]
1847+
1848+
diff = make_diff_collector_with_mock_baseline(
1849+
[common_sample] * 3 + [old_sample]
1850+
)
1851+
diff.collect(common_sample)
1852+
1853+
data = diff._convert_to_flamegraph_format()
1854+
elided_root = data["stats"]["elided_flamegraph"]
1855+
common_node = elided_root["children"][0]
1856+
old_node = common_node["children"][0]
1857+
1858+
for ancestor in (elided_root, common_node):
1859+
self.assertEqual(ancestor["self"], 0)
1860+
self.assertEqual(ancestor["baseline"], 0)
1861+
self.assertNotIn("opcodes", ancestor)
1862+
self.assertLessEqual(
1863+
ancestor["baseline"], ancestor["baseline_total"]
1864+
)
1865+
self.assertEqual(old_node["self"], 1)
1866+
self.assertEqual(old_node["baseline"], old_node["baseline_total"])
1867+
17511868
def test_diff_flamegraph_empty_current(self):
17521869
"""Empty current profile still produces differential metadata and elided paths."""
17531870
baseline_frames = [
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Prevent differential flamegraphs from duplicating self time across line
2+
nodes for the same function.

0 commit comments

Comments
 (0)