Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* #16820

dmitripivkine · 2023-03-03T17:41:01Z

This is non-functional change: Remove usage of J9VM_MODRON_SCAVENGER_CACHE_* defined names, use new OMR_COPYSCAN_CACHE_* defined names instead.

Depends on eclipse/omr#6921.

amicic · 2023-03-03T18:06:29Z

runtime/gc_vlhgc/CopyScanCacheVLHGC.hpp

@@ -52,7 +52,7 @@ class MM_CopyScanCacheVLHGC : public MM_CopyScanCache
 	UDATA _objectSize;   /**< sum of objects sizes copied to this copy cache */
 	U_64 _lowerAgeBound; /**< lowest possible age of any object in this copy cache */
 	U_64 _upperAgeBound; /**< highest possible age of any object in this copy cache */
-	UDATA _arraySplitIndex; /**< The index within the array in scanCurrent to start scanning from (meaningful is J9VM_MODRON_SCAVENGER_CACHE_TYPE_SPLIT_ARRAY is set) */ 
+	UDATA _arraySplitIndex; /**< The index within the array in scanCurrent to start scanning from (meaningful is OMR_SCAVENGER_CACHE_TYPE_SPLIT_ARRAY is set) */


While touching this, I recommend to replace UDATA with uintptr_t, and similar ones, too. Base MM_CopyScanCache already uses uintptr_t.

same for other smaller files you touched

amicic · 2023-03-03T18:19:27Z

runtime/gc_vlhgc/CopyScanCacheListVLHGC.cpp

@@ -143,7 +143,7 @@ MM_CopyScanCacheListVLHGC::removeAllHeapAllocatedChunks(MM_EnvironmentVLHGC *env
 			MM_CopyScanCacheVLHGC *cache = cacheList->_cacheHead;

 			while(cache != NULL) {


try to fix some formatting too, in nearby code - missing spacing between 'while' and '('

amicic · 2023-03-03T18:53:44Z

runtime/gc_vlhgc/CopyScanCacheChunkVLHGCInHeap.cpp

@@ -31,9 +31,9 @@
 UDATA


missed this one

amicic · 2023-03-03T18:54:37Z

runtime/gc_vlhgc/CopyForwardScheme.cpp

-	if(0 == (cache->flags & J9VM_MODRON_SCAVENGER_CACHE_TYPE_COPY)) {
-		if(0 == (cache->flags & J9VM_MODRON_SCAVENGER_CACHE_TYPE_CLEARED)) {
+	if(0 == (cache->flags & OMR_SCAVENGER_CACHE_TYPE_COPY)) {
+		if(0 == (cache->flags & OMR_SCAVENGER_CACHE_TYPE_CLEARED)) {


missing space after 'if'

Also, it should use OMR_ARE_NO_BITS_SET().

Also, it should use OMR_ARE_NO_BITS_SET().

Bit functions like this are not used in GC code. I am not sure we should start to use them now

They are even used in this file, see line 2622, for example.

I already stated our current view on when to use these macros in a previous comment (#16820 (comment)) and you said you'd respect that.

The example you refer to now fits within VM flags, while this PR deals with a GC specific flag.

Being unified within a file is also important, but of a secondary priority (relative to the criteria I mentioned)

Still disappointed, but I did say that.

amicic · 2023-03-03T19:02:00Z

runtime/gc_vlhgc/CopyForwardScheme.cpp

@@ -2906,7 +2906,7 @@ MM_CopyForwardScheme::bestCacheForScanning(MM_CopyScanCacheVLHGC* copyCache, MM_
 	if (!copyCache->isScanWorkAvailable()) {
 		return false;
 	}
-	if (!((*scanCache)->flags & J9VM_MODRON_SCAVENGER_CACHE_TYPE_COPY)) {
+	if (!((*scanCache)->flags & OMR_SCAVENGER_CACHE_TYPE_COPY)) {


this is probably not a style that conforms with coding standard.
we should compare it with 0 (which Scavenger.cpp) uses, or even use some of bitwise macros (common in core VM code, but no so in GC)

Tests like this should use OMR_ARE_NO_BITS_SET().

amicic · 2023-03-03T19:02:32Z

runtime/gc_vlhgc/CopyForwardScheme.cpp

@@ -3266,7 +3266,7 @@ MM_CopyForwardScheme::incrementalScanCacheBySlot(MM_EnvironmentVLHGC *env)
 					 * if deferred cache is occupied, then queue current scan cache on scan list
 					 */
 					scanCache->clearCurrentlyBeingScanned();	
-					if (!(scanCache->flags & J9VM_MODRON_SCAVENGER_CACHE_TYPE_COPY)) {
+					if (!(scanCache->flags & OMR_SCAVENGER_CACHE_TYPE_COPY)) {


another one that should compare with 0, rather than treating as a bool

amicic · 2023-03-03T19:33:05Z

runtime/gc_vlhgc/CopyForwardScheme.cpp

 		return false;
 	}

 	memset((void *)_reservedRegionList, 0, sizeof(MM_ReservedRegionListHeader) * _compactGroupMaxCount);
-	for(UDATA index = 0; index < _compactGroupMaxCount; index++) {
+	for (UDATA index = 0; index < _compactGroupMaxCount; index++) {


well, now that you fixed spacing in this huge file (which I did not expect), you could go and fix UDATA, too :)

I afraid such update might hide original change, Spaces can be filtered out from view. I suggest to do update of types in separate PR

dmitripivkine · 2023-03-08T15:33:57Z

Depends eclipse/omr#6921

dmitripivkine · 2023-03-08T16:12:31Z

@keithc-ca Would you please review DDR part? These constants haven't been used in DDR code. With removing them everywhere there is no need to keep them around. The old set of constants has been replaced to a new one. I think there is no need to add new set to DDR.

keithc-ca · 2023-03-08T18:08:00Z

The DDR changes look fine to me.

amicic · 2023-03-13T14:00:42Z

need to squash the commits

amicic · 2023-03-13T14:01:00Z

The DDR changes look fine to me.

@keithc-ca, could you formally approve?

runtime/gc_vlhgc/CopyForwardScheme.cpp

keithc-ca · 2023-03-13T14:24:07Z

runtime/gc_vlhgc/CopyForwardScheme.cpp

@@ -4371,15 +4371,15 @@ MM_CopyForwardScheme::scanRoots(MM_EnvironmentVLHGC* env)

 			while (NULL != (classLoader = classLoaderIterator.nextSlot())) {
 				if (0 == (classLoader->gcFlags & J9_GC_CLASS_LOADER_DEAD)) {
-					if(J9_ARE_ANY_BITS_SET(classLoader->flags, J9CLASSLOADER_ANON_CLASS_LOADER)) {
+					if (J9_ARE_ANY_BITS_SET(classLoader->flags, J9CLASSLOADER_ANON_CLASS_LOADER)) {


Since this code already uses J9_ARE_ANY_BITS_SET(), it should do so consistently (although I think the OMR equivalents should be preferred). The test here and similar ones on lines 1771,1772, 1786, 2909, 3269, etc., for example should be updated.

runtime/gc_vlhgc/CopyScanCacheVLHGC.hpp

runtime/gc_vlhgc/CopyScanCacheChunkVLHGCInHeap.hpp

amicic · 2023-03-15T20:34:02Z

@keithc-ca While we, in GC code, are not completely consistent with use of macros of *_BITS_SET(), we tend not to use them.

Exceptions for the most part is code that deals with bits that are imposed by core VM structs (public/private flags in J9VMThread, flags in J9Class, etc) while still in GC code, although there are some other minor exceptions.

To stay consistent with this state:

I do not want to introduce these macros in pure GC code, that does not Interact with VM structures, what all of OMR code is (although there are actually a few usages of these macros), and some part of J9 GC code is, too. In fact, in future, I may ask them to be removed, if fit within this category.
For the code that deals with VM specific flags, but in GC specific files, we should continue to use those macros to stay consistent with the rest of core VM code. I don't have a preference if they should be J9_ or OMR_ variant.

keithc-ca · 2023-03-15T21:20:27Z

In my opinion use of those macros has nothing to do with any particular data structure: they're really only about managing bit sets or masks. I'm disappointed you don't agree that their use would improve GC code, but I will respect your position.

This is non-functional change: Remove usage of J9VM_MODRON_SCAVENGER_CACHE_* defined names, use neutral OMR_COPYSCAN_CACHE_* defined names instead. Usage of Copy Scan Caches is not limited to Scavenger and can be used by other Collectors, Balance for instanse. Also replace variables types according standards for modified files and fix formatting Signed-off-by: Dmitri Pivkine <Dmitri_Pivkine@ca.ibm.com>

amicic · 2023-04-17T18:01:53Z

jenkins test sanity aix,win jdk11

dmitripivkine added the comp:gc label Mar 3, 2023

dmitripivkine requested a review from amicic March 3, 2023 17:41

amicic reviewed Mar 3, 2023

View reviewed changes

dmitripivkine force-pushed the master branch from 08d8564 to 15d8fa2 Compare March 3, 2023 18:51

amicic reviewed Mar 3, 2023

View reviewed changes

runtime/gc_vlhgc/CopyScanCacheChunkVLHGCInHeap.cpp Outdated

@@ -31,9 +31,9 @@

UDATA

Copy link

Contributor

amicic Mar 3, 2023

Choose a reason for hiding this comment

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

missed this one

amicic reviewed Mar 3, 2023

View reviewed changes

dmitripivkine force-pushed the master branch from 356659c to eb91f5a Compare March 3, 2023 19:29

amicic reviewed Mar 3, 2023

View reviewed changes

dmitripivkine force-pushed the master branch from d19a73b to 2caa7ca Compare March 8, 2023 14:46

dmitripivkine changed the title Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_SCAVENGER_CACHE_* Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* Mar 8, 2023

dmitripivkine changed the title Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* WIP:Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* Mar 8, 2023

dmitripivkine added the depends:omr label Mar 8, 2023

dmitripivkine changed the title WIP:Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* Mar 8, 2023

dmitripivkine force-pushed the master branch from 2caa7ca to 5cc16ef Compare March 8, 2023 16:43

amicic approved these changes Mar 13, 2023

View reviewed changes

keithc-ca requested changes Mar 13, 2023

View reviewed changes

dmitripivkine force-pushed the master branch from 6064158 to d36ff7e Compare March 13, 2023 15:40

keithc-ca reviewed Mar 13, 2023

View reviewed changes

runtime/gc_vlhgc/CopyScanCacheChunkVLHGCInHeap.hpp Show resolved Hide resolved

dmitripivkine force-pushed the master branch from f1b76eb to 6276faf Compare April 12, 2023 15:00

dmitripivkine force-pushed the master branch from 6276faf to f1ecdfb Compare April 12, 2023 15:02

keithc-ca approved these changes Apr 17, 2023

View reviewed changes

amicic merged commit 8c3b2e2 into eclipse-openj9:master Apr 24, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* #16820

Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* #16820

dmitripivkine commented Mar 3, 2023 •

edited by keithc-ca

Loading

amicic Mar 3, 2023

amicic Mar 3, 2023

amicic Mar 3, 2023

amicic Mar 3, 2023

amicic Mar 3, 2023

keithc-ca Mar 8, 2023

dmitripivkine Mar 8, 2023

keithc-ca Apr 14, 2023

amicic Apr 17, 2023 •

edited

Loading

keithc-ca Apr 17, 2023

amicic Mar 3, 2023

keithc-ca Mar 8, 2023 •

edited

Loading

amicic Mar 3, 2023

amicic Mar 3, 2023

dmitripivkine Mar 3, 2023

dmitripivkine commented Mar 8, 2023

dmitripivkine commented Mar 8, 2023

keithc-ca commented Mar 8, 2023

amicic commented Mar 13, 2023

amicic commented Mar 13, 2023

keithc-ca Mar 13, 2023

amicic commented Mar 15, 2023

keithc-ca commented Mar 15, 2023

amicic commented Apr 17, 2023

		@@ -143,7 +143,7 @@ MM_CopyScanCacheListVLHGC::removeAllHeapAllocatedChunks(MM_EnvironmentVLHGC *env
		MM_CopyScanCacheVLHGC *cache = cacheList->_cacheHead;

		while(cache != NULL) {

Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* #16820

Replace J9VM_MODRON_SCAVENGER_CACHE_* to OMR_COPYSCAN_CACHE_* #16820

Conversation

dmitripivkine commented Mar 3, 2023 • edited by keithc-ca 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 Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca Mar 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

Choose a reason for hiding this comment

dmitripivkine commented Mar 8, 2023

dmitripivkine commented Mar 8, 2023

keithc-ca commented Mar 8, 2023

amicic commented Mar 13, 2023

amicic commented Mar 13, 2023

Choose a reason for hiding this comment

amicic commented Mar 15, 2023

keithc-ca commented Mar 15, 2023

amicic commented Apr 17, 2023

dmitripivkine commented Mar 3, 2023 •

edited by keithc-ca

Loading

amicic Apr 17, 2023 •

edited

Loading

keithc-ca Mar 8, 2023 •

edited

Loading