Sampling allocation bytes precisely without compromising the performance #5260

LinHu2016 · 2020-05-29T19:02:26Z

in order to sampling heap allocation bytes precisely without
compromising the performance, we have the below changes.

Handle instrumentableAllocateHook and
VM_OBJECT_ALLOCATE_WITHIN_THRESHOLD is still via disabling inline
allocation
Handle smapling for tracepoint is still during out of line allocation
Handle smapling for JEP331 is via setTLHSamplingTop(size)

Using fake Heap Top instead of fake Heap Alloc for disabling inline
allocation (realHeapAlloc-->realHeapTop,
set/getRealAlloc()-->set/getRealTop(), getRealSize(), getUsedSize())
Using fake Heap Top to force out of line allocation at sampling thresold
for sampling heap allocation (setTLHSamplingTop()/resetTLHSamplingTop())
setTLHSamplingTop(size) are only called in the below 3 cases
1, sampling threshold has been changed via GC-VM api
j9gc_set_allocation_sampling_interval()
2, TLH is refreshed
3, after sampling is done

Counting trace allocation byte includes allocation bytes inside TLH
Cache before flushing(_stats.bytesAllocated(true),
stats->_tlhAllocatedUsed, )
Handle traceAllocationByte for Health
Center(_oolTraceAllocationBytesForTracepoint,
oolObjectSamplingBytesGranularityForTracepoint) and traceAllocationByte
for JEP331(_traceAllocationBytesForHook,
objectSamplingBytesGranularityForHook) independently

depends on eclipse-openj9/openj9#9767
Signed-off-by: Lin Hu linhu@ca.ibm.com

LinHu2016 · 2020-05-29T19:06:15Z

@amicic @dmitripivkine please review changes, thanks

amicic · 2020-06-01T15:01:00Z

gc/base/EnvironmentBase.hpp

@@ -664,8 +693,9 @@ class MM_EnvironmentBase : public MM_BaseVirtual
 		,_currentTask(NULL)
 		,_slaveThreadCpuTimeNanos(0)
 		,_freeEntrySizeClassStats()
-		,_oolTraceAllocationBytes(0)
-		,_traceAllocationBytes(0)
+		,_oolTraceAllocationBytesForTracepoint(0)


Now that I realized that Hook and Tracepoint reside on OpenJ9 side, I'm wondering if it makes sense to expand these names with ... ForTracepoint/ForHook. They make little sense to someone who looks at OMR code and is not aware of OpenJ9 code. @dmitripivkine what do you think?

I agree. "ForTracepoint/ForHook" does not explain what these variables are used for without understanding how "Tracepoint" or "Hook" are used for

GCExtensionsBase.hpp:
oolObjectSamplingBytesGranularityForTracepoint --> oolObjectSamplingBytesGranularity
objectSamplingBytesGranularityForHook --> objectSamplingBytesGranularity
EnvironmentBase.hpp:
_oolTraceAllocationBytesForTracepoint --> _oolTraceAllocationBytes
_traceAllocationBytesForHook --> _traceAllocationBytes

Yeah, let's restore the naming back. Sorry, I know I proposed the change to start with, since I did not like one just being OOL and the other not. (and I still don't like that separation, since both of them end up doing OOL)

Perhaps one should be coarseSamplingAllocatedBytes/coarseSamplingByteGranularity (what OOL is now) and the other one is fineSamplingAllocatedByte/fineSamplingByteGranularity , but unless Dmitri strongly agrees that OOL differentiation is confusing, leave it as you suggested.

I am ok to reuse old names. At least it simplifies merge for this change.

amicic · 2020-06-01T15:08:29Z

gc/base/TLHAllocationSupport.hpp

 		}
 	};
+
+	/* @return the number of bytes, which are available for allocation in the TLH */
+	MMINLINE uintptr_t getRealSize() { return (uintptr_t)getRealTop() - (uintptr_t)getAlloc(); };


would it make more sense to call it Unused or Remaining or Available?

@dmitripivkine again, please your opinion. 'Real' is still ok, though.

I like Remaining, it explains meaning better. I am not strong on this

please update this one as per Dmitri's suggestion

amicic · 2020-06-01T15:19:19Z

gc/base/TLHAllocationInterface.cpp

-		env->_traceAllocationBytes += sizeInBytesAllocated;
-
-		if (!allocDescription->isCompletedFromTlh()) {
+	if (NULL != result && !allocDescription->isCompletedFromTlh()) {


Suggested change

if (NULL != result && !allocDescription->isCompletedFromTlh()) {

if ((NULL != result) && !allocDescription->isCompletedFromTlh()) {

amicic · 2020-06-01T15:29:43Z

gc/base/TLHAllocationSupport.cpp


 	/* Try to cache the current TLH */
-	if (NULL != getRealAlloc() && getSize() >= tlhMinimumSize) {
+	if (NULL != getRealTop() && getRealSize() >= tlhMinimumSize) {


Suggested change

if (NULL != getRealTop() && getRealSize() >= tlhMinimumSize) {

if ((NULL != getRealTop()) && (getRealSize() >= tlhMinimumSize)) {

amicic · 2020-06-01T15:45:29Z

gc/base/TLHAllocationInterface.cpp


+	uintptr_t sizeInBytesAllocated = (_stats.bytesAllocated(true) - _bytesAllocatedBase);
+	env->_oolTraceAllocationBytesForTracepoint += sizeInBytesAllocated; /* Increment by bytes allocated for Trace */
+	env->_traceAllocationBytesForHook += sizeInBytesAllocated; /* Increment by bytes allocated for Hook */


This is critical part to understand. You are saying that
a) if we don't force any extra OOL allocations (due to sampling rate being < TLH size), that bytesAllocated(true) is equal to what bytesAllocated() used to be. Or in other words, that _tlhAllocatedFresh - _tlhDiscardedBytes == _tlhAllocatedUsed? Did you verify that - is it exactly true or just approximately true?
b), if we do force extra OOL allocations (due to sampling rate being < TLH size), that SUM of all bytesAllocated(true) will still be equal to what bytesAllocated() used to be.

I think that there might be a difference in behavior since _tlhAllocatedFresh was taken after TLH was refreshed (hence it was for next TLH, what seems like not completely correct), and _tlhDiscardedBytes was taken before TLH was refreshed (for previous TLH). On the other side _tlhAllocatedUsed is correctly for previous TLH.
In a long run, when things are averaged _tlhAllocatedFresh - _tlhDiscardedBytes == _tlhAllocatedUsed might hold true, but strictly speaking, I suspect it does not hold true each time this code is executed

Should we do special handling in case of dual TLH support? It was not relevant before (TLH allocation was disabled essentially so did not matter which one). But looks like it becomes relevant now. Is it correct? The simple solution can be disabling of non-zeroed TLH in case of instrumentation
Correction: Dual TLH is handled in OpenJ9 part, so we can discuss it there

_tlhAllocatedUsed is the amount of memory has been flushed, "_tlhAllocatedFresh - _tlhDiscardedBytes == _tlhAllocatedUsed" is true when the TLH is just right before refreshing, after refreshing _tlhAllocatedFresh would be added with new TLH size.
bytesAllocated(true) try to return the size for all of allocated bytes excludes any bytes in current TLH. naming of _tlhAllocatedUsed might be too confuse.

amicic · 2020-06-01T16:15:50Z

gc/stats/AllocationStats.hpp

@@ -59,14 +60,22 @@ class MM_AllocationStats : public MM_Base

 #if defined(OMR_GC_THREAD_LOCAL_HEAP)
 	uintptr_t tlhBytesAllocated() { return _tlhAllocatedFresh - _tlhDiscardedBytes; }
+	uintptr_t tlhBytesAllocatedUsed() { return _tlhAllocatedUsed; }


Put some comment here. For example (if you think this is true):

For tlhBytesAllocated, it's allocated bytes since last TLH refresh.
For tlhBytesAllocatedUsed, it's allocated bytes since last OOL allocation from this TLH (which could be forced before TLH refresh multiple times due to allocation sampling).

bytesAllocated(true) try to return the size for all of allocated bytes excludes any bytes in current TLH.
tlhBytesAllocated() also includes the current TLH size, since tlhBytesAllocated() has been used for multiple places, I don't want to change the behavior except for traceAllocationBytes.

amicic · 2020-06-01T16:44:15Z

gc/base/TLHAllocationSupport.cpp

+		uintptr_t samplingBytesGranularity = UDATA_MAX;
+		if (!env->needDisableInlineAllocation() && (UDATA_MAX != (samplingBytesGranularity = env->getExtensions()->objectSamplingBytesGranularityForHook))) {


Suggested change

uintptr_t samplingBytesGranularity = UDATA_MAX;

if (!env->needDisableInlineAllocation() && (UDATA_MAX != (samplingBytesGranularity = env->getExtensions()->objectSamplingBytesGranularityForHook))) {

uintptr_t samplingBytesGranularity = extensions->objectSamplingBytesGranularityForHook;

if (!env->needDisableInlineAllocation() && (UDATA_MAX != samplingBytesGranularity)) {

amicic · 2020-06-01T16:54:00Z

gc/base/EnvironmentBase.hpp

+	 */
+	bool needDisableInlineAllocation() {
+		return (getExtensions()->fvtest_disableInlineAllocation || getExtensions()->instrumentableAllocateHookEnabled || getExtensions()->disableInlineCacheForAllocationThreshold);
+	}


I'd move this helper to Extensions. From a quick look, at all call sites, Extensions is already available as a local.

amicic · 2020-06-02T14:29:06Z

gc/base/EnvironmentBase.hpp

-	uintptr_t _traceAllocationBytes;  /**< Tracks the bytes allocated since the last object trace include ool and allocation is completed from TLH */
+	uintptr_t _oolTraceAllocationBytes; /**< Tracks the bytes allocated since the last ool object trace for Tracepoint */
+	uintptr_t _traceAllocationBytes;  /**< Tracks the bytes allocated since the last ool object trace for Hooks ( only record ool allocation bytes and flushed TLH size) */
+	uintptr_t _traceAllocationAdjustmentBytes; /**< keep the bytes of times of sampling thresold for last object trace(include allocation byttes inside TLH) */


how about _traceAllocationBytesCurrentTLH?

amicic · 2020-06-02T14:35:58Z

gc/base/GCExtensionsBase.hpp

@@ -293,6 +293,8 @@ class MM_GCExtensionsBase : public MM_BaseVirtual {
 	bool doOutOfLineAllocationTrace;
 	bool doFrequentObjectAllocationSampling; /**< Whether to track object allocations*/
 	uintptr_t oolObjectSamplingBytesGranularity; /**< How often (in bytes) we do an ool allocation trace */
+	uintptr_t objectSamplingBytesGranularity; /**< How often (in bytes) we do an allocation trace (for triggering J9HOOK_MM_OBJECT_ALLOCATION_SAMPLING) */
+


you should not really be referring to J9 specific hook. Perhaps you can say:
How often (in bytes) we do allocation sampling as tracked by per thread's local _traceAllocationBytes.
And similarly adjust the OOL one.

amicic · 2020-06-02T14:40:52Z

gc/stats/AllocationStats.hpp

+	/* return bytesAllocated includes new refreshed TLH, if used == false(default)
+	 * return bytesAllocated (but does not include new refreshed TLH), if used == true. 
+	 */
+	uintptr_t bytesAllocated(bool used = false) {


The flag name should more resemble to your comment (and invert the logic, if needed). Something like include(Just?)RefreshedTLH ?

amicic · 2020-06-02T16:27:18Z

gc/stats/AllocationStats.hpp

@@ -37,6 +37,7 @@ class MM_AllocationStats : public MM_Base
 	uintptr_t _tlhRefreshCountFresh; /**< Number of refreshes where fresh memory was allocated. */
 	uintptr_t _tlhRefreshCountReused; /**< Number of refreshes where TLHs were reused. */
 	uintptr_t _tlhAllocatedFresh; /**< The amount of memory allocated fresh out of the heap. */
+	uintptr_t _tlhAllocatedUsed; /**< The amount of memory has been flushed */


to align comment with the variable name: amount of used memory of already flushed TLHs

amicic · 2020-06-02T18:27:34Z

gc/base/TLHAllocationInterface.cpp


+	uintptr_t sizeInBytesAllocated = (_stats.bytesAllocated(false) - _bytesAllocatedBase);
+	env->_oolTraceAllocationBytes += sizeInBytesAllocated; /* Increment by bytes allocated for Trace */
+	env->_traceAllocationBytes += sizeInBytesAllocated; /* Increment by bytes allocated for Hook */


Remove the comments. They reference Trace and Hook, which is out of context for OMR. And they don't even comply with coding standards.

amicic · 2020-06-02T18:37:02Z

@rwy0717 or @youngar please, proceed with further review

in order to sampling heap allocation bytes precisely without compromising the performance, we have the below changes. Handle instrumentableAllocateHook and VM_OBJECT_ALLOCATE_WITHIN_THRESHOLD is still via disabling inline allocation Handle smapling for tracepoint is still during out of line allocation Handle smapling for JEP331 is via setTLHSamplingTop(size) Using fake Heap Top instead of fake Heap Alloc for disabling inline allocation (realHeapAlloc-->realHeapTop, set/getRealAlloc()-->set/getRealTop(), getRealSize(), getUsedSize()) Using fake Heap Top to force out of line allocation at sampling thresold for sampling heap allocation (setTLHSamplingTop()/resetTLHSamplingTop()) setTLHSamplingTop(size) are only called in the below 3 cases 1, sampling threshold has been changed via GC-VM api j9gc_set_allocation_sampling_interval() 2, TLH is refreshed 3, after sampling is done Counting trace allocation byte includes allocation bytes inside TLH Cache before flushing(_stats.bytesAllocated(true), stats->_tlhAllocatedUsed, ) Handle traceAllocationByte for Health Center(_oolTraceAllocationBytesForTracepoint, oolObjectSamplingBytesGranularityForTracepoint) and traceAllocationByte for JEP331(_traceAllocationBytesForHook, objectSamplingBytesGranularityForHook) independently Signed-off-by: Lin Hu <linhu@ca.ibm.com>

rwy7 · 2020-06-03T19:16:41Z

@genie-omr build all

LinHu2016 requested review from charliegracie, mstoodle, rwy7 and youngar as code owners May 29, 2020 19:02

LinHu2016 mentioned this pull request May 29, 2020

Sampling allocation bytes precisely without compromising the performance eclipse-openj9/openj9#9745

Merged

github-actions bot added the comp:gc label May 29, 2020

amicic reviewed Jun 1, 2020

View reviewed changes

LinHu2016 force-pushed the JEP331_update branch 2 times, most recently from 6bf16c2 to 7335914 Compare June 2, 2020 02:45

amicic reviewed Jun 2, 2020

View reviewed changes

LinHu2016 force-pushed the JEP331_update branch 2 times, most recently from 52e91cf to 411fe76 Compare June 2, 2020 16:08

amicic reviewed Jun 2, 2020

View reviewed changes

LinHu2016 force-pushed the JEP331_update branch from 411fe76 to 4b47308 Compare June 2, 2020 17:45

amicic reviewed Jun 2, 2020

View reviewed changes

amicic approved these changes Jun 2, 2020

View reviewed changes

dmitripivkine approved these changes Jun 2, 2020

View reviewed changes

LinHu2016 force-pushed the JEP331_update branch from 4b47308 to 93245aa Compare June 2, 2020 19:02

rwy7 self-assigned this Jun 3, 2020

rwy7 merged commit c2b3c97 into eclipse-omr:master Jun 3, 2020

amicic mentioned this pull request Jun 27, 2020

Support tracing allocation bytes eclipse-openj9/openj9#9254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sampling allocation bytes precisely without compromising the performance #5260

Sampling allocation bytes precisely without compromising the performance #5260

LinHu2016 commented May 29, 2020 •

edited

Loading

LinHu2016 commented May 29, 2020

amicic Jun 1, 2020 •

edited

Loading

dmitripivkine Jun 1, 2020

LinHu2016 Jun 1, 2020 •

edited

Loading

amicic Jun 1, 2020

dmitripivkine Jun 1, 2020

amicic Jun 1, 2020 •

edited

Loading

amicic Jun 1, 2020

dmitripivkine Jun 1, 2020

amicic Jun 2, 2020

amicic Jun 1, 2020

amicic Jun 1, 2020

amicic Jun 1, 2020 •

edited

Loading

amicic Jun 1, 2020 •

edited

Loading

dmitripivkine Jun 1, 2020 •

edited

Loading

LinHu2016 Jun 1, 2020

amicic Jun 1, 2020

LinHu2016 Jun 1, 2020

amicic Jun 1, 2020

amicic Jun 1, 2020

amicic Jun 2, 2020

amicic Jun 2, 2020

amicic Jun 2, 2020 •

edited

Loading

amicic Jun 2, 2020

amicic Jun 2, 2020

amicic commented Jun 2, 2020

rwy7 commented Jun 3, 2020

	if (NULL != result && !allocDescription->isCompletedFromTlh()) {
	if ((NULL != result) && !allocDescription->isCompletedFromTlh()) {

	if (NULL != getRealTop() && getRealSize() >= tlhMinimumSize) {
	if ((NULL != getRealTop()) && (getRealSize() >= tlhMinimumSize)) {

		uintptr_t samplingBytesGranularity = UDATA_MAX;
		if (!env->needDisableInlineAllocation() && (UDATA_MAX != (samplingBytesGranularity = env->getExtensions()->objectSamplingBytesGranularityForHook))) {

Sampling allocation bytes precisely without compromising the performance #5260

Sampling allocation bytes precisely without compromising the performance #5260

Conversation

LinHu2016 commented May 29, 2020 • edited Loading

LinHu2016 commented May 29, 2020

amicic Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinHu2016 Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Jun 1, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

amicic Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

dmitripivkine Jun 1, 2020 • 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

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 Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic commented Jun 2, 2020

rwy7 commented Jun 3, 2020

LinHu2016 commented May 29, 2020 •

edited

Loading

amicic Jun 1, 2020 •

edited

Loading

LinHu2016 Jun 1, 2020 •

edited

Loading

amicic Jun 1, 2020 •

edited

Loading

amicic Jun 1, 2020 •

edited

Loading

amicic Jun 1, 2020 •

edited

Loading

dmitripivkine Jun 1, 2020 •

edited

Loading

amicic Jun 2, 2020 •

edited

Loading