Adds perf jitdump support #360

jlb6740 · 2019-09-23T08:10:35Z

Patch adds support for the perf jitdump file specification.
With this patch it should be possible to see profile data for code
generated and maped at runtime. Specifically the patch adds support
for the JIT_CODE_LOAD and the JIT_DEBUG_INFO record as described in
the specification. Dumping jitfiles is enabled with the --jitdump
flag. When the -g flag is also used there is an attempt to dump file
and line number information where this option would be most useful
when the WASM file already includes DWARF debug information.

wasmtime-runtime/src/jit_dump.rs

bjorn3 · 2019-09-24T08:42:06Z

Shouls this be put in a new crate independent of wasmtime?

jlb6740 · 2019-09-24T18:41:54Z

Shouls this be put in a new crate independent of wasmtime?

@bjorn3 @yurydelendik @sunfishcode
We can definitely do that .. if that's preferred. In fact, I wanted to provide similar support for Vtune and was hoping to follow a similar implementation structure based on the feedback for this patch .. perhaps some general crate like wasmtime-perf-tools or are you thinking something else? At the same time, I would personally like to get this patch in it's current structure and iterate on suggestions like refactoring if that is reasonable and possible. Also note ... support for other record types are needed in the future as well so more patches are coming. So we can update the crate now, or in the future or not at all, whichever is preferred I am definitely OK with.

jlb6740 · 2019-09-25T18:40:33Z

@bjorn3 Thanks for all the suggestions, they are all very helpful. Depending on how folks would like to structure this (the comment about where to put jit_dump.rs), this current iteration may be ready. Let me know your thoughts on whether you'd like to move this file and/or see it as a separate crate.

bjorn3 · 2019-09-25T18:57:34Z

Let me know your thoughts on whether you'd like to move this file and/or see it as a separate crate.

I am fine with both. Don't know how others think about it.

yurydelendik · 2019-09-25T22:12:48Z

Let me know your thoughts on whether you'd like to move this file and/or see it as a separate crate.

I am fine with both. Don't know how others think about it.

I personally don't see an urgency in creating a crate for one file. If it a question about viewing it as an optional "feature", maybe it a good idea to use rust features for that.

jlb6740 · 2019-09-26T00:25:20Z

@bjorn3 @yurydelendik @sunfishcode FYI .. the last push rebased against recent updates to wasmtime and also removed unneeded dependencies in the cargo.toml. Let me know if there is anything else.

jlb6740 · 2019-09-26T19:32:32Z

@bjorn3 @yurydelendik @sunfishcode Hi ... I do not make any changes to lightbeam, but formatting checks fail for code under lightbeam causing not all checks to pass. How should this be handled? https://dev.azure.com/CraneStation/Wasmtime/_build/results?buildId=1102

bjorn3 · 2019-09-26T19:37:42Z

CraneStation/lightbeam#28

sunfishcode

Thanks for trimming the depenencies :-).

wasmtime-runtime/Cargo.toml

wasmtime-runtime/src/lib.rs

jlb6740 · 2019-10-28T23:38:20Z

Rebased to fix conflicts with the latest changes.

jlb6740 · 2020-02-12T21:37:33Z

@yurydelendik @alexcrichton Thanks for the comments. This latest push should have improvements to the previous. Now we use a generic interface for accessing the profiler API. In addition there has been some other cleanup w.r.t. how the profiler is created and stored for thread safety. Hopefully the code here is cleaner and easier to expand upon. Please let me know if you have more suggestions.

yurydelendik

Really good. I did not look into JitDumpAgent logic in details yet, but I would like to address the Clone workaround involving Arc<Mutex<Box<dyn..>>> (?) to reduce complexity there first. Thanks

Cargo.toml

crates/jit/src/instantiate.rs

yurydelendik

Looks really good.

I would like to see better "defined" error handling story before landing:

ignore all errors at module_load and just skip the invalid data and unsupported OS
- print some messages in the console?
return errors back to wasmtime-jit
- propagate it as instantiation error
- ignore it there (and print messages)

crates/jit/src/instantiate.rs

crates/profiling/src/jitdump.rs

Patch adds support for the perf jitdump file specification. With this patch it should be possible to see profile data for code generated and maped at runtime. Specifically the patch adds support for the JIT_CODE_LOAD and the JIT_DEBUG_INFO record as described in the specification. Dumping jitfiles is enabled with the --jitdump flag. When the -g flag is also used there is an attempt to dump file and line number information where this option would be most useful when the WASM file already includes DWARF debug information. The generation of the jitdump files has been tested on only a few wasm files. This patch is expected to be useful/serviceable where currently there is no means for jit profiling, but future patches may benefit line mapping and add support for additional jitdump record types. Usage Example: Record sudo perf record -k 1 -e instructions:u target/debug/wasmtime -g --jitdump test.wasm Combine sudo perf inject -v -j -i perf.data -o perf.jit.data Report sudo perf report -i perf.jit.data -F+period,srcline

jlb6740 · 2020-02-20T19:49:27Z

Looks really good.

I would like to see better "defined" error handling story before landing:

ignore all errors at module_load and just skip the invalid data and unsupported OS

print some messages in the console?

return errors back to wasmtime-jit

propagate it as instantiation error

ignore it there (and print messages)

I agree. For now took option 1, made some changes to the error handling .. encapsulated the two main kind of errors in a jitdumperror and propagate everything up to methods in jitdump that print some messages. There are two cases where we call unwrap() .. one is when we are access an option for the jitdump file to exist. This is already guarded at the entry interface function "module_load" so I suspect this should be ok. The other time is when parsing the debug_image in the call dump_from_debug_image.

Hopefully these are improvements in simplicity and clarity but it may could use some modifications. Let me know.

Thanks.

yurydelendik

I mentioned couple of issues below that might simplify reading of the code. There are some problem in reading of DWARF data, but I not planing fixing that at this moment. We are going to address that as we iterate in the future PRs.

yurydelendik · 2020-02-20T22:32:16Z

crates/profiling/src/jitdump.rs

+
+        if let Some(img) = &dbg_image {
+            if let Err(err) = self.dump_from_debug_image(img, module_name, addr, len, pid, tid) {
+                println!(


let's make it eprintln!() , here and elsewhere in this file

yurydelendik · 2020-02-20T22:36:29Z

crates/profiling/src/jitdump.rs

+
+                    let mut attrs = entry.attrs();
+                    while let Some(attr) = attrs.next()? {
+                        if let Some(n) = attr.name().static_string() {


No need to convert it to string: attr.name() == gimli::constants::DW_AT_low_pc

The match construct might be a better choice here match attr.name() { gimli::constants::DW_AT_low_pc => ....

yurydelendik · 2020-02-20T22:37:30Z

crates/profiling/src/jitdump.rs

+                                        if let Ok(s) = dwarf.debug_str.get_str(offset) {
+                                            clr_name.push_str("::");
+                                            clr_name.push_str(&s.to_string_lossy()?);
+                                            clr_name


the clr_name = above is not need, then this if-return can be removed.

yurydelendik · 2020-02-20T22:38:15Z

crates/profiling/src/jitdump.rs

+                        continue;
+                    }
+                    //  TODO: Temp check to make sure well only formed data is processed.
+                    if clr_name == "?" {


mostly false (unless "?" is a module_name)

it might be a better choice to make clr_name and func_name as Option<String> and set them valid e.g. Some(format!("{}::{}", module_name, s.to_string_lossy()?)) when needed.

yurydelendik

I'll land this PR since it is mostly a base for profiling support. There is basic support for jitdump present here, but needs improvements (see comments above). The further work can be done in different PRs.

Thank you for the patch.

bjorn3 suggested changes Sep 23, 2019

View reviewed changes

wasmtime-runtime/src/jit_dump.rs Outdated Show resolved Hide resolved

wasmtime-runtime/src/jit_dump.rs Outdated Show resolved Hide resolved

wasmtime-runtime/src/jit_dump.rs Outdated Show resolved Hide resolved

wasmtime-runtime/src/jit_dump.rs Outdated Show resolved Hide resolved

jlb6740 force-pushed the add-jit-dump-support branch from 3bb0a62 to 94001b3 Compare September 24, 2019 08:21

bjorn3 reviewed Sep 24, 2019

View reviewed changes

jlb6740 force-pushed the add-jit-dump-support branch 8 times, most recently from 9314319 to 676235a Compare September 25, 2019 01:30

jlb6740 force-pushed the add-jit-dump-support branch 2 times, most recently from e738faa to 8df39da Compare September 26, 2019 00:22

jlb6740 force-pushed the add-jit-dump-support branch from 8df39da to 74e4be0 Compare September 26, 2019 17:50

jlb6740 force-pushed the add-jit-dump-support branch from 74e4be0 to 228fbb3 Compare September 26, 2019 20:45

sunfishcode reviewed Sep 26, 2019

View reviewed changes

wasmtime-runtime/Cargo.toml Outdated Show resolved Hide resolved

wasmtime-runtime/src/lib.rs Outdated Show resolved Hide resolved

jlb6740 force-pushed the add-jit-dump-support branch from 228fbb3 to 16dac20 Compare September 26, 2019 23:44

jlb6740 force-pushed the add-jit-dump-support branch from 16dac20 to c1ea03e Compare October 28, 2019 23:37

jlb6740 force-pushed the add-jit-dump-support branch from c1ea03e to 832220c Compare October 28, 2019 23:50

jlb6740 force-pushed the add-jit-dump-support branch 2 times, most recently from 97e3549 to 8565188 Compare November 6, 2019 19:53

jlb6740 force-pushed the add-jit-dump-support branch 6 times, most recently from 6e508c8 to 32bae77 Compare February 12, 2020 21:23

jlb6740 force-pushed the add-jit-dump-support branch 6 times, most recently from 43a04d4 to 4441788 Compare February 12, 2020 22:00

yurydelendik reviewed Feb 17, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

crates/jit/src/instantiate.rs Outdated Show resolved Hide resolved

jlb6740 force-pushed the add-jit-dump-support branch from 4441788 to 478a2a5 Compare February 18, 2020 22:54

yurydelendik reviewed Feb 19, 2020

View reviewed changes

crates/jit/src/instantiate.rs Outdated Show resolved Hide resolved

crates/jit/src/instantiate.rs Outdated Show resolved Hide resolved

crates/profiling/src/jitdump.rs Outdated Show resolved Hide resolved

jlb6740 closed this Feb 20, 2020

jlb6740 reopened this Feb 20, 2020

jlb6740 force-pushed the add-jit-dump-support branch from 478a2a5 to 81e58ec Compare February 20, 2020 04:15

jlb6740 force-pushed the add-jit-dump-support branch 4 times, most recently from 97c00c2 to b070011 Compare February 20, 2020 19:36

yurydelendik reviewed Feb 20, 2020

View reviewed changes

yurydelendik approved these changes Feb 21, 2020

View reviewed changes

yurydelendik merged commit 9c6150b into bytecodealliance:master Feb 21, 2020

jlb6740 mentioned this pull request Feb 27, 2020

Jit profiling support broken when instances created with malloc #1017

Closed

jlb6740 deleted the add-jit-dump-support branch May 5, 2020 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds perf jitdump support #360

Adds perf jitdump support #360

jlb6740 commented Sep 23, 2019

bjorn3 commented Sep 24, 2019

jlb6740 commented Sep 24, 2019

jlb6740 commented Sep 25, 2019

bjorn3 commented Sep 25, 2019

yurydelendik commented Sep 25, 2019

jlb6740 commented Sep 26, 2019

jlb6740 commented Sep 26, 2019

bjorn3 commented Sep 26, 2019

sunfishcode left a comment

jlb6740 commented Oct 28, 2019

jlb6740 commented Feb 12, 2020

yurydelendik left a comment

yurydelendik left a comment

jlb6740 commented Feb 20, 2020

yurydelendik left a comment

yurydelendik Feb 20, 2020

yurydelendik Feb 20, 2020

yurydelendik Feb 20, 2020

yurydelendik Feb 20, 2020

yurydelendik left a comment •

edited

Loading

Adds perf jitdump support #360

Adds perf jitdump support #360

Conversation

jlb6740 commented Sep 23, 2019

bjorn3 commented Sep 24, 2019

jlb6740 commented Sep 24, 2019

jlb6740 commented Sep 25, 2019

bjorn3 commented Sep 25, 2019

yurydelendik commented Sep 25, 2019

jlb6740 commented Sep 26, 2019

jlb6740 commented Sep 26, 2019

bjorn3 commented Sep 26, 2019

sunfishcode left a comment

Choose a reason for hiding this comment

jlb6740 commented Oct 28, 2019

jlb6740 commented Feb 12, 2020

yurydelendik left a comment

Choose a reason for hiding this comment

yurydelendik left a comment

Choose a reason for hiding this comment

jlb6740 commented Feb 20, 2020

yurydelendik left a comment

Choose a reason for hiding this comment

yurydelendik Feb 20, 2020

Choose a reason for hiding this comment

yurydelendik Feb 20, 2020

Choose a reason for hiding this comment

yurydelendik Feb 20, 2020

Choose a reason for hiding this comment

yurydelendik Feb 20, 2020

Choose a reason for hiding this comment

yurydelendik left a comment • edited Loading

Choose a reason for hiding this comment

yurydelendik left a comment •

edited

Loading