Filter trace to range #222

pbaumbacher · 2022-06-13T18:13:41Z

Adds a new flag -filter "(<start> <stop>)" to magic trace. With this flag, the trace is trimmed to only include events that occur between start (inclusive) and stop (exclusive) (#81). If either symbol doesn't appear in the trace, no output is produced. In multi-snapshot mode, the trace only includes regions between consecutive start and stop calls. The -filter flag is compatible with the same modes as the -trigger flag.

When producing a .sexp output, only events that occur in the filtered region are emitted. When producing a fuchsia .fxt output, the full available callstack around the filtered region is reconstructed. Active stack frames at the beginning of the filtered region have the time start was called as their inferred start time.

Xyene

Good work! Could you also please interactive rebase over current master, and squash commits into logical groupings? Seems like a few commits unrelated to your work snuck in.

Xyene · 2022-06-15T14:59:31Z

core/trace_filter.ml

+open! Core
+
+module Range = struct
+  type t = string * string [@@deriving sexp]


Can we make this a record? Accepting it as a (start stop) sexp and then manually expanding it into named record fields in the arg type parser sounds reasonable.

Actually, if we do this, we don't need the extra Range module, and could call of_command_string for both left/right of the tuple here.

I think we still need two modules to express the difference between a the filter we get from the command line (range of Symbol_selection.ts) and a filter that's been evaluated (turned into strings) by fzf.

Turned it into a record and changed things around a bit so [Unevaluated] is the inner module. I think it reads a bit better, but let me know if I'm missing something obvious and can get rid of the extra module altogether.

I guess what I'm unclear on is why not have:

module Symbol_range = struct type t = { start_symbol : Symbol_selection.t ; stop_symbol : Symbol_selection.t } (* Accepts '(foo bar)', returns { start_symbol = User_selected "foo"; stop_symbol = User_selected = "bar" } *) let arg_type = ... end

and not have the Option.map call.

(This lets you get rid of String_pair I think.)

Yeah that makes sense, wasn't thinking through what I was doing with the Option.map

core/trace_filter.mli

test/test.ml

Xyene · 2022-06-15T15:04:18Z

src/trace_writer.ml

+  rewrite_callstack t ~callstack:thread_info.callstack ~thread_info ~time
+;;
+
+let maybe_start_hot_section t ~should_write ~time =


maybe_start_trimmed_region?

filtered_region?

Xyene · 2022-06-15T15:04:41Z

src/trace_writer.ml

+    Hashtbl.iter t.thread_info ~f:(fun thread_info ->
+        flush t ~to_time:time thread_info;
+        Deque.clear thread_info.start_events);
+    t.in_hot_section <- true;


`in_trimmed_range?

Xyene · 2022-06-20T15:57:16Z

core/symbol.ml

@@ -9,6 +9,8 @@ type t =
  | Syscall
 [@@deriving sexp, compare]

+let equal t1 t2 = compare t1 t2 = 0


Does [@@deriving equal] work here?

No, because Int64.Hex (used by Perf_map_location) doesn't derive equal.

Xyene · 2022-06-20T16:02:57Z

core/trace_filter.ml

+open! Core
+
+module Range = struct
+  type t = string * string [@@deriving sexp]


I guess what I'm unclear on is why not have:

module Symbol_range = struct type t = { start_symbol : Symbol_selection.t ; stop_symbol : Symbol_selection.t } (* Accepts '(foo bar)', returns { start_symbol = User_selected "foo"; stop_symbol = User_selected = "bar" } *) let arg_type = ... end

and not have the Option.map call.

src/perf_tool_backend.ml

Xyene · 2022-06-20T16:18:00Z

core/trace_filter.mli

+  [@@deriving fields]
+end
+
+type t =


Is this type used?

It's returned by evaluate_trace_filter in trace.ml and gets passed around until the filter is actually applied.

src/for_range.ml

src/trace.ml

Xyene · 2022-06-24T13:27:19Z

src/for_range.ml

+            | Error _ | Ok (Power _) -> hits, in_filtered_region
+            | Ok (Trace ok) ->
+              (match
+                 ( Time_ns_unix.Span.(ok.time = hd.time)


How about something like

let hits, in_filtered_region = match hits with | [] -> hits, in_filtered_region | hd :: tl -> (match event with | Ok (Trace { kind = Some Call; time; dst; _ }) when Time_ns_unix.Span.(time = hd.time) && Symbol.equal dst.symbol hd.symbol -> tl, not in_filtered_region | _ -> hits, in_filtered_region) in

?

Changed to something close to this

Xyene

Otherwise LGTM. I think if you rebased/squashed we could be good to merge.

pbaumbacher · 2022-06-24T21:59:30Z

Otherwise LGTM. I think if you rebased/squashed we could be good to merge.

I had to update the range logic a bit to handle Sample events. The most straightforward way to do this was just to treat any stack containing a range symbol as a Symbol_hit, but a consequence is that in sampling mode the filtered region begins at the end of the range start function, instead of at the beginning in tracing mode.

Making the behavior identical would require a bit more work: the control flow would have to be fairly different depending on if we're receiving sampling events or perf events. I think using sampling with the filter isn't likely to be a super common use-case, but let me know what you think.

…put to include only events between consecutive occurences of <start> and <stop>. Signed-off-by: Peter Baumbacher <pbaumbacher@gmail.com>

Xyene

LGTM, thanks!

pbaumbacher force-pushed the range-trace branch from a9ca28d to d6b8dc0 Compare June 13, 2022 18:17

Xyene reviewed Jun 15, 2022

View reviewed changes

pbaumbacher force-pushed the range-trace branch from 78966b1 to 0d00fd3 Compare June 15, 2022 20:30

Xyene reviewed Jun 20, 2022

View reviewed changes

src/perf_tool_backend.ml Outdated Show resolved Hide resolved

Xyene reviewed Jun 20, 2022

View reviewed changes

src/for_range.ml Outdated Show resolved Hide resolved

Xyene reviewed Jun 20, 2022

View reviewed changes

src/trace.ml Outdated Show resolved Hide resolved

Xyene reviewed Jun 24, 2022

View reviewed changes

pbaumbacher force-pushed the range-trace branch 2 times, most recently from 1d7693e to d792775 Compare June 24, 2022 20:57

pbaumbacher force-pushed the range-trace branch from d792775 to 686796a Compare June 27, 2022 13:40

Add [-filter (start stop)] flag. This truncates the trace or sexp out…

8af827b

…put to include only events between consecutive occurences of <start> and <stop>. Signed-off-by: Peter Baumbacher <pbaumbacher@gmail.com>

pbaumbacher force-pushed the range-trace branch from 686796a to 8af827b Compare June 28, 2022 19:35

Xyene approved these changes Jun 29, 2022

View reviewed changes

Xyene merged commit ed4fb49 into janestreet:master Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter trace to range #222

Filter trace to range #222

pbaumbacher commented Jun 13, 2022

Xyene left a comment

Xyene Jun 15, 2022

Xyene Jun 15, 2022

pbaumbacher Jun 15, 2022

pbaumbacher Jun 15, 2022

Xyene Jun 20, 2022

Xyene Jun 20, 2022

pbaumbacher Jun 21, 2022

Xyene Jun 15, 2022

pbaumbacher Jun 15, 2022

Xyene Jun 15, 2022

Xyene Jun 20, 2022

pbaumbacher Jun 21, 2022

Xyene Jun 20, 2022

Xyene Jun 20, 2022

pbaumbacher Jun 21, 2022

Xyene Jun 24, 2022

pbaumbacher Jun 24, 2022

Xyene left a comment

pbaumbacher commented Jun 24, 2022

Xyene left a comment

Filter trace to range #222

Filter trace to range #222

Conversation

pbaumbacher commented Jun 13, 2022

Xyene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xyene left a comment

Choose a reason for hiding this comment

pbaumbacher commented Jun 24, 2022

Xyene left a comment

Choose a reason for hiding this comment