-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add use-trace-processor-shell flag to automate large trace processing #248
base: master
Are you sure you want to change the base?
Changes from all commits
86aebc1
1897453
39fc733
4a91831
679ad5d
681bf54
f567768
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,8 +8,11 @@ let supports_command command = | |||||
Lazy.from_fun (fun () -> | ||||||
match Core_unix.fork () with | ||||||
| `In_the_child -> | ||||||
Core_unix.close Core_unix.stdout; | ||||||
Core_unix.close Core_unix.stderr; | ||||||
(* gracefully hide perf outputs *) | ||||||
(* simply closing stdout and stderr irregularly caused this child process to exit with 1 and halted the overall process of things *) | ||||||
let devnull = Core_unix.openfile ~mode:[ O_WRONLY ] "/dev/null" in | ||||||
Core_unix.dup2 ~src:devnull ~dst:Core_unix.stdout (); | ||||||
Core_unix.dup2 ~src:devnull ~dst:Core_unix.stderr (); | ||||||
Core_unix.exec ~prog:command ~argv:[ command; "--version" ] ~use_path:true () | ||||||
|> never_returns | ||||||
| `In_the_parent pid -> | ||||||
|
@@ -561,6 +564,42 @@ module Make_commands (Backend : Backend_intf.S) = struct | |||||
{ Decode_opts.output_config; decode_opts; print_events } | ||||||
;; | ||||||
|
||||||
module Trace_processor_config = struct | ||||||
type t = | ||||||
| Disabled | ||||||
| Enabled of { trace_processor_shell_path : string } | ||||||
|
||||||
let param = | ||||||
match Env_vars.trace_processor_shell_path with | ||||||
| None -> Command.Param.return Disabled | ||||||
| Some processor_shell_path -> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Nit: avoid having multiple variable names for the semantically-same thing. It's best to shadow the previously-declared name in such cases. |
||||||
let%map_open.Command processor = | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Nit: descriptive variable names. |
||||||
flag | ||||||
"use-trace-processor-shell" | ||||||
no_arg | ||||||
~doc:[%string "use the trace processor set in environment variables"] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
in | ||||||
if processor | ||||||
then Enabled { trace_processor_shell_path = processor_shell_path } | ||||||
else Disabled | ||||||
;; | ||||||
end | ||||||
|
||||||
(* CR-someday abena: generalize call_trace_processor with the perf version *) | ||||||
(* Same as [Caml.exit] but does not run at_exit handlers *) | ||||||
external sys_exit : int -> 'a = "caml_sys_exit" | ||||||
|
||||||
let call_trace_processor ?env ~prog ~argv () = | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's either generalize this with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am unsure which part of this function is perf-specific. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None of it is, so ideally this'd be the same function as magic-trace/src/perf_tool_backend.ml Line 70 in d51d797
You don't need to do this in this PR, but it would be good to leave a note for the future. |
||||||
let pr_set_pdeathsig = Or_error.ok_exn Linux_ext.pr_set_pdeathsig in | ||||||
match Core_unix.fork () with | ||||||
| `In_the_child -> | ||||||
pr_set_pdeathsig Signal.kill; | ||||||
never_returns | ||||||
(try Core_unix.exec ?env ~prog ~argv () with | ||||||
| _ -> sys_exit 127) | ||||||
| `In_the_parent _ -> () | ||||||
;; | ||||||
|
||||||
let run_command = | ||||||
Command.async_or_error | ||||||
~summary:"Runs a command and traces it." | ||||||
|
@@ -573,11 +612,17 @@ module Make_commands (Backend : Backend_intf.S) = struct | |||||
magic-trace run -multi-thread ./program -- arg1 arg2\n\n\ | ||||||
# Run a process, tracing its entire execution (only practical for short-lived \ | ||||||
processes)\n\ | ||||||
magic-trace run -full-execution ./program\n") | ||||||
magic-trace run -full-execution ./program\n\ | ||||||
# Run a process that generates a large trace file, and automatically process \n\ | ||||||
\ the file locally (flag only exists when the environment variable \ | ||||||
called \n\ | ||||||
\ MAGIC_TRACE_TRACE_PROCESSOR_SHELL is set) magic-trace run ./program \ | ||||||
-use-trace-processor-shell\n") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should leave this out of the documentation for now, or maybe make the |
||||||
(let%map_open.Command record_opt_fn = record_flags | ||||||
and decode_opts = decode_flags | ||||||
and debug_print_perf_commands = debug_print_perf_commands | ||||||
and prog = anon ("COMMAND" %: string) | ||||||
and trace_processor_exe = Trace_processor_config.param | ||||||
and argv = | ||||||
flag "--" escape ~doc:"ARGS Arguments for the command. Ignored by magic-trace." | ||||||
in | ||||||
|
@@ -589,31 +634,53 @@ module Make_commands (Backend : Backend_intf.S) = struct | |||||
| Some path -> path | ||||||
| None -> failwithf "Can't find executable for %s" prog () | ||||||
in | ||||||
record_opt_fn ~executable ~f:(fun opts -> | ||||||
let elf = Elf.create opts.executable in | ||||||
let%bind range_symbols = | ||||||
evaluate_trace_filter ~trace_filter:opts.trace_filter ~elf | ||||||
in | ||||||
let%bind pid = | ||||||
let argv = prog :: List.concat (Option.to_list argv) in | ||||||
run_and_record | ||||||
opts | ||||||
let%bind () = | ||||||
record_opt_fn ~executable ~f:(fun opts -> | ||||||
let elf = Elf.create opts.executable in | ||||||
let%bind range_symbols = | ||||||
evaluate_trace_filter ~trace_filter:opts.trace_filter ~elf | ||||||
in | ||||||
let%bind pid = | ||||||
let argv = prog :: List.concat (Option.to_list argv) in | ||||||
run_and_record | ||||||
opts | ||||||
~elf | ||||||
~debug_print_perf_commands | ||||||
~prog | ||||||
~argv | ||||||
~collection_mode:opts.collection_mode | ||||||
in | ||||||
let%bind.Deferred perf_maps = Perf_map.Table.load_by_pids [ pid ] in | ||||||
decode_to_trace | ||||||
~perf_maps | ||||||
?range_symbols | ||||||
~elf | ||||||
~trace_scope:opts.trace_scope | ||||||
~debug_print_perf_commands | ||||||
~prog | ||||||
~argv | ||||||
~record_dir:opts.record_dir | ||||||
~collection_mode:opts.collection_mode | ||||||
in | ||||||
let%bind.Deferred perf_maps = Perf_map.Table.load_by_pids [ pid ] in | ||||||
decode_to_trace | ||||||
~perf_maps | ||||||
?range_symbols | ||||||
~elf | ||||||
~trace_scope:opts.trace_scope | ||||||
~debug_print_perf_commands | ||||||
~record_dir:opts.record_dir | ||||||
~collection_mode:opts.collection_mode | ||||||
decode_opts)) | ||||||
decode_opts) | ||||||
in | ||||||
let output_config = decode_opts.Decode_opts.output_config in | ||||||
let output = Tracing_tool_output.output output_config in | ||||||
let output_file = | ||||||
match output with | ||||||
| `Sexp _ -> failwith "unimplemented" | ||||||
| `Fuchsia store_path -> store_path (* path for tracing output file *) | ||||||
in | ||||||
let%bind.Deferred () = | ||||||
match trace_processor_exe with | ||||||
| Disabled -> | ||||||
print_endline "Warning: must use local processor on large trace files"; | ||||||
Deferred.return () | ||||||
| Enabled processor_path -> | ||||||
Deferred.return | ||||||
(call_trace_processor | ||||||
~prog:processor_path.trace_processor_shell_path | ||||||
~argv:[ "-D"; output_file ] | ||||||
()) | ||||||
in | ||||||
return ()) | ||||||
;; | ||||||
|
||||||
let select_pid () = | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should leave a comment here about what versions of perf we observed breakages when close-ing and under what circumstances, because it was surprising.