Basic wiring to connect multicore statmemprof to GC and allocation #12390
Conversation
4fd0bb6 to
5c60ad9
Compare
runtime/major_gc.c
Outdated
| { | ||
| uintnat num_domains_in_stw; | ||
|
|
||
| /* TODO: Not clear this memprof work is really part of the "cycle" |
There was a problem hiding this comment.
@sadiqj I'd appreciate your input on this block. As the comment describes, I had a version in which this was separated out into its own function, which I called on all participating domains, but it failed because caml_try_run_on_all_domains() would push domains back into mark/sweep. See this commit which fails numerous tests such as tests/memory-model/forbidden.ml
There was a problem hiding this comment.
I'm just trying to understand what needs to be done at caml_memprof_after_major_gc so I can figure out where it's best placed.
I was looking at 4.14.1 and I only see caml_memprof_update_clean_phase being called from the major_gc. Is that the intention for this hook, that it should be called at the point where ephemerons need cleaning? That's a different place from where it is here.
There was a problem hiding this comment.
It needs to be called after marking is complete, for a similar purpose to cleaning ephemerons - memprof needs to walk its internal data structures, to update all entries for blocks which have not survived collection. Originally I put the call in major_collection_slice, close to the ephemerons stuff, but that didn't work as previously noted. It's ended up here as a placeholder, really.
|
Note: this PR now incorporates #12398 (a small change to the test suite which I made to get this to pass). |
jhjourdan
left a comment
There was a problem hiding this comment.
These changes are small, and not really arguable. The one thing I'm not sure about is the call to caml_memprof_after_major_gc, because I'm not an expert on functionning of multicore's GC. It seems like it's indeed not at the right place, but @sadiqj will know better than me.
Appart from that, the changes to lib-runtime-event/test_caml.ml does not seem to be related, so it should be submitted as a separated PR.
(Other minor comments, see details of review.)
|
Yet another comment: when #12193 will be merged, this will have to be adapted to take into account memprof's roots. |
Yes. Happily this should be doable by simply calling |
|
Well, that's not that easy, because the weaknesses of the roots are different, so you don't traverse the same roots when doing mark&sweep and when doing compaction (where you need to traverse al the roots, including weak ones). |
You are quite right. I think the strong roots should be done in that way, and the weak roots by adding something akin to |
fb80ad4 to
f119bbc
Compare
|
#12398 is merged now so this now passes that test. |
|
@NickBarnes can you resolve the conflicts on the This PR was approved in July. Are we ok to merge? |
7260532 to
0c33306
Compare
0c33306 to
e1621da
Compare
|
rebased. |
e1621da to
43c9b44
Compare
|
Rebased. |
43c9b44 to
639ccad
Compare
639ccad to
7e0616a
Compare
|
This PR is wholly subsumed by the forthcoming main statmemprof PR, so should not be merged. Converting to draft. |
|
Closing as wholly included (and improved!) in #12923. |
Statmemprof needs to be called when allocations are made, and at the start and end of each GC (at the start for scanning roots, at the end for cleaning up weak roots pointing to any tracked blocks which have been collected). This PR adds an appropriate interface, with stub implementation, to the memprof module, and adds calls to those stubs from the GC and memory modules.
This is the latest in a series of small PRs working towards multicore statmemprof. See #12379 for a monolithic (unreviewable) PR with a more complete implementation.