Skip to content

Conversation

@bnjbvr
Copy link
Contributor

@bnjbvr bnjbvr commented Jan 25, 2023

The documentation of the iJIT_METHOD_LOAD event states that when calling into notify_event with that event, the return type is undefined, so we can't really tell whether that worked or not. So far, the library assumed that this would return 1, which is only the case when the event type is SHUTDOWN. This PR fixes that.

Also, avoid a double shutdown. I don't know if this prevents actual issues, but better be safe than sorry.

@bnjbvr bnjbvr force-pushed the notify-event-error-checking branch from 3baa873 to 42480ff Compare January 25, 2023 15:07
@abrown
Copy link
Contributor

abrown commented Jan 25, 2023

I tested this out today with Wasmtime in a variety of settings:

  • with an empty.wat but no VTune present, a new error appears--I think this is related to improper shutdown, which does specify that its return code must be 1:

    $ target/debug/wasmtime --vtune /tmp/empty.wat 
     ERROR ittapi::jit > Error when shutting down VTune: error when notifying event with tag 2: return code = 0
    $ echo $?
    0

    Maybe the problem here is that, when no VTune is present, we should be catching this shutdown error and informing the user that the --vtune flag expects the command to be run inside VTune?

  • with an empty.wat and VTune present, the original error is gone but the data is corrupted:

$ rm -rf /tmp/vtune-results && RUST_LOG=trace vtune -collect hotspots -result-dir=/tmp/vtune-results target/debug/wasmtime --vtune /tmp/empty.wat 
vtune: Collection started. To stop the collection, either press CTRL-C or enter from another console window: vtune -r /tmp/vtune-results -command stop.
 DEBUG wasmtime_cache::worker > Cache worker thread started.
 DEBUG wasmtime_cache::worker > New nice value of worker thread: 3
 TRACE wasmtime_cache         > get_data() for path: /home/abrown/.cache/wasmtime/modules/wasmtime-36e5bdfd0ec207bbfb1da79725ac37c7cbc331d0-1674683513442/9dcctsgQJsVC0l449oxB9E5LEbaSA0qZyMJ43JVOGJc
 DEBUG wasmtime_jit::code_memory > ignoring section 
 DEBUG wasmtime_jit::code_memory > ignoring section .wasmtime.engine
 DEBUG wasmtime_jit::code_memory > ignoring section .symtab
 DEBUG wasmtime_jit::code_memory > ignoring section .strtab
 DEBUG wasmtime_jit::code_memory > ignoring section .shstrtab
 TRACE wasmtime_cache::worker    > handle_on_cache_get() for path: /home/abrown/.cache/wasmtime/modules/wasmtime-36e5bdfd0ec207bbfb1da79725ac37c7cbc331d0-1674683513442/9dcctsgQJsVC0l449oxB9E5LEbaSA0qZyMJ43JVOGJc
 TRACE ittapi::jit               > notify_event: tag=2
vtune: Collection stopped.
vtune: Using result path `/tmp/vtune-results'
vtune: Executing actions 12 % Loading '85217-85229.0.trace' file               
vtune: Error: Cannot load data file `/tmp/vtune-results/data.0/85217-85229.0.trace' (Data file is corrupted).

This "data file is corrupted" might be a separate error; not sure at this point. (Also, shouldn't I be seeing a notify_event for any compiled code? Maybe there is none to report with (module)).

@abrown
Copy link
Contributor

abrown commented Jan 25, 2023

(Also, shouldn't I be seeing a notify_event for any compiled code? Maybe there is none to report with (module)).

Yeah, never mind: with modules that have functions I do see tag 16 events. Data file is still corrupted, though.

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Jan 26, 2023

Sorry, I've got no ideas about the corrupted report, I've never seen that before.

@abrown
Copy link
Contributor

abrown commented Feb 2, 2023

@jlb6740, did you want to review this? If not, I think we should merge it since it resolves some of the problems we were seeing.

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Feb 16, 2023

@abrown @jlb6740 Gentle review ping, please?

@abrown
Copy link
Contributor

abrown commented Feb 16, 2023

Yeah, sorry for the delay; I think this is the right thing to do even if all the issues aren't completely sorted out.

@abrown abrown merged commit 2de8a23 into intel:master Feb 16, 2023
@bnjbvr bnjbvr deleted the notify-event-error-checking branch February 17, 2023 15:27
@bnjbvr
Copy link
Contributor Author

bnjbvr commented Feb 17, 2023

Thanks @abrown! I'll look into profiling with vtune into our wasmtime embedding soonish, so I'll let you know if I run into other errors.

@abrown
Copy link
Contributor

abrown commented Feb 17, 2023

Thanks for this fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants