Conversation
|
Thanks a lot for this! I have a couple of questions:
|
|
|
We’re trying to get 2.2.0 out the door asap and i’d prefer to merge this after the release to avoid unnecessary time spent in extra maintenance and CI work (don’t worry the CI failure is unrelated to you changes). |
|
Of course, no worries! This is really a tool to debug/improve opam itself, not something end users urgently need, I think. |
6ed05e0 to
c7b6f5e
Compare
this cuts the time spent on parsing `pacman -Si` significantly down
c7b6f5e to
c64b484
Compare
kit-ty-kate
left a comment
There was a problem hiding this comment.
I've squashed the PR and cleaned a couple trailing whitespaces. It looks good overall but there are a couple of things to split off into their own PRs I think
|
|
||
| let simulate_autopin st ?quiet ?(for_view=false) ?locked ?recurse ?subpath | ||
| atom_or_local_list = | ||
| OpamTrace.with_span "AuxCommands.simulate_autopin" @@ fun () -> |
There was a problem hiding this comment.
I think it's better to give the full name of the module even if the Opam prefix is repetitive
| OpamTrace.with_span "AuxCommands.simulate_autopin" @@ fun () -> | |
| OpamTrace.with_span "OpamAuxCommands.simulate_autopin" @@ fun () -> |
| (* ## setup ## *) | ||
|
|
||
| let setup ~trace_file () : unit = | ||
| match trace_file, Sys.getenv_opt "TRACE_FILE" with |
There was a problem hiding this comment.
I think this should be OPAMTRACEFILE instead and be added through OpamCoreConfig and OpamArg like the rest of the environment variables
| (* old compat version (Re 1.2.0) | ||
| {[Re_str.split (Re_str.regexp (Printf.sprintf "[%c]+" c)) s]} *) | ||
| Re.(split (compile (rep1 (char c)))) s | ||
| let acc = ref [] in | ||
| let in_run = ref false in | ||
| let slice_start = ref 0 in | ||
|
|
||
| for i=0 to String.length s-1 do | ||
| if String.get s i = c then ( | ||
| if not !in_run then ( | ||
| if i > !slice_start then | ||
| acc := String.sub s !slice_start (i - !slice_start) :: !acc; | ||
| in_run := true; | ||
| ) | ||
| ) else ( | ||
| if !in_run then ( | ||
| in_run := false; | ||
| slice_start := i; | ||
| ) | ||
| ) | ||
| done; | ||
| if not !in_run && !slice_start < String.length s then | ||
| acc := String.sub s !slice_start (String.length s - !slice_start) :: !acc; | ||
| List.rev !acc |
There was a problem hiding this comment.
I think this change should be in its own PR
| if Sys.win32 then | ||
| fun ch fmt -> | ||
| flush (if ch = `stdout then stderr else stdout); | ||
| fun ~force_flush ch fmt -> |
There was a problem hiding this comment.
is this for performance improvement or does the tracing itself need it? If it's the former, i think this should be in its own PR
| @@ -0,0 +1,9 @@ | |||
| (** Tracing *) | |||
There was a problem hiding this comment.
| (** Tracing *) | |
| (**************************************************************************) | |
| (* *) | |
| (* Copyright 2023 Simon Cruanes *) | |
| (* *) | |
| (* All rights reserved. This file is distributed under the terms of the *) | |
| (* GNU Lesser General Public License version 2.1, with the special *) | |
| (* exception on linking described in the file LICENSE. *) | |
| (* *) | |
| (**************************************************************************) | |
| (** Tracing *) |
| @@ -0,0 +1,110 @@ | |||
| module Json = OpamJson | |||
There was a problem hiding this comment.
| module Json = OpamJson | |
| (**************************************************************************) | |
| (* *) | |
| (* Copyright 2023 Simon Cruanes *) | |
| (* *) | |
| (* All rights reserved. This file is distributed under the terms of the *) | |
| (* GNU Lesser General Public License version 2.1, with the special *) | |
| (* exception on linking described in the file LICENSE. *) | |
| (* *) | |
| (**************************************************************************) | |
| module Json = OpamJson |
This is WIP.
I'm adding tracing to find why opam is so slow on my machine. It gives excellent insights on where time is spent (e.g after
opam upd, all 30k files are read one by one, to update the cache).I've been instrumenting things as I explore but it needs to be more principled in general I think.
The overhead when tracing is not active (ie
TRACE_FILEis not set) should be fairly low.