CP-17268 integrate coverage profiling #267
Conversation
Bisect dumps it report by registering a function through at_exit. This wasn't called by xenopsd. We added a handler to catch the SIGTERM signal used by the systemd service framework to stop the service. This leads to the profiling data being dumped. Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
Since opam is not used to pull in all dependencies I'm not sure this opam file is working as intended. I've just added the biscec_ppx dependency. See the README for how to control where log data is written. Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
We don't want all log data to end up in / where others might log, too. This code finds a temp dir and directs bisect_ppx to log there. The BISECT_FILE env variable can still override this. Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
We need this code for the other binaries in this repo, too. Hence, it better lives in its own module Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
To set up a build for coverage profiling, use "make coverage" followed by "make". This should it make possible to interate this code into the main line. Supporting code lives in profiling/. All other changes to the code are concerned with better signal handling and should likewise go into the main line. Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
….ml) This was missed in the previous commit. Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
libvirt/xenops_libvirt_main.ml
Outdated
|
|
||
| (* Start the program with the libvirt backend *) | ||
| let _ = | ||
| Coverage.init "xenopsd-libvirt"; |
There was a problem hiding this comment.
Indentation does not seem right here.
|
Since you are touching all the oasis auto-generated files, this would be a good opportunity to switch to |
| xenstore.unix, | ||
| xenstore_transport, | ||
| xenstore_transport.unix, | ||
| oclock |
There was a problem hiding this comment.
I'm glad you have split these out onto separate lines! Much better for resolving merges. :)
There was a problem hiding this comment.
I'd like to move to oasis setup -setup-update dynamic in a separate step. It is not in all projects straight forward but I have tested that the proposed method works in that context, too. It's done here: https://github.com/lindig/luna-rossa.
There was a problem hiding this comment.
That's fine. Let's make sure we come back to it before we wrap this work up.
Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
This was an oversight in the last commit. Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
|
Can we get this merged? Since this is a testbed for more repos I'd like to see this exercised in a test. |
_tags
Outdated
| # compile and link with package bisect_ppx | ||
| <**/*.ml{,i,y}>: pkg_bisect_ppx | ||
| <**/*.native>: pkg_bisect_ppx | ||
| # END_COVERAGE |
There was a problem hiding this comment.
This is what I'm most nervous about with this method.
|
If @euanh is happy then I'm happy. |
|
49f1024 This makes me nervous at how easy it is to check this file in accidentally |
|
I agree. That's why it would be better in the long run to build regular and instrumented binaries in the same build. As we discussed, I'd suggest we look into the way |
|
There are still a couple of indentation slips that @euanh noticed that still need fixing. Other than that, I agree that switching modes is dangerous, but this approach is at least nicer than some we've had in the past. If we can get ocamlbuild to do a parallel coverage-enabled build all in one go, that's definitely the way we should be going, but that might take a bit more time than we have right now to sort out. |
Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
This change adds optional coverage profiling using
bisect_ppx. The code changes don't affect the normal (non-profiling) case. To set up the build environment for a profiling build, do:The first step rewrites the
_tagsfile to enable code instrumentation. It can be reverted withSee the COVERAGE.md for general information about coverage profiling. Supporting code lives in
profiling/-- it doesn't have an effect when not profiling as it only sets up an environment variable. The build requiresbisect_ppxto be available. This has been added to the SPEC file andoasisas well (although this is not used).An important code change is a handler for the
SIGTERMsignal to make sure the profiling code writes it data to a file before the daemon terminates.