Minor post-merge improvements to statmemprof #13068
Conversation
NickBarnes
force-pushed
the
nick-statmemprof-tweaks
branch
from
4e1c367 to
6f6ef42
Compare
gadmm
left a comment
gadmm
left a comment
There was a problem hiding this comment.
Looks good to me. One minor question below about something that could be worth documenting in the code.
runtime/memprof.c
Outdated
| /* Final attempt to run allocation callbacks. */ | ||
| if (!thread->suspended) { | ||
| update_suspended(domain, true); | ||
| value res = entries_run_callbacks_exn(thread, &thread->entries); | ||
| update_suspended(domain, false); | ||
| (void) caml_raise_if_exception(res); | ||
| } |
There was a problem hiding this comment.
You seems careful not to call caml_memprof_run_callbacks_exn (another choice could be caml_process_pending_actions). What is the reason for this choice?
There was a problem hiding this comment.
This was just a judgement call. I mainly want to run any pending allocation callbacks here, to avoid them being discarded. Pending allocation callbacks are all on the thread->entries table. If I callcaml_memprof_run_callbacks_exn, then other entries tables for this domain will be processed too, including (at present) the domain->entries table before the thread->entries callbacks. This may prevent the thread->entries callbacks from running, for example if one of the domain->entries callbacks raises an exception.
This is really just an aesthetic choice, as of course those other callbacks might run at any time.
There was a problem hiding this comment.
I've added a comment.
Try to run deferred allocation callbacks before stopping sampling. No change entry needed.
NickBarnes
force-pushed
the
nick-statmemprof-tweaks
branch
from
6f6ef42 to
b029b8c
Compare
* Import new Gc.Memprof API * Import new statmemprof testsuite * Runtime4 support for new statmemprof testsuite * Port 12383: backtrace abstractions * Import runtime5 statmemprof implementation Ports PRs ocaml#12382, ocaml#12817, ocaml#12824, ocaml#12923, ocaml#13068 * Avoid allocating unnecessarily during bytecode callbacks * Minor statmemprof testsuite robustness fixes
Minor post-merge improvements to statmemprof
Minor post-merge improvements to statmemprof (cherry picked from commit 753c8ac)
Minor post-merge improvements to statmemprof (cherry picked from commit 753c8ac)
5 participants
A couple of very minor improvements to statmemprof, following comments made after the merge of #12923:
caml_memprof_stop(), to further increase the likelihood of running all appropriate allocation callbacks.