Reserve only caml_minor_heap_max_wsz * Max_domains for the minor heap#11082
Reserve only caml_minor_heap_max_wsz * Max_domains for the minor heap#11082kayceesrk merged 1 commit intoocaml:trunkfrom
Conversation
|
Looks nice, thanks! However the tests fail because you have a bunch of C functions that are not prefixed by |
|
This PR contains 20% of the feature set discussed in #10971 and 80% of the difficulty: we don't handle changing the maximum number of domains here, but we still have to un-map and re-map minor heaps at runtime, which is the hard part. We hope to be able to add other features easily on top. (@sabine and myself have prototyped those additional features, but we went back to the bare minimum when we had to track a segfault by staring inquisitively at the diff.) In the middle of a Stop-The-World (STW) section, we unmap and then re-map the minor heap space, and we modify the When @sabine and myself wrote the code of the present PR, I was assuming that no code touching the domain structures could not run in parallel with STW sections (before a STW barrier), and that this justified the correctness of our change. In fact the assumption, as discussed in #11072, is not entirely correct: domain initialization code cannot run in parallel with STW sections, but domain cleanup code can. All this to say: reviews are warmly welcome! (cc @Engil, who did a pre-review already, and @ctk21 who is most probably interested in looking at this.) |
aaab6c4 to
1c814d4
Compare
|
Sorry for the delay in reviewing this: I will be taking a look at this PR very shortly, at the very least by the end of Monday I will have a review for it. Thank you very much for this work! |
ctk21
left a comment
There was a problem hiding this comment.
Good feature that fixes some real problems with large virtual allocations. I believe the code to work as intended.
I was surprised that a new STW section was introduced rather than just adding the functionality onto the end of the existing minor heap collection; you always have to empty the minor heaps to resize. However the implementation here works fine.
I see one possibility that might simplify things and deals with the FIXME in the code: I need convincing that we really need to expose both caml_set_minor_heap_size and caml_update_minor_heap_max.
There was a problem hiding this comment.
Thank you very much for this PR @sabine, @gasche !
I already did a preview a few weeks back with @gasche, and the changes looks good to me.
I have a few minor nitpicks; however I think the STW segment is pretty straightforward and has no flaws I can see.
As for the whether or not adding a new STW segment is preferable to reusing one: I tend to be a neutral about the notion. (at least here.)
We made similar changes when dealing with DLABs and I was not necessarily fond of extending an already complex STW segment (minor collection) to support resizing/slabbing of the minor segments.
I think the use case here is fine, and the general world of advice for dealing with STW segments should be to avoid touching or adding them as much as possible. (which I think @ctk21 has stated many times in the past :-))
Food for thoughts as well would be could this segment be reused for later extension of the minor heap allocator (the return of slabbing?).
Or would there be a mix of things between this new STW segment and the regular minor collection segment?
I think if we were to have a fancier, more dynamic allocator for the minor heap, if STW is required, reusing this segment rather than using the minor heap collection one should be fine.
Lastly, another item to ponder upon one day: I see @sabine and @gasche are also now enjoying the virtue of the burn-style tests! (a true Multicore staple.)
I wonder if we should one day factor some common bits in the ones we have, if only to have a streamlined control interface.
(which is a throwback to some discussions around the testsuite and parallel testcases)
runtime/domain.c
Outdated
| void* heaps_base; | ||
| /* minor heap initialization and resizing */ | ||
|
|
||
| static uintnat get_minor_heap_reservation_size() { |
There was a problem hiding this comment.
The two places where get_minor_heap_reservation_size is used, the result is immediately passed to reserve_minor_heaps. Best to push get_minor_heap_reservation_size code into reserve_minor_heaps and delete that function.
5c94e6a to
3375448
Compare
7cc6fdf to
f231d7b
Compare
|
My thinking was the second ("modify the code of the minor gc"); the argument for this would be that it keeps all the minor heap code in one place and we would just check to see if the resize code path is needed at the point we hit the minor GC collection barrier (that is the point that all domains signal they have collected their minor heap). An argument against is that the STW minor GC collection function is big and hairy. It seems like you considered this in your design and decided what you are doing is better. That's fine! |
xavierleroy
left a comment
There was a problem hiding this comment.
I'm still unsure what needs to be done to get this PR merged. At any rate here are silly comments on the test and the Changes entry. I'm not sure we need a Changes entry, but if we have one it should explain the changes w.r.t. 4.14, not w.r.t. Multicore OCaml.
f231d7b to
72e3a3c
Compare
|
Yes, a Changes entry should not be needed because none of the possible issues arising from large heap reservation were ever exposed to the user. All we're doing here is to fix the memory reservation/allocation strategy in such a way that it does what the user already expects (i.e. minimal amount of reservation and allocation). Looking at 4.14 as the reference point for the Changes entry is the correct thing to do. Thank you for reminding, it's something we need to take into account for all the multicore-related PRs. |
abbysmal
left a comment
There was a problem hiding this comment.
I took a last look at this PR and besides one final nitpick to the eventlog_metadata.in file I think things looks good here.
When a domain uses Gc.set to increase its own minor heap size to be larger than the current reservation, we reserve a new memory area for all minor heaps. This is done inside a STW section: 1. every domain frees its own heap, 2. inside a global barrier, one domain unreserves the old minor heap area, then reserves the new minor heap area. This domain also adjusts the minor heap area pointers of all domains while the other domains wait for the end of the barrier, 3. every domain allocates its own heap with size `Caml_state->minor_heap_wsz`. Co-authored-by: Gabriel Scherer <gabriel.scherer@gmail.com>
72e3a3c to
ffda59d
Compare
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
Every call to `Gc.set` that modifies the minor heap size causes a stop-the-world section, in order to adjust the minor heaps of all domains. On `domain_terminate()`, we free the minor heap to maintain the invariant "only active domains have a minor heap allocated". Resulting simplifications: we no longer keep track of individual domains' minor heap size - instead, we have a global variable `caml_minor_heap_wsz` for the size of the minor heap, and every domain allocates a minor heap of this size. This patch resolves the FIXME from ocaml#11082 relating to the duplicate reallocation of the leader domain's minor heap.
This addresses #10971 in the sense that
s * Max_domainsGc.setto increase their minor heap size beyond the reservation (causes STW) or modify their minor heap size within the current reservation (doesn't cause STW)caml_minor_heap_max_wsz * Max_domainsIt's now possible to use tools like AFL and Valgrind. The changes to
Gc.setdo not break existing code.There is the possibility of a different pull request to add an OCAMLRUNPARAM option to set
Max_domains, but we intentionally did not include this here to keep the patch minimal.summary
When a domain uses Gc.set to increase its own minor heap size to be larger
than the current reservation, we reserve a new memory area for
all minor heaps. This is done inside a STW section:
area, then reserves the new minor heap area. This domain also adjusts
the minor heap area pointers of all domains while the other domains
wait for the end of the barrier,
Caml_state->minor_heap_wsz.Co-authored-by: Gabriel Scherer gabriel.scherer@gmail.com