Allow values reachable from ephemeron keys to be collected by minor GC - #13643
Conversation
|
Out of curiosity I did a quick check of what effect this change has on the sandmark ZDD benchmark. I'm not sure what to make of it, maybe noise, but for this case it seems that trunk is faster: FTR these were all compiled with flambda -O3, and run on a mac with an M3 Max. |
|
I locally rebased this over #13740 and #13737 and ran the benchmark again. On top of those, this PR is very close in performance on that benchmark, seemingly around only 3% slower. The allocation stats indicate that the actual behavioral improvement of this PR does not take effect on this benchmark (all stats essentially the same, notably ~40% promotion rate), so this result serves as a way to indicate how much overhead the additional tracking this PR does for ephemerons in the case that it doesn't help. So nicely low. I do not yet understand why the behavioral improvement does not take effect on this benchmark. |
kayceesrk
added
the
Performance
|
Thanks for the benchmarking work here @jberdine! It was extremely helpful. I reproduced the same effect, seeing no real difference with the patch: Note that not only do these runs have the same elapsed time, they also agree on the weak table stats. Looking at the code of So, I removed the memoization from --- zdd_orig.ml 2025-01-20 15:44:18.611777747 +0000
+++ zdd.ml 2025-01-20 15:44:38.080741235 +0000
@@ -54,12 +54,15 @@
let node = Ite {v; t; e; hash} in
WeakSet.merge unique_set node
+let no_memo = Sys.getenv_opt "NO_MEMO" <> None
+
module Memo1 = struct
module Tbl = Ephemeron.K1.Make (T)
let create = Tbl.create
let find_or_add tbl p k =
+ if no_memo then k () else
match Tbl.find_opt tbl p with
| Some r -> r
| None ->
@@ -74,6 +77,7 @@
let create = Tbl.create
let find_or_add tbl p q k =
+ if no_memo then k () else
match Tbl.find_opt tbl (p, q) with
| Some r -> r
| None ->This made the program much faster: Ephemeron memoization does have some overhead but I'm surprised it's this large! However, even in The reason for this turns out to be that the patch is broken. This diff fixes it: diff --git a/runtime/minor_gc.c b/runtime/minor_gc.c
index b6f936865a..e42f20e79c 100644
--- a/runtime/minor_gc.c
+++ b/runtime/minor_gc.c
@@ -455,7 +455,7 @@ again:
re->offset != CAML_EPHE_DATA_OFFSET && /* ephe key (not data) */
Is_block(v) && /* a block */
young_start <= v && v < young_end && /* on *this* minor heap */
- Tag_val(v) != 0 && /* not already promoted */
+ Hd_val(v) != 0 && /* not already promoted */
atomic_compare_exchange_strong(data, &v, caml_ephe_locked)) {
/* locked, clean it later */
re->locked = v;The previous version of the patch was not correctly checking for promoted values (whose header is overwritten with zero), but instead checking that The table is about 10x smaller and the runtime 2.5x faster than trunk in |
f078a6a to
39988d7
Compare
This benchmark is about 90% ephemeron overhead in the best case; if I remove all ephemerons and weak tables (i.e. replace |
It is completely true that this benchmark does not benefit from the operation caching. I don't know if it is particularly useful for this PR as it also has the behavior where each element of the Weak array is immediately referenced (as data not just key) from an operation cache, but I proposed a more representative one ocaml-bench/sandmark#478 where the cache does effectively turn exptime operations into polytime. Comparing this PR vs trunk vs 4.14 using that benchmark: |
|
My understanding from this conversation is that @jberdine is now convinced that this PR would be good to have. Let's consider it a consensus (two people agree!); I would be happy to merge if it had been reviewed for correctness, but this is in a tricky part of the runtime and I wouldn't trust myself to do this review (in addition to not having much time to do it for now). If anyone is interested in reviewing for correctness, please be my guest :-) |
|
(It may be that @damiendoligez, @NickBarnes or @bobot would be willing to review the code.) |
| if (new_v != v) { | ||
| /* atomic CAS, because another domain might be trying to lock it. | ||
| (We don't care who wins the race, so result not checked) */ | ||
| atomic_compare_exchange_strong(data, &v, new_v); |
There was a problem hiding this comment.
How could another domain lock it, since it seems a domain only lock keys in its own domain?
There was a problem hiding this comment.
It's true that domains only lock keys in their own domain. This code runs when v is not in the current domain, so its owning domain may be trying to lock it.
There was a problem hiding this comment.
Thank you, I forgot that oldify_one can work on other domains minor heap. (Still this code can also run if v is in the current domain, no? e.g. DATA_OFFSET)
There was a problem hiding this comment.
Yes, it also runs in that case. The comment is about why the atomic CAS is needed, which is to handle the case when it is owned by another domain.
| value v = *data; | ||
| if (v != caml_ephe_none && Is_block(v) && Is_young(v) ) { | ||
| mlsize_t offs = Tag_val(v) == Infix_tag ? Infix_offset_val(v) : 0; | ||
| v -= offs; |
There was a problem hiding this comment.
Why does the handling of Infix_tag disappeared?
There was a problem hiding this comment.
Well spotted! I think the code here is correct by coincidence, because Infix_tag values never have zero headers (it always points to the prior header), so the optimisation doesn't apply to Infix_tag. I've changed it to be more explicit about this.
There was a problem hiding this comment.
Ok for the optimisation. But when the optimisation is not triggered (the else) why the handling of infix_tag is removed? Only oldify_one is done directly.
EDIT: I looked a little more at oldify_one, and it seems the previous code was just an inlining of the first part of oldify_one. So in other term the previous code could have been written as the following, and this change is independent:
atomic_value* data = Op_atomic_val(re->ephe) + re->offset;
value v = atomic_load_relaxed(data);
value new_v;
oldify_one(st, v, &new_v);
if (new_v != v) {
atomic_compare_exchange_strong(data, &v, new_v);
redo = 1
}
Yes, correct! |
|
We recently switched to OCaml 5.3.0 at LexiFi. As you may know we use hash consing heavily in the core of our codebase. We now have identified a real-life case where OCaml 5.3.0 takes 1.8x time relative to 4.14.2. With this PR, it takes 2x time relative to 4.14.2 (so it is actually worse than the current 5.3.0 performance). The timings (in s) are: 4.14.2: 6.44038796425 Unfortunately, I don't know if it is feasible to make the test self-contained enough so that it can be made available publicly. For what is worth, this is what the
and under 5.3.0:
|
|
It's hard to get much of use out of a perf screenshot! Can you post the GC summary output, as printed when running under Are you using ephemerons in generality or just as weak pointers? (i.e. do you use the stdlib |
Yes, sorry about that.
I will prepare these and upload them here.
I believe just the |
@stedolan The requested files are below. Thanks! GC-full-414.txt |
|
This is very strange! I have some notes and many questions:
The biggest differences between 4 and 5 and this PR appear to be in the allocation behaviour and lifetimes of allocated blocks. This information is very visible in a memory profile, so I'd suggest using memtrace, which might make the problem obvious. |
|
@nojb Do I remember correctly that you folks have your own implementation of weak.ml (based on the same runtime system primitives)? If so, do there appear to be significant differences, or is that sharable? |
|
Re Is there a way to tell from the stats if |
Yes.
I tried this, but I cannot spot anything obvious (to my untrained eye). Most of the difference in allocation behaviour seems to come from the trace-414.ctf.txt
From the memprof trace, this is the allocation profile for OCaml 4.14:
and for OCaml 5.3:
Nothing that I know of. In OCaml 4.14 we were using "best fit".
I will recheck and get back.
I will recheck and get back. |
Indeed, that's the case, but for the purposes of this discussion I replaced our custom version by the one in the standard library to try to make the measurements as reproducible as possible (the difference between the two was small). I will try to share our custom version later anyway. |
|
As @jberdine says:
This does seem to be the explanation for the allocation differences here, which are almost entirely inside Weak.Make.merge: The slower GC pacing is consistent with the high number of ephemeron marking cycles observed in both 5.3 and this PR: the GC is spending so much time and so many slices redoing ephemeron marking that it takes a very long time to complete a cycle, causing huge heap growth in the meantime. The mystery is why the GC is redoing ephemeron marking. Ephemeron marking begins only when the GC has finished all available marking work, and is redone only if new marking work is discovered during ephemeron marking. In principle, the only way that this can occur is if ephemerons with marked keys and not-yet-marked data are discovered. (In particular, new allocations are not sufficient, since new values are allocated This cannot happen here, since Weak uses only ephemerons with trivial data. From the memtrace, this program appears not to use the Yet it does, more than 200 times in a row. I think this would not be too hard to figure out by messing around in a debugger for a while, but I understand you can't share the original program. Would it be possible to try to cut it down to something shareable? (To reproduce the issue here, I don't care whether the performance difference is visible: the thing I'd like to have is a program that prints the |
I believe I have what you want here: https://github.com/lthls/ephemeron_repro I created it from some files I had made to investigate this issue earlier, and I verified that the setup that is currently committed produces the message you're looking for and only uses |
|
Thanks @lthls, that example was very helpful. The bug here is that OCaml 5 is missing a crucial optimisation when marking ephemerons. Ephemerons cause their data to be marked if all of their keys and the ephemeron block itself are marked. In general, when any marking occurs all ephemerons need to be re-examined to see whether any new data fields must be marked, which can be slow. There are two simple cases where this work need not be redone:
OCaml 5 implements only optimisation (1) above, not (2). The latter is particularly important for programs using The other thing that confused me before is the source of the extra marking work that triggers ephemeron remarking - ephemeron marking does not begin until you've run out of marking work, so it should be hard to trigger the re-marking logic. It turns out to be I'll try to get a fix up for this in the next day or two. |
1f7d545 to
dddcbae
Compare
The PR seems to be 5% or 10% slower than |
|
Personally I would still be in favor of merging a PR that improves the situation dramatically in some cases, at the cost of a 5-10% slowdown in some other cases. (I would rather avoid pathological runtime behaviors.) But we still need a full review and the change is arguably tricky/invasive. |
|
@stedolan Do we forbid to put weak pointers in weak pointers? The infamous |
That's true, but I don't propose we do anything about it. Putting weak pointers in weak pointers is fine, but
This patch adds "potentially create a forever-locked Weak.t" to this list of sins, but that seems no worse than the above. |
|
Thank you, I knew some of the problems of I have reviewed this PR, and think it should be accepted. |
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
I am approving this PR on behalf of @bobot -- thanks a lot for your time and your review!
This is tricky code and a non-trivial change, so ideally it would be nice to get extra pairs of eyes, but no extra eye has volunteered and @stedolan and @bobot are both experts in this stuff.
(I wanted to say "I'll wait a few days just in case and then go ahead and merge", but I wouldn't want to rely on @stedolan to ping me, we are all going to forget about it, so I propose that someone merges right away once the Changes entry is updated.)
| McGilchrist and Fabrice Buoro) | ||
|
|
||
| - #13643: Allow values reachable from ephemeron keys to be collected by minor GC | ||
| (Stephen Dolan, review by ??) |
Just browsing and came across this by accident. I hope it's okay if I ping @stedolan |
|
I went ahead, merged the PR, and fixed the Changes via a direct push. Thanks! |
|
Thanks! |
The runtime has emitted this phase since 5.4 (ocaml#13643), when a minor collection has to clean locked ephemerons, but the OCaml type had no constructor for it, so consumers were handed an out-of-range value and crashed when matching on it.
The runtime has emitted this phase since 5.4 (ocaml#13643), when a minor collection has to clean locked ephemerons, but the OCaml type had no constructor for it, so consumers were handed an out-of-range value and crashed when matching on it.
The runtime has emitted this phase since 5.4 (ocaml#13643), when a minor collection has to clean locked ephemerons, but the OCaml type had no constructor for it, so consumers were handed an out-of-range value and crashed when matching on it.
The runtime has emitted this phase since 5.4 (ocaml#13643), when a minor collection has to clean locked ephemerons, but the OCaml type had no constructor for it, so consumers were handed an out-of-range value and crashed when matching on it.
11 participants
Multicore Ephemeron GC is tricky, requiring a complex multi-round synchronisation to get right. To keep minor GC simple and fast, it punts on the complexity: all values in ephemeron keys or data are unconditionally promoted.
Unfortunately, this policy essentially disables generational GC for values stored in ephemerons (including weak tables). This patch improves the situation by identifying an easy case: the data of an ephemeron is still promoted unconditionally, but the keys are treated as weak references by the minor GC if they point to the current domain's minor heap.
This is not a full implementation of the ephemeron condition: in particular, if the data points to the key both will be promoted. Difficult cases like this are, as before, left to the major GC.
However, it greatly improves the situation for data using
Stdlib.Weakarrays (which are implemented as ephemerons with many keys and trivial data), especially for programs using hashconsing with lots of values that are only seen once. Here's a silly program, hashconsing values that are never reused:and its approximate runtime/space usage (on a Debian amd64 laptop):
I haven't bisected this to find exactly what changed, but I think the big improvement from 5.0 to 5.1 is due to @kayceesrk in #11743 and @NickBarnes in #12131. The results are noisy enough that I wouldn't read much into the differences between 5.1/5.2/5.3.
Note for reviewers: Start by reading the big comment and the new block of code in
oldify_mopupinminor_gc.c. This is the core logic of the patch. Everything else is:ephe_clean_minorand the other changes inminor_gc.c)weak.c)weak-ephe-final)