Wasmtime GitHub notifications bot (Mar 16 2023 at 12:50): Wasmtime GitHub notifications bot (Mar 16 2023 at 12:50):bnjbvr opened PR #6030 from perf-map to main:
This adds support for a simpler way to profile with
perf, using perf maps files. Whilejitdumpsupport is pretty effective and complete when you have access to DWARF metadata, theperf-injectstep may take lots of time, and it is not optimized at all when modules contain lots of functions. (In our embedding, I've stoppedperf-injectafter it ran for 5 hours and didn't complete, while creating 175K.soobjects.) Simplistic support for perf maps enable basic profiling: caller/callee trees, top/bottom time analysis, flamegraphs etc., and works very nicely when the wasm modules have many functions.I've also tweaked the documentation and tried to make the feature available in the C API too.
Wasmtime GitHub notifications bot (Mar 16 2023 at 13:36):bnjbvr updated PR #6030 from perf-map to main.
Wasmtime GitHub notifications bot (Mar 16 2023 at 13:39):bnjbvr edited PR #6030 from perf-map to main:
This adds support for a simpler way to profile with
perf, using perf maps files. Whilejitdumpsupport is pretty effective and complete when you have access to DWARF metadata, theperf-injectstep may take lots of time, and it is not optimized at all when modules contain lots of functions. (In our embedding, I've stoppedperf-injectafter it ran for 5 hours and didn't complete, while creating 175K.soobjects.) Simplistic support for perf maps enable basic profiling: caller/callee trees, top/bottom time analysis, flamegraphs etc., and works very nicely when the wasm modules have many functions.I've also tweaked the documentation and tried to make the feature available in the C API too.
I haven't put this behind a feature toggle because this profiling agent does very little, so it's always compiled _when running Linux_. For consistency I could add one, if you prefer.
Wasmtime GitHub notifications bot (Mar 16 2023 at 14:49):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 16 2023 at 14:49):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 16 2023 at 14:49):alexcrichton created PR review comment:
Given the text-based format of this file I think this may need to do some more sanitization of the
namesince any utf-8 sequence is allowed in wasm modules. For example I suspect a function with a newline in the name might cause issues here?
Wasmtime GitHub notifications bot (Mar 16 2023 at 14:49):alexcrichton created PR review comment:
Given the text-based format of this file and the multiple writes done here, this might be a good case for using
BufWriterto write out all this data and then using aflush()to finish it when the whole module has been written.
Wasmtime GitHub notifications bot (Mar 16 2023 at 14:49):alexcrichton created PR review comment:
We may be getting somewhat close to the limit of "now it's probably better to have
--profile=jitdump" as opposed to a separate flag-per-strategy, but that's fine to defer to a different PR.
Wasmtime GitHub notifications bot (Mar 16 2023 at 17:25):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Mar 16 2023 at 17:25):bnjbvr created PR review comment:
Good point; I've sanitized by removing
\nand\r, not sure if there's any other control character I should be worried about?
Wasmtime GitHub notifications bot (Mar 16 2023 at 17:37):bnjbvr updated PR #6030 from perf-map to main.
Wasmtime GitHub notifications bot (Mar 16 2023 at 17:39):bnjbvr updated PR #6030 from perf-map to main.
Wasmtime GitHub notifications bot (Mar 16 2023 at 18:01):bnjbvr updated PR #6030 from perf-map to main.
Wasmtime GitHub notifications bot (Mar 16 2023 at 18:28):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 16 2023 at 18:28):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 16 2023 at 18:28):alexcrichton created PR review comment:
Ah I missed this earlier, but I think we'll want ot do something other than ignoring errors here ideally. If an error happens it should probably "close" the file and terminate all future writing to it I suspect? (along with perhaps a warning message printed?)
Wasmtime GitHub notifications bot (Mar 16 2023 at 18:28):alexcrichton created PR review comment:
I think this may want a flush at the end too?
Also with a
BufWriterI think you could pass that intomake_lineto avoid the intermediate string allocation (e.g. write directly to the buffer). Although not required of course, that's ok to defer to if it's ever actually an issue.
Wasmtime GitHub notifications bot (Mar 20 2023 at 14:05):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Mar 20 2023 at 14:06):bnjbvr created PR review comment:
Good point! I've added log messages and early-returns. Looks like a
BufWriterwill automatically flush in its Drop impl, so we can avoid doing it explicitly in most early-returns.
Wasmtime GitHub notifications bot (Mar 20 2023 at 14:15):bnjbvr updated PR #6030 from perf-map to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 14:16):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Mar 20 2023 at 14:16):bnjbvr created PR review comment:
Turns out
Fileis also aWriteimplementator, so could havemake_linetake&mut dyn Writeand use it here without having aBufWriter(since there's only one write in this function, it didn't seem worth having an extra buffer).
Wasmtime GitHub notifications bot (Mar 20 2023 at 14:31):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 20 2023 at 14:32):alexcrichton has enabled auto merge for PR #6030.
Wasmtime GitHub notifications bot (Mar 20 2023 at 15:11):bnjbvr updated PR #6030 from perf-map to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 15:50):bnjbvr updated PR #6030 from perf-map to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 15:58):bnjbvr has enabled auto merge for PR #6030.
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:05):bnjbvr merged PR #6030.
Last updated: Dec 23 2024 at 12:05 UTC