Skip to content

Emit major slice counters in the runtime events#13364

Merged
kayceesrk merged 1 commit into
ocaml:trunkfrom
kayceesrk:major_slice_counters
Aug 21, 2024
Merged

Emit major slice counters in the runtime events#13364
kayceesrk merged 1 commit into
ocaml:trunkfrom
kayceesrk:major_slice_counters

Conversation

@kayceesrk
Copy link
Copy Markdown
Contributor

This PR emits the counters updated as part of the major slice computation. The related discussion is here: #13321. olly has already been updated to emit these counters in the trace: tarides/runtime_events_tools#46.

I had originally planned to make this PR after #13320 was merged. However, it looks like it may take longer: #13320 (comment). The change itself can be easily modified to accommodate future changes in the slice computation.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine overall. I think it would be nice to try to document a bit the new runtime_counters constructors, possibly just to say that they correspond to internal implementation details of the GC related to pacing computation.

Live blocks of a Domain's major heap large allocations.
@since 5.1 *)
| EV_C_MAJOR_HEAP_WORDS
| EV_C_MAJOR_ALLOCATED_WORDS
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructors above all have careful documentation. Shouldn't we try to document those as well, or at least make it explicit why they are not documented?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Let me try to document these.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would generate useful documentation on the Runtime_events module.
Unfortunately some of the other constructors are undocumented.

@sadiqj
Copy link
Copy Markdown
Contributor

sadiqj commented Aug 18, 2024

I've added some docs for the constructors in kayceesrk#16

One problem with this PR (ocaml/ocaml) though is that major_collection_slice can get hammered pretty frequently as we try to do opportunistic major slices in both in the minor GC entry and exit barriers. It is called in the spin wait loop and we had to add a flag to prevent emitting tens of thousands of phase events: https://github.com/ocaml/ocaml/blob/trunk/runtime/major_gc.c#L1643

We probably don't want to emit events when that happens. (do we even want to be updating our work in that case?)

@kayceesrk
Copy link
Copy Markdown
Contributor Author

(do we even want to be updating our work in that case?)

Good point. I see that slice_target and slice_budget are updated in the case of opportunistic slices. We'd have to do something about them.

I'd prefer to keep logic changes in a separate PR. For now, I've updated update_major_slice_work to take in log_events as an argument. The caml_gc_log and caml_gc_message remain unguarded as is the case with major_collection_slice.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy with the current state. Approved. Thanks @sadiqj for the review, suggestion to use log_events, and documentation.

@kayceesrk kayceesrk force-pushed the major_slice_counters branch from 28179ba to c31de35 Compare August 21, 2024 07:06
@kayceesrk
Copy link
Copy Markdown
Contributor Author

Thanks @gasche.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants