Trace segments alpha + inlined frames by Svetlitski · Pull Request #353 · janestreet/magic-trace

Svetlitski · 2026-02-09T21:18:48Z

Introduce new, experimental trace-writing backend

The existing Trace_writer module works well enough (albeit not perfectly) most of the time. However, it is difficult to reason about, in large part because it writes the trace in a streaming fashion. That introduces significant additional complexity and bookkeeping, and limits the ability of the trace-writer to make use of information discovered later in the trace (I believe the latter is why traces produced today often have the few frames closest to the root wrong). Because we want to extend the trace-writer with new functionality, we're starting fresh with a different design that's easier to reason about. The new implementation currently exists alongside the original, but the goal is to eventually replace it entirely.

Instead of writing the trace in a streaming fashion, we construct an internal representation of the trace in memory, and write out the trace in a separate, final pass once all of the events have been consumed. The module responsible for doing most of the heavy lifting is the new Trace_segment, which represents a continuous, lossless, and error-free segment of the trace; we create a new trace-segment whenever we encounter an error.

The other major addition is that the new implementation includes inlined function calls, using LLVM for symbolization, dramatically increasing the fidelity of the trace.

This PR is effectively an alpha of the new implementation. The code here does indeed work,
and produces better traces than the existing backend in many cases, but there are a couple
critical issues:

Error recovery: We create a new trace-segment whenever we encounter an error,
but at present we naively treat each trace-segment as disjoint. We need to add an additional
"stitching" pass before the trace is written out, making a heuristic, best-effort attempt to
join together adjacent trace-segments in a way that preserves control-flow continuity.
All of this is a long way of saying that if you encounter any error while using the new
implementation, your trace is likely to be horribly broken.
Performance: Including inlined frames makes the traces significantly larger, and the supporting
code for this new functionality is written pretty naively from a performance standpoint. As a result,
the new implementation is roughly 2x slower than the old implementation.

It should also go without saying that while this code appears to work well on the traces I've tried it on, I would not at all be surprised if there are still bugs/edge-cases.

We will address these shortcomings over time, but in the meantime the new implementation is opt-in; setting the environment variable MAGIC_TRACE_USE_NEW_TRACE_WRITER=1 will enable it.

Xyene · 2026-02-23T04:48:12Z

+    thread. *)
+type t
+
+val create : Ocaml_exception_info.t option -> in_filtered_region:bool -> t


Nit: ~ocaml_exception_info

Even once it's the only argument (as I'm getting rid of in_filtered_region in favor of externalizing that in response to your other review comments)? I'm happy either way, just clarifying.

Svetlitski

Note that this review does not cover Trace_segment; I'll take a final pass over that module on Monday.

Svetlitski · 2026-04-24T21:43:28Z

+
+let equal = phys_equal
+let compare t1 t2 = Int.compare (Obj.magic t1) (Obj.magic t2)
+let hash t = Int.hash (Obj.magic t)


This is problematic because the hash will change if the string is relocated, either via promotion to the major heap or during compaction.

Svetlitski · 2026-04-24T22:19:30Z

@@ -0,0 +1,6 @@
+open! Core


Given that this module is only used to intern DSO/executable paths, I think it makes more sense to replace this with an equivalent Build_id module, where instead of Interned_string.intern you have Build_id.resolve which gets the Build_id corresponding to that path, resolved via LLVM.

Svetlitski · 2026-04-24T22:23:28Z

+module Response : sig
+  type t [@@deriving sexp_of]
+
+  val physical_frame : t -> Info.t


Yes, this isn't used at present. The idea is it will be useful when we completely replace Owee with LLVM.

Svetlitski · 2026-04-24T22:28:41Z

@@ -1 +1 @@
-(lang dune 2.0)
+(lang dune 3.0)


I needed to change this in order to get access to the link_flags environment setting in the top-level dune, which we need for our statically linked build to work. This came with the unfortunate side-effect of changing dune's auto-formatting of its own files, hence why most of the project's dune files are modified in this PR.

Svetlitski

Took another pass at Trace_segment

Svetlitski · 2026-05-13T19:42:18Z

+end
+
+module Control_flow = struct
+  type t =


Making this private and gating construction behind val call : int -> t and val return : int -> t functions that internally return values from a statically allocated array when 0 <= {depth,distance} <= 15 would be an easy way to eliminate most allocations for this type.

This PR is already gigantic, so I'll punt this to the follow-up with all of the other performance improvements.

Svetlitski · 2026-05-13T20:02:10Z

+      let dst_frame = Frame.create dst ~parent:(t.root :> Frame.t) ~kind:Physical in
+      append_inlined_frames t time ~physical_frame:dst_frame ~physical_frame_is_new:true
+    | This parent ->
+      (* This is probably a non-recursive tail-call. *)


It's outside the scope of this PR, but in the future it would be nice to retain this information in the Callstack.t so that we can produce a flow event to make the control-flow more obvious in the UI.

Svetlitski · 2026-05-13T20:54:31Z

+           ~dso:t.last_known_location.dso
+           ~addr:t.last_known_location.instruction_pointer
+       in
+       (* TODO I fear symbolizing at every byte like this is likely to be *very* expensive, but let's focus on just getting things working for now. *)


According to Claude's reading of the LLVM source, it would not be hard to make a small patch to LLVM that surfaces the enclosing address range for a queried address, so we could avoid making N symbolization requests for N bytes of contiguous code that all have the same debuginfo. Given just how expensive this has turned out to be, this seems worthwhile.

I have a working prototype of this on another branch; it makes things appreciably faster.

kiwigitops · 2026-05-25T16:57:08Z

One thing to guard before the new writer is usable with events_writer: New_trace_writer.write_event_and_callstack currently serializes every event with an empty compressed callstack (new_symbols = []; callstack = []). That is a silent data-loss path for -z-events-output/binio output when MAGIC_TRACE_USE_NEW_TRACE_WRITER is enabled for Intel PT.

If callstack compression is not ready yet, it may be safer to reject event-output with the new writer or fall back to the old writer for that mode. Otherwise downstream consumers get a valid-looking events file with all callstack context stripped.

Signed-off-by: Kevin Svetlitski <ksvetlitski@janestreet.com>

Xyene

LGTM, thanks a lot for working on this!

Signed-off-by: Kevin Svetlitski <ksvetlitski@janestreet.com>

This is useful for testing. Signed-off-by: Kevin Svetlitski <ksvetlitski@janestreet.com>

This is in preparation for writing the new backend. After we've made substantial changes to `src/new_trace_writer.ml`, you'll still be able to track the history by running: ```bash git blame -C -C src/new_trace_writer.ml ``` Yes, you need to pass `-C` *twice*: > -C ... when this option is given twice, the command additionally looks for copies from other files in the commit that creates the file. Signed-off-by: Kevin Svetlitski <ksvetlitski@janestreet.com>

The existing `Trace_writer` module works well enough (albeit not perfectly) most of the time. However, it is difficult to reason about, in large part because it writes the trace in a streaming fashion. That introduces significant additional complexity and bookkeeping, and limits the ability of the trace-writer to make use of information discovered later in the trace (I believe the latter is why traces produced today often have the few frames closest to the root wrong). Because we want to extend the trace-writer with new functionality, we're starting fresh with a different design that's easier to reason about. The new implementation currently exists alongside the original, but the goal is to eventually replace it entirely. Instead of writing the trace in a streaming fashion, we construct an internal representation of the trace in memory, and write out the trace in a separate, final pass once all of the events have been consumed. The module responsible for doing most of the heavy lifting is the new `Trace_segment`, which represents a continuous, lossless, and error-free segment of the trace; we create a new trace-segment whenever we encounter an error. The other major addition is that the new implementation includes inlined function calls, using LLVM for symbolization, dramatically increasing the fidelity of the trace. **This PR is effectively an alpha of the new implementation.** The code here does indeed work, and produces better traces than the existing backend in many cases, but there are a couple critical issues: 1. **Error recovery**: We create a new trace-segment whenever we encounter an error, **but at present we naively treat each trace-segment as disjoint**. We need to add an additional "stitching" pass before the trace is written out, making a heuristic, best-effort attempt to join together adjacent trace-segments in a way that preserves control-flow continuity. All of this is a long way of saying that if you encounter *any* error while using the new implementation, your trace is likely to be horribly broken. 2. **Performance**: Including inlined frames makes the traces significantly larger, and the supporting code for this new functionality is written pretty naively from a performance standpoint. As a result, the new implementation is roughly 2x slower than the old implementation. It should also go without saying that while this code appears to work well on the traces I've tried it on, I would not at all be surprised if there are still bugs/edge-cases. We will address these shortcomings over time, but in the meantime the new implementation is opt-in; setting the environment variable `MAGIC_TRACE_USE_NEW_TRACE_WRITER=1` will enable it. Signed-off-by: Kevin Svetlitski <ksvetlitski@janestreet.com>

Svetlitski mentioned this pull request Feb 9, 2026

Trace segments #350

Closed

6 tasks

Svetlitski marked this pull request as ready for review February 9, 2026 21:26

Svetlitski requested a review from Xyene February 9, 2026 21:27

Xyene reviewed Mar 3, 2026

View reviewed changes

Comment thread src/trace_segment.ml Outdated

Comment thread src/trace_segment.ml Outdated

Comment thread src/trace_segment.ml Outdated

Comment thread src/trace_segment.ml Outdated

Comment thread src/trace_segment.ml Outdated

Comment thread src/trace_segment.ml

Xyene reviewed Mar 3, 2026

View reviewed changes

Comment thread src/trace_segment.ml Outdated

Comment thread src/trace_segment.ml Outdated

Comment thread src/trace_segment.ml Outdated

Comment thread src/new_trace_writer.ml Outdated

Svetlitski force-pushed the trace-segments-alpha branch 3 times, most recently from 6abeb17 to d1c106d Compare April 16, 2026 15:49

Svetlitski marked this pull request as draft April 16, 2026 15:50

Svetlitski force-pushed the trace-segments-alpha branch 2 times, most recently from 8ca37d7 to b689daf Compare April 22, 2026 19:00

Svetlitski marked this pull request as ready for review April 22, 2026 19:01

Svetlitski force-pushed the trace-segments-alpha branch 2 times, most recently from 3f580d3 to babd49e Compare April 24, 2026 21:06

Svetlitski mentioned this pull request Apr 24, 2026

[Testing] Alpine-based GitHub Action #362

Closed

Svetlitski commented Apr 24, 2026

View reviewed changes

Svetlitski force-pushed the trace-segments-alpha branch from babd49e to b9112ac Compare April 27, 2026 14:46

Svetlitski marked this pull request as draft April 27, 2026 14:51

Svetlitski requested a review from Xyene April 27, 2026 14:51

TheNumbat mentioned this pull request May 6, 2026

OCaml stack switching #367

Closed

Svetlitski changed the title ~~Trace segments alpha~~ Trace segments alpha + inlined frames May 11, 2026

Svetlitski force-pushed the trace-segments-alpha branch from 69ed37d to 6d5cf09 Compare May 13, 2026 19:24

Svetlitski commented May 13, 2026

View reviewed changes

Svetlitski force-pushed the trace-segments-alpha branch from 4868404 to c2f98f5 Compare May 18, 2026 14:33

Xyene reviewed May 20, 2026

View reviewed changes

Svetlitski force-pushed the trace-segments-alpha branch 2 times, most recently from 3545c82 to 75afa99 Compare May 22, 2026 21:09

Svetlitski mentioned this pull request May 25, 2026

Add C++ symbol demangling via __cxa_demangle #364

Closed

Svetlitski force-pushed the trace-segments-alpha branch 2 times, most recently from c69c09c to db94b3c Compare May 25, 2026 16:22

Add trace file formats to .gitignore

f534f13

Signed-off-by: Kevin Svetlitski <ksvetlitski@janestreet.com>

Svetlitski force-pushed the trace-segments-alpha branch 2 times, most recently from 4b78e6b to 24e7fa5 Compare May 25, 2026 18:39