gh-135953: Add flamegraph reporter to sampling profiler #138304
Conversation
lkollar
force-pushed
the
profiling/flamegraph
branch
from
689b05c to
0803b28
Compare
There was a problem hiding this comment.
Brief review of the JS code. Please disregard any feedback that is irrelevant due to use of the D3 framework -- I am primarily used to working with vanilla/framework-less JS.
On formatting, obviously PEP 8 doesn't apply as it's not Python code, but it would still be good to keep to the spirit of it, and wrap at 79 characters or thereabouts for readability.
I've had advice/reviews in the past that from a security perspective it's best to avoid constructing HTML in JS directly and instead use e.g. document.createElement(). I don't know enough about the D3/JS interactions to know if this applies here, though. There was one place that looked like it was performing HTML escaping in JavaScript, which seems like something we might consider doing in the Python layer before the data gets to JS?
A
There was a problem hiding this comment.
Potentially use the SVG instead? https://github.com/python/devguide/blob/main/_static/python-logo.svg?short_path=f69d24a
The SVG is ~18x smaller, but it hasn't been minified.
There was a problem hiding this comment.
Perhaps add .min.js to the list of generated files in .gitattributes?
| .style("position", "absolute") | ||
| .style("padding", "20px") | ||
| .style("background", "white") | ||
| .style("color", "#2e3338") | ||
| .style("border-radius", "8px") | ||
| .style("font-size", "14px") | ||
| .style("border", "1px solid #e9ecef") | ||
| .style("box-shadow", "0 8px 30px rgba(0, 0, 0, 0.15)") | ||
| .style("z-index", "1000") | ||
| .style("pointer-events", "none") | ||
| .style("font-weight", "400") | ||
| .style("line-height", "1.5") | ||
| .style("max-width", "500px") | ||
| .style("font-family", "'Source Sans Pro', sans-serif") | ||
| .style("opacity", 0); |
There was a problem hiding this comment.
Is it possible to apply these styles via a class, instead of inline?
Lib/profiling/sampling/flamegraph.js
Outdated
| } | ||
|
|
||
| const pythonTooltip = flamegraph.tooltip.defaultFlamegraphTooltip(); | ||
|
|
Lib/profiling/sampling/flamegraph.js
Outdated
| const sourceLines = source | ||
| .map( | ||
| (line) => | ||
| `<div style="font-family: 'SF Mono', 'Monaco', 'Consolas', monospace; font-size: 12px; color: ${line.startsWith("→") ? "#3776ab" : "#5a6c7d"}; white-space: pre; line-height: 1.4; padding: 2px 0;">${line.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">")}</div>`, |
There was a problem hiding this comment.
Likewise, is it possible to use a class instead of inline styles?
Should we escape the HTML (line.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">")) whilst we're still in Python?
For the color codes, could we use CSS variables so that if we change them in the future, we only need to change it in one place?
Lib/profiling/sampling/flamegraph.js
Outdated
| ${ | ||
| calls > 0 | ||
| ? ` | ||
| <span style="color: #5a6c7d; font-weight: 500;">Function Calls:</span> | ||
| <strong style="color: #2e3338;">${calls.toLocaleString()}</strong> | ||
| ` | ||
| : "" | ||
| } |
There was a problem hiding this comment.
Perhaps extract to local variables instead of nested substitutions?
Lib/profiling/sampling/flamegraph.js
Outdated
| } | ||
|
|
||
| // Wait for libraries to load | ||
| document.addEventListener("DOMContentLoaded", function () { |
There was a problem hiding this comment.
My understanding is that DOMContentLoaded may fire before the script loads, so the suggested pattern is to check for document.readyState. I don't know if this applies here or not, though.
Lib/profiling/sampling/flamegraph.js
Outdated
| // Add search functionality - wait for chart to be ready | ||
| if (searchInput) { |
There was a problem hiding this comment.
Could this block be extracted to a helper function?
| def _create_flamegraph_html(self, data): | ||
| data_json = json.dumps(data) | ||
|
|
||
| template_dir = pathlib.Path(__file__).parent |
There was a problem hiding this comment.
Should we use importlib.resources here? I believe e.g. for the Windows embedable builds, the stdlib is distributed as a zip file so modules might not have a __file__ attribute.
Since this is a static template there are no advantages of doing this and having the web parts in Python is a bit messy specially since we cannot use something like Jinja. Having all the web parts in JS or the template itself is a common practice for this case and keeps the Python code focused just on the data part and not in creating HTML. Also since this is just a static file served with no dependencies so there are no concerns aboutsecurity or injection so I prefer to maximize maintainability. I will give it a pass with prettier to format it as you indicate :) |
|
Thanks for the review @AA-Turner. I think I addressed everything except for the comments around constructing HTML in JS. I also ended up breaking up the JS functions a bit in 7c4d792 as it was getting really unwieldy and I think this will make maintenance easier. |
lkollar
requested review from
AlexWaygood,
FFY00,
JelleZijlstra,
ambv,
barneygale,
berkerpeksag,
ethanfurman,
giampaolo,
gpshead,
jaraco,
lysnikolaou,
rhettinger,
terryjreedy,
tomasr8,
vsajip and
warsaw
as code owners
pablogsal
removed request for
a team,
1st1,
Fidget-Spinner,
ZeroIntensity,
abalkin,
asvetlov,
brandtbucher,
brettcannon,
corona10,
diegorusso,
encukou,
ericsnowcurrently,
ericvsmith,
ezio-melotti,
freakboy3742,
hugovk,
iritkatriel,
kumaraditya303,
markshannon,
mhsmith,
ncoghlan,
pganssle,
picnixz,
savannahostrowski,
serhiy-storchaka,
terryjreedy,
tiran,
tomasr8 and
willingc
4 participants
Add
--flamegraphoption to the sampling profiler to produce a self-contained HTML report.