Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new runtime events counters for major heap stats and minor heap sizing #11919

Merged
merged 3 commits into from Jan 29, 2023

Conversation

sadiqj
Copy link
Contributor

@sadiqj sadiqj commented Jan 19, 2023

This PR introduces some additional counters for runtime events which contain useful information about the state of the major heap and allocator. These are:

EV_C_MAJOR_HEAP_POOL_WORDS
EV_C_MAJOR_HEAP_POOL_LIVE_WORDS
EV_C_MAJOR_HEAP_LARGE_WORDS
EV_C_MAJOR_HEAP_POOL_FRAG_WORDS
EV_C_MAJOR_HEAP_POOL_LIVE_BLOCKS
EV_C_MAJOR_HEAP_LARGE_BLOCKS
EV_C_MINOR_HEAP_SIZE_WORDS

These are the same counts that are tracked in heap_stats. I opted not to include max as users could track it using individual pool/large word events if they wished.

Crucially though these events are emitted per-domain (using the domain-local heap_stats structures), this may prove useful identifying cases where's uneven major heap usage between domains (though adoption of pools makes attribution difficult if domains are spawning and joining frequently).

This is a re-do of an earlier attempt that was mixed in with some bugfixes. I think @gasche had some opinions on the last PR.

@gasche
Copy link
Member

gasche commented Jan 19, 2023

I opted not to include as users could track it using individual pool/large word events if they wished.

Is there a missing word here?

Copy link
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 have a question inline about C_MINOR_HEAP_SIZE_WORDS.

Is there documentation somewhere for the meaning of each runtime event? (If not, maybe we should consider having some?)

runtime/minor_gc.c Outdated Show resolved Hide resolved
| EV_C_MAJOR_HEAP_POOL_FRAG_WORDS
| EV_C_MAJOR_HEAP_POOL_LIVE_BLOCKS
| EV_C_MAJOR_HEAP_LARGE_BLOCKS
| EV_C_MINOR_HEAP_SIZE_WORDS
Copy link
Member

Choose a reason for hiding this comment

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

Hmm - is this a point to consider whether runtime_counter ought to be an extensible variant?

At a minimum, these need @since markings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point on the since annotations I will add those.

I'm not opposed to turning these in to an extensible variant but I don't know what that will do for the C API. Are there any examples in the compiler for similar things?

@sadiqj
Copy link
Contributor Author

sadiqj commented Jan 21, 2023

Is there a missing word here?

Fixed. Thanks.

@sadiqj
Copy link
Contributor Author

sadiqj commented Jan 21, 2023

Is there documentation somewhere for the meaning of each runtime event? (If not, maybe we should consider having some?)

No, I don't think they're individually listed anywhere. I think that would be a useful thing to have, is it better in the manual or in the mli?

@gasche
Copy link
Member

gasche commented Jan 21, 2023

I would go for the .mli, which is a simpler place to modify and also a more natural place to look.

@sadiqj sadiqj force-pushed the runtime_events_new_counters branch from 3d3985e to 550e0f1 Compare January 27, 2023 15:36
Copy link
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 believe that the proposed change is correct and useful. Approved.

As @dra27 hinted at, we might be observing growing pains with the idea of defining all runtime events in a single type -- it might be possible (as we mentioned already in the first round of runtime-event reviews) to have something more extensible than a single global variant definition. But I am not sure what a better design would be, the single global variant definition has advantages as well, and the PR does not make things worse than before in terms of design. So I support the idea (implicit here) of keeping doing the simple things as long as the (few) tool authors accept the breaking changes that it creates, and think about something more elaborate only once we perceive a problem with the current approach / we have more experience on the tooling side.

@gasche gasche merged commit bbdcaea into ocaml:trunk Jan 29, 2023
@sadiqj
Copy link
Contributor Author

sadiqj commented Jan 29, 2023

Thanks @gasche and @dra27 .

Agreed that we should discuss possible alternatives to the single global type. Is this a good candidate for a github discussion?

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

Successfully merging this pull request may close these issues.

None yet

4 participants