Gc stats fix #2825
Gc stats fix #2825
Conversation
|
This seems right to me! @stedolan should also glance. We should upstream this if it's correct. |
ocaml/runtime/gc_stats.c
Outdated
| @@ -57,11 +57,18 @@ void caml_accum_alloc_stats( | |||
| acc->forced_major_collections += s->forced_major_collections; | |||
| } | |||
|
|
|||
| double caml_minor_words(caml_domain_state *local) | |||
| { | |||
There was a problem hiding this comment.
This function shouldn't take a caml_domain_state* parameter, because its mechanism only works to compute the minor words of the current domain. Other domains (a) do not synchronise accesses to young_end / young_ptr, so this code causes a data race and (b) do not keep these values up to date, since young_ptr is kept in a register while running OCaml code.
ocaml/runtime/gc_stats.c
Outdated
| void caml_collect_alloc_stats_sample( | ||
| caml_domain_state *local, | ||
| struct alloc_stats *sample) | ||
| { | ||
| sample->minor_words = local->stat_minor_words; | ||
| sample->minor_words = caml_minor_words(local); |
There was a problem hiding this comment.
I think this change is in the wrong place, since this only makes sense to do for the current domain (see other comment)
|
It would be good to get a clear spec (and ideally, tests) for what the intended behaviour actually is in multi-domain programs. There are several functions that compute stats about the GC, including The original design was that This changed at some point, so now stat and quick_stat are sampled stats for other domains plus immediate stats for the current domain. That patch isn't quite right though, because it gets the wrong value for minor allocations, and I think the intent here is to change that. So, is the idea that after this patch, the minor allocations / promotions fields of quick_stat and counters should change by the same amount if only one domain is allocating? What sort of code motivated this change? If it was using |
|
It was a single-domain program that motivated this change: Users noticed that
Yes. I'd be in favor of making the necessary changes in this patch to just dereference |
Sounds good to me. I'm not completely happy with this meaning for |
Signed-off-by: Thomas Del Vecchio <tdelvecchio@janestreet.com>
fef6c84 to
bb50ae3
Compare
|
Apologies, I missed that more code had been pushed here addressing my comments above! At this stage, let's wait until 5.2 is merged (since it affects a bunch of the same code), but then I'll re-read this and we should merge it. |
|
@stedolan ping |
|
@stedolan ping again |
Fixes an issue where
Gc.quick_stat ()would not update minor words until a gc promotion happened. Also fixes a mis-reporting of minor words oncaml_do_exit.Testing: In current HEAD, repeatedly calling
Gc.quick_stat ()would not update the minor words until you allocate enough to cause a gc, but now it updates on every call toGc.quick_stat, which matches the behavior of the Ocaml 4 runtime.