wasmtime / issue #5749 Support plugging external profiler... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #5749 Support plugging external profiler...

Wasmtime GitHub notifications bot (Feb 08 2023 at 18:05):

This is the code necessary on cg_clif's side for this:

struct MeasuremeProfiler(SelfProfilerRef);

struct TimingGuard {
    profiler: std::mem::ManuallyDrop<SelfProfilerRef>,
    inner: Option<rustc_data_structures::profiling::TimingGuard<'static>>,
}

impl Drop for TimingGuard {
    fn drop(&mut self) {
        self.inner.take();
        unsafe {
            std::mem::ManuallyDrop::drop(&mut self.profiler);
        }
    }
}

impl cranelift_codegen::timing::Profiler for MeasuremeProfiler {
    fn start_pass(
        &self,
        pass: cranelift_codegen::timing::Pass,
    ) -> Box<dyn std::any::Any> {
        let mut timing_guard = TimingGuard {
            profiler: std::mem::ManuallyDrop::new(self.0.clone()),
            inner: None,
        };
        timing_guard.inner = Some(
            unsafe {
                &*(&*timing_guard.profiler as &SelfProfilerRef
                    as *const SelfProfilerRef)
            }
            .generic_activity(pass.description()),
        );
        Box::new(timing_guard)
    }
}

cranelift_codegen::timing::set_thread_profiler(Box::new(MeasuremeProfiler(
    cx.profiler.clone(),
)));

Most of it deals with a lifetime issue on measureme's side.

Wasmtime GitHub notifications bot (Feb 08 2023 at 18:07):

bjorn3 commented on issue #5749:

Measureme also supports outputting chrome profiler profiles, but chrome's profiler seems to suffer from float rounding errors causing some issues.

Wasmtime GitHub notifications bot (Feb 08 2023 at 18:07):

bjorn3 edited a comment on issue #5749:

Measureme also supports outputting chrome profiler profiles, but chrome's profiler seems to suffer from float rounding errors causing some issues for very short passes.

Wasmtime GitHub notifications bot (Feb 08 2023 at 18:51):

jameysharp commented on issue #5749:

I've reviewed everything in this PR except the most important commit ("Introduce a Profiler trait"), which I want to take a little longer to think about. I think it's a good idea! I just want to dig into the details.

Everything else I think is easy to approve: it's a bunch of good cleanups.

If you'd like to split that one commit to a separate PR I'd be happy to merge the rest now and either I or somebody else will review the Profiler trait parts later. But it's also okay if you'd like to just wait for it to all be reviewed and merged at once.

I'm not familiar with the measureme profiler. Is that something we could use in other Cranelift drivers like Wasmtime as well, or is it a module in rustc? I'd love to get flamegraphs like your demo for other uses of Cranelift as well.

Wasmtime GitHub notifications bot (Feb 08 2023 at 19:24):

bjorn3 commented on issue #5749:

If you'd like to split that one commit to a separate PR I'd be happy to merge the rest now and either I or somebody else will review the Profiler trait parts later. But it's also okay if you'd like to just wait for it to all be reviewed and merged at once.

I'm fine with waiting until the entire PR is reviewed, but if you prefer a smaller PR I can split it out.

I'm not familiar with the measureme profiler. Is that something we could use in other Cranelift drivers like Wasmtime as well, or is it a module in rustc? I'd love to get flamegraphs like your demo for other uses of Cranelift as well.

See https://github.com/rust-lang/measureme/. Having a measureme based profiler in Cranelift would be possible, but the Profiler interface is still necessary as it wouldn't be possible for Cranelift to use the exact same version of measureme as rustc without #![feature(rustc_private)]. Measureme is designed to be as low overhead as possible (the rustc benchmark suite runs with it enabled by default to allow for more detailed investigation.) It supports both wall time based and on x86 instruction based profiling. It supports both interval and instant events and allows extra metadata (like the name of the function being processed or the size of a compiled artifact).

Wasmtime GitHub notifications bot (Mar 09 2023 at 21:21):

bjorn3 commented on issue #5749:

@jameysharp gentle ping

Wasmtime GitHub notifications bot (Mar 09 2023 at 22:26):

jameysharp commented on issue #5749:

Oh, I think you'll need to rebase before I can merge since the set of required CI jobs changed.

Wasmtime GitHub notifications bot (Mar 10 2023 at 12:41):

bjorn3 commented on issue #5749:

Rebased

If you feel like opening another PR to make that change I'd be happy to take it.

Sure

Wasmtime GitHub notifications bot (Apr 29 2023 at 11:37):

bjorn3 commented on issue #5749:

cg_clif side of this implemented in https://github.com/bjorn3/rustc_codegen_cranelift/commit/ef07e8e60f994ec014d049a95591426fb92ebb79

Last updated: Dec 23 2024 at 12:05 UTC