Update stats at the end of a major cycle (and potentially a compaction)

[sadiqj]

Small change but one that bit me last week. This PR adds an additional caml_collect_gc_stats_sample_stw to the major heap cycling stw. This means that Gc.quick_stat now actually reflects the state of the heap after a major cycle or compaction.

[gasche]

I checked that the synchronization looks correct: this is running during a critical STW section, on all domains.

I am wondering about the placement of this call, however. This is below a conditional call to the compaction logic, and the comment says "this is useful for example if compaction happened":

• Is this useful only if compaction happened, in which case I would suggest moving it inside the compaction code (possibly at the end of the COMPACT_RELEASE phase)?
• or is it useful for some other reasons, which ones?

Minor comment: the definition of sampled_gc_stats in gc_stats.c has a comment that explains that stats are updated on each minor collection, which is invalidated by the present change. It could either be kept up-to-date by adding "and after each {compaction, major cycle}", or possibly be made vaguer.

[sadiqj]

Thanks for taking a look @gasche

Is this useful only if compaction happened, in which case I would suggest moving it inside the compaction code (possibly at the end of the COMPACT_RELEASE phase)?

A major slice may result in the pool words / live words stats changing. We can't also put an update in to major_collection_slice because it's not always in a stw.

Minor comment: the definition of sampled_gc_stats in gc_stats.c has a comment that explains that stats are updated on each minor collection, which is invalidated by the present change. It could either be kept up-to-date by adding "and after each {compaction, major cycle}", or possibly be made vaguer.

Updated the comment in f1183fc

[dra27]

I took the lack of a comment between gasche's review and approval as meaning he did not require being added to the list of reviewers in Changes ... if that's wrong, please push to trunk 🙂

[gasche]

I rarely explicitly require to be added as a reviewer (it's a fairly unpleasant thing to do), but I believe that it would have been the right thing to do in this case as I did an actual review. Of course I agree that this is just fine as I will push to trunk. I failed to be explicit about should be done before merging, mostly because I hadn't done the job myself of wondering about this.

[dra27]

I have updated my mental model for next time - although I would say that "Please can I be added to the list of reviewers in the Changes entry" shouldn't be too bad a thing to have to ask!

[sadiqj]

Oops, was just getting around to updating Changes and merging, have seen you've beat me too it. Thanks for the review @gasche .

[shindere]

David Allsopp (2023/12/21 04:25 -0800):

I have updated my mental model for next time - although I would say that "Please can I be added to the list of reviewers in the Changes entry" shouldn't be too bad a thing to have to ask!

Couldn't we even have one of the CI actions suggest names that may be added to the Changes entry, based on the names of thepersons who have commented on the PR? It feels it would be a gentle reminder to the PR's author and it would avoid those who reviewed to have to explictly asked to be added.

[gasche]

It's a delicate social balance. The question of what comments "count" as reviews is highly subjective and I think that it is better to rely on the committers' best judgment than trying to nudge them using automated processes. (For example: would people feel comfortable not following the recommendation of such an automated tools for comments that do not correspond to an actual review effort?)

[shindere]

Gabriel Scherer (2023/12/21 08:35 -0800):

It's a delicate social balance. The question of what comments "count" as reviews is highly subjective and I think that it is better to rely on the committers' best judgment than trying to nudge them using automated processes. (For example: would people feel comfortable *not* following the recommendation of such an automated tools for comment that do not correspond to an actual review effort?)

I'd say it's all a question of how things are worded. I'm pretty sure it is possilbe to word things in a way that people do not feel pressurized. But I do understand and share your concern andappreciate that you voiced it.