Add VGC output for Container #7199

kangyining · 2023-12-08T17:30:39Z

Add two booleans to the VGC's output

addressable Physical Memory
If the JVM is running inside a container, and its memory limit is set

Signed-off-by: Frank Kang [email protected]

amicic · 2023-12-08T17:35:01Z

gc/verbose/VerboseHandlerOutput.cpp

@@ -363,6 +363,8 @@ MM_VerboseHandlerOutput::outputInitializedStanza(MM_EnvironmentBase *env, MM_Ver

 	buffer->formatAndOutput(env, 1, "<system>");
 	buffer->formatAndOutput(env, 2, "<attribute name=\"physicalMemory\" value=\"%llu\" />", omrsysinfo_get_physical_memory());
+	buffer->formatAndOutput(env, 2, "<attribute name=\"using container\" value=\"%s\" />", OMR_CGROUP_SUBSYSTEM_MEMORY == omrsysinfo_cgroup_are_subsystems_enabled(OMR_CGROUP_SUBSYSTEM_MEMORY)?"true":"false");


The condition should include '&& omrsysinfo_cgroup_is_memlimit_set().

Both conditions must be met before we take the path of 75% rule for containers.

sorry misread that you broke it into 2 lines...

I don't know if this check really represent 'using container' , so I'd rather simplify and not report it separately, but combine it with the other one. I think we just want to report here which of the 2 possible heuristics we chose (75% one or 25% one)

But we can hear @dmitripivkine's opinion, too.

I would not call omrsysinfo_cgroup_is_memlimit_set() in the line below if omrsysinfo_cgroup_are_subsystems_enabled() does not return true. It does not seem intended to be called this way (reports errors in Snap traces).

I agree with Aleks we just want to distinguish 75% and 25% cases

to be precise there is also case 50% if total memory is less than 1G

... or alternatively just report here in one line "run in container with memlimit set to <value>" (not sure "container" is generalized word enough. Also might be it is a place to report about memlimit value.

I'm ok with cashing the result of 'container limit set' in GC Ext and using that in VGC.

But I don't think that we need to report what that limit is. That limit already affected omrsysinfo_get_addressable_physical_memory() value (and our internal usablePhysicalMemory cached value) which we should report as a separate line regardless if container limit is set or not (or if it's the same or different value than omrsysinfo_get_physical_memory() value). I think this is just a slightly more generic way of reporting in case omrsysinfo_get_addressable_physical_memory() is affected by some other things than the container limit.

If we are sure that the containter limit is always (the only one) reason that will make omrsysinfo_get_addressable_physical_memory() be different/lower than omrsysinfo_get_physical_memory(), then I'm ok reporting it as a container limit value.

Again, the point of the flag is just to clarify why reported maxHeapSize looks like 75/50% (because container limit was set) vs 25% (because container limit was not set) of addressablePhysical memory.

ok, I agree to keep it more general. Should we have formal "selection reason" variable to be reported (there are two settings at the moment, "general" and "container") ?

I was thinking that same line "container limit set" but value="false" would mean "general/default". Perhaps that line could be slightly formalized (feel free to suggest something specific), but I prefer no new lines to report.

amicic · 2023-12-08T17:37:56Z

gc/verbose/VerboseHandlerOutput.cpp

@@ -363,6 +363,8 @@ MM_VerboseHandlerOutput::outputInitializedStanza(MM_EnvironmentBase *env, MM_Ver

 	buffer->formatAndOutput(env, 1, "<system>");
 	buffer->formatAndOutput(env, 2, "<attribute name=\"physicalMemory\" value=\"%llu\" />", omrsysinfo_get_physical_memory());


Let's add another line for omrsysinfo_get_addressable_physical_memory with name="addressablePhysicalMemory"
This is actually a more important one, since this is the one we use for further Xmx/softmx calculations.

amicic · 2023-12-11T20:08:01Z

gc/verbose/VerboseHandlerOutput.cpp

@@ -363,6 +363,10 @@ MM_VerboseHandlerOutput::outputInitializedStanza(MM_EnvironmentBase *env, MM_Ver

 	buffer->formatAndOutput(env, 1, "<system>");
 	buffer->formatAndOutput(env, 2, "<attribute name=\"physicalMemory\" value=\"%llu\" />", omrsysinfo_get_physical_memory());
+	buffer->formatAndOutput(env, 2, "<attribute name=\"addressablePhysicalMemory\" value=\"%llu\" />", omrsysinfo_get_addressable_physical_memory());
+	buffer->formatAndOutput(
+		env, 2, "<attribute name=\"using container and its memory limit is set\" value=\"%s\" />",


Do this as well: #7199 (comment)

In this change that flag would default to false, but you'd need to follow up with another change where some other code sets the value. Specifically for OpenJ9 you'd need a change there (OMR GC does not look into container limit).

Also I prefer a bit shorter name like 'container memory limit set/selected'. The fact that the container limit is set implies that we are using container.

amicic · 2023-12-12T17:44:38Z

looks good, but update the commit/PR comment

Add two booleans to the VGC's output 1. addressable Physical Memory 2. If the JVM is running inside a container, and its memory limit is set Signed-off-by: Frank Kang [email protected]

amicic · 2023-12-12T19:08:00Z

@babsingh please, proceed

babsingh · 2023-12-12T19:46:30Z

jenkins build all

babsingh · 2023-12-12T21:33:52Z

Only known failures #6516 and #7181 are seen.

kangyining requested review from charliegracie and youngar as code owners December 8, 2023 17:30

github-actions bot added the comp:gc label Dec 8, 2023

amicic reviewed Dec 8, 2023

View reviewed changes

kangyining force-pushed the add_container_vgc branch from a593d6f to ffca81b Compare December 8, 2023 17:54

amicic reviewed Dec 11, 2023

View reviewed changes

Add VGC output for Container

303aa86

Add two booleans to the VGC's output 1. addressable Physical Memory 2. If the JVM is running inside a container, and its memory limit is set Signed-off-by: Frank Kang [email protected]

kangyining force-pushed the add_container_vgc branch from 632df8e to 303aa86 Compare December 12, 2023 18:31

amicic approved these changes Dec 12, 2023

View reviewed changes

babsingh approved these changes Dec 12, 2023

View reviewed changes

babsingh merged commit 1b204d4 into eclipse:master Dec 12, 2023
16 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VGC output for Container #7199

Add VGC output for Container #7199

kangyining commented Dec 8, 2023 •

edited

Loading

amicic Dec 8, 2023

amicic Dec 8, 2023

amicic Dec 8, 2023 •

edited

Loading

dmitripivkine Dec 11, 2023

dmitripivkine Dec 11, 2023

dmitripivkine Dec 11, 2023 •

edited

Loading

amicic Dec 11, 2023

amicic Dec 11, 2023

dmitripivkine Dec 11, 2023 •

edited

Loading

amicic Dec 11, 2023

amicic Dec 8, 2023

amicic Dec 11, 2023

amicic commented Dec 12, 2023

amicic commented Dec 12, 2023

babsingh commented Dec 12, 2023

babsingh commented Dec 12, 2023

		@@ -363,6 +363,8 @@ MM_VerboseHandlerOutput::outputInitializedStanza(MM_EnvironmentBase *env, MM_Ver

		buffer->formatAndOutput(env, 1, "<system>");
		buffer->formatAndOutput(env, 2, "<attribute name=\"physicalMemory\" value=\"%llu\" />", omrsysinfo_get_physical_memory());

Add VGC output for Container #7199

Add VGC output for Container #7199

Conversation

kangyining commented Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic commented Dec 12, 2023

amicic commented Dec 12, 2023

babsingh commented Dec 12, 2023

babsingh commented Dec 12, 2023

kangyining commented Dec 8, 2023 •

edited

Loading

amicic Dec 8, 2023 •

edited

Loading

dmitripivkine Dec 11, 2023 •

edited

Loading

dmitripivkine Dec 11, 2023 •

edited

Loading