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

Expose counts for each GC reason in GC.stat #7250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

casperisfine
Copy link
Contributor

@casperisfine casperisfine commented Feb 6, 2023

[Feature #19435]

This information is currently only available through debug counters which makes it unpractical to use in production.

However this information is very useful to optimize GC in applications.

cc @peterzhu2118 @eightbitraptor @jasonhl

@byroot byroot marked this pull request as ready for review February 7, 2023 11:06
gc.c Outdated
Comment on lines 9577 to 9581
if (reason & GPR_FLAG_NEWOBJ) objspace->profile.minor_gc_newobj_count++;
if (reason & GPR_FLAG_MALLOC) objspace->profile.minor_gc_malloc_count++;
if (reason & GPR_FLAG_METHOD) objspace->profile.minor_gc_method_count++;
if (reason & GPR_FLAG_CAPI) objspace->profile.minor_gc_capi_count++;
if (reason & GPR_FLAG_STRESS) objspace->profile.minor_gc_stress_count++;
Copy link
Member

Choose a reason for hiding this comment

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

It's misleading to call this the reason for minor GC. This is the reason for GC, which may be a major or a minor (it's a major when reason & GPR_FLAG_MAJOR_MASK, and a minor otherwise).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean for stress specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'm not sure exposing the stress count is very helpful, same with force, we could probably just not expose these.

I simply exposed them all without thinking too much.

Copy link
Member

Choose a reason for hiding this comment

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

No I mean for all of these. These flags tells us how the GC was started, but not what type of GC (major vs. minor). So I don't think it's correct to prefix these with minor_gc_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. I had no idea. So all of these reasons above could trigger a major?

Copy link
Member

Choose a reason for hiding this comment

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

I think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, I just dug into this, and I'm not sure you're correct:

    /* major reason */
    GPR_FLAG_MAJOR_BY_NOFREE    = 0x001,
    GPR_FLAG_MAJOR_BY_OLDGEN    = 0x002,
    GPR_FLAG_MAJOR_BY_SHADY     = 0x004,
    GPR_FLAG_MAJOR_BY_FORCE     = 0x008,
#if RGENGC_ESTIMATE_OLDMALLOC
    GPR_FLAG_MAJOR_BY_OLDMALLOC = 0x020,
#endif
    GPR_FLAG_MAJOR_MASK         = 0x0ff,

    /* gc reason */
    GPR_FLAG_NEWOBJ             = 0x100,
    GPR_FLAG_MALLOC             = 0x200,
    GPR_FLAG_METHOD             = 0x400,
    GPR_FLAG_CAPI               = 0x800,
    GPR_FLAG_STRESS            = 0x1000,

So none of these are ever gonna match the MAJOR_GC_MASK. It is true that they might be flipped up at the same time than a major reason, hence a major will trigger, but as far as I can tell they are never the reason for a major.

Copy link
Member

Choose a reason for hiding this comment

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

They aren't. They don't tell what kind of GC is being run. But that doesn't mean they guarantee a minor. The GC reason could cause both either a major or minor GC, so I don't think it's accurate to call it minor_gc_*_count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I guess that's where the misunderstanding is.

The point isn't to say a minor was triggered. I don't expect the sum of minor_reason to match minor_gc_count.

The goal here is to see how many time we reached these limits, if a major was triggered instead I still want these to be bumped because if you change the settings to avoid the major, you'd see that minor.

Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm getting more confused.

The point isn't to say a minor was triggered.

If a minor wasn't triggered (and a major was triggered instead), isn't it misleading to call it minor_gc_newobj_count or minor_gc_malloc_count?

@casperisfine
Copy link
Contributor Author

I'm removing the following counters as I don't think they really help tuning GC in any way:

  • minor_gc_method_count
  • minor_gc_capi_count
  • minor_gc_stress_count
  • major_gc_force_count

[Feature #19435]

This information is currently only available through debug counters
which makes it unpractical to use in production.

However this information is very useful to optimize GC in applications.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants