Fix discrepancy in GC Extensions Base functions #6715

dmitripivkine · 2022-09-20T17:43:03Z

There is a number of functions in the GC Extensions Base defined under
the OMR_GC_MODRON_SCAVENGER flag. However, there are many places where
these functions are called unconditionally without checking this flag.
This flag is always enabled in our compilation environment, which
allows the code to compile. If it is disabled, the code fails to
compile. The solution is to always define these functions and wrap
Scavenger related content inside them.

Signed-off-by: Dmitri Pivkine [email protected]

dmitripivkine · 2022-09-20T17:49:18Z

gc/base/standard/ConfigurationGenerational.cpp

@@ -217,7 +217,6 @@ MM_ConfigurationGenerational::createHeapWithManager(MM_EnvironmentBase *env, UDA
 	MM_GCExtensionsBase *extensions = env->getExtensions();
 	MM_Heap *heap = NULL;

-#if defined(OMR_GC_MODRON_SCAVENGER)


This one was removed because it is done at the beginning of the file

amicic · 2022-09-20T18:18:26Z

gc/base/GCExtensionsBase.hpp

+#endif /* OMR_GC_MODRON_SCAVENGER */
+	}
+
+	MMINLINE bool isRememberedSetInOverflowState()


Now that these 3 RS methods are exposed outside of Scavenger compile fla, it would nice that method names are more explicit what kind of RS we are talking (since there is more then just Scavenger RS in our code base).
So, I recommend we expand these names to:
isScavengerRememberedSetInOverflowState
and similarly for 2 others related.
Since this is being used by dependent projects, you'll have keep the exisiting APIs, till depended projects get updated.

new functions have added

dmitripivkine · 2022-09-20T20:12:40Z

I did not realize I can change OMR code to use new functions now without waiting for cleanup PR. Adding more code

dmitripivkine · 2022-09-20T23:36:30Z

@0xdaryl @babsingh Would you please proceed?
I believe failures in Mac OS build are unrelated

babsingh · 2022-09-21T00:23:42Z

gc/base/GCExtensionsBase.hpp

 		*start = _guaranteedNurseryStart;
 		*end = _guaranteedNurseryEnd;
+#elif /* OMR_GC_MODRON_SCAVENGER */


All#elifs will need to be replaced with #else or #elif defined(MACRO_NAME). Otherwise, the code may not work as expected.

Suggested change

#elif /* OMR_GC_MODRON_SCAVENGER */

#else /* OMR_GC_MODRON_SCAVENGER */

babsingh · 2022-09-21T00:30:40Z

Does this PR completely resolve the issue described in #6702 (comment) and allow OMR to be built without OMR_GC_MODRON_SCAVENGER?

babsingh · 2022-09-21T02:12:21Z

Locally built without OMR_GC_MODRON_SCAVENGER. Got the following errors:

/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp:1123:36: error: #elif with no expression
 #elif /* OMR_GC_MODRON_SCAVENGER */
                                    ^
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp:1133:36: error: #elif with no expression
 #elif /* OMR_GC_MODRON_SCAVENGER */
                                    ^
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp:1156:36: error: #elif with no expression
 #elif /* OMR_GC_MODRON_SCAVENGER */
                                    ^
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp:1179:36: error: #elif with no expression
 #elif /* OMR_GC_MODRON_SCAVENGER */
                                    ^
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp: In member function ‘bool MM_GCExtensionsBase::isRememberedSetInOverflowState()’:
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp:1136:2: error: no return statement in function returning non-void [-Werror=return-type]
  }
  ^
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp: In member function ‘bool MM_GCExtensionsBase::isScavengerRememberedSetInOverflowState()’:
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp:1159:2: error: no return statement in function returning non-void [-Werror=return-type]
  }
  ^
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp: In member function ‘bool MM_GCExtensionsBase::isScavengerBackOutFlagRaised()’:
/root/openj9-openjdk-jdk17/omr/gc/base/GCExtensionsBase.hpp:1182:2: error: no return statement in function returning non-void [-Werror=return-type]
  }
  ^

dmitripivkine · 2022-09-21T13:46:48Z

Does this PR completely resolve the issue described in #6702 (comment) and allow OMR to be built without OMR_GC_MODRON_SCAVENGER?

The goal of this PR is cleanup usage of scavenger related functions in MM_GCExtensionsBase.
I have not check complete rebuild without OMR_GC_MODRON_SCAVENGER
If you have setup for local standalone OMR compilation would you please try it?
I thought there is more time consuming work to do (so prepared this partial fix) but maybe you are right and we can complete it now.

babsingh · 2022-09-21T14:06:08Z

If you have setup for local standalone OMR compilation would you please try it?

Setup is pretty easy. If an OpenJ9 or OMR setup already exists, then you only need the following cmds to build OMR standalone:

cd $OMR_REPO
mkdir build
cd build
cmake ..             // Configure will build without OMR_GC_MODRON_SCAVENGER
make -j8             // Compile
ctest -V             // Test

If a setup does not exist, then the following dependencies need to be installed before running the above cmds: https://github.com/eclipse/omr/blob/f27ffa9b7e747c1f316240c5ec2335543b48aec6/buildenv/docker/x86_64/ubuntu20/Dockerfile#L28-L48

Configure alternate:

// Configure will build with OMR_GC_MODRON_SCAVENGER
cmake -Wdev -C ../cmake/caches/Travis.cmake ..

babsingh · 2022-09-27T21:17:01Z

With #6525 merged, we can disable OMR_GC_MODRON_SCAVENGER and dependent/related flags in PR builds to test these changes.

Example:
jenkins build xlinux(cmake:'-DOMR_GC_MODRON_SCAVENGER=OFF -DOMR_GC_MODRON_CONCURRENT_MARK=OFF -DOMR_GC_SEGREGATED_HEAP=OFF')

dmitripivkine · 2022-09-27T21:34:01Z

@babsingh Would you please review again? OMR compiles with OMR_GC_MODRON_SCAVENGER set OFF. Surprisingly, internal testing passed too (with exception of porttest failed in original build too)

babsingh · 2022-09-28T04:10:58Z

jenkins build all(cmake:'-DOMR_GC_MODRON_SCAVENGER=OFF')

babsingh · 2022-09-28T04:28:33Z

jenkins build arm(cmake:'--disable-OMR_GC_MODRON_SCAVENGER'),aarch64(cmake:'--disable-OMR_GC_MODRON_SCAVENGER'),zos(cmake:'-DOMR_GC_MODRON_SCAVENGER=OFF')

babsingh · 2022-09-28T04:33:49Z

jenkins build arm(cmake:'EXTRA_CONFIGURE_ARGS=--disable-OMR_GC_MODRON_SCAVENGER'),aarch64(cmake:'EXTRA_CONFIGURE_ARGS=--disable-OMR_GC_MODRON_SCAVENGER'),zos(cmake:'-DOMR_GC_MODRON_SCAVENGER=OFF')

babsingh · 2022-09-28T04:38:19Z

jenkins build arm(cmake:'EXTRA_CONFIGURE_ARGS=--disable-OMR_GC_MODRON_SCAVENGER')

babsingh · 2022-09-28T13:52:20Z

jenkins build zos(cmake:'-DOMR_GC_MODRON_SCAVENGER=OFF')

babsingh

functionally changes look good. minor formatting nitpicks.

gc/base/GCExtensionsBase.hpp

babsingh · 2022-09-28T15:58:23Z

Suggested rephrase of the commit message:

Fix discrepancy in GC Extensions Base functions

There are a number of functions in the GC Extensions Base defined under
the OMR_GC_MODRON_SCAVENGER flag. However, there are many places where
these functions are called unconditionally without checking this flag.
This flag is always enabled in our compilation environment, which
allows the code to compile. If it is disabled, the code fails to
compile. The solution is to always define these functions and wrap
Scavenger related content inside them.

Signed-off-by: Dmitri Pivkine <[email protected]>

There is a number of functions in the GC Extensions Base defined under the OMR_GC_MODRON_SCAVENGER flag. However, there are many places where these functions are called unconditionally without checking this flag. This flag is always enabled in our compilation environment, which allows the code to compile. If it is disabled, the code fails to compile. The solution is to always define these functions and wrap Scavenger related content inside them. Signed-off-by: Dmitri Pivkine <[email protected]>

dmitripivkine · 2022-09-28T17:47:02Z

@babsingh ready for review

babsingh · 2022-09-29T12:59:45Z

jenkins build all(cmake:'-DOMR_GC_MODRON_SCAVENGER=OFF')

babsingh · 2022-09-29T12:59:55Z

jenkins build arm(cmake:'EXTRA_CONFIGURE_ARGS=--disable-OMR_GC_MODRON_SCAVENGER')

babsingh · 2022-09-29T13:00:25Z

jenkins build aarch64(cmake:'EXTRA_CONFIGURE_ARGS=--disable-OMR_GC_MODRON_SCAVENGER')

babsingh · 2022-09-29T13:02:10Z

jenkins build arm(cmake:'EXTRA_CONFIGURE_ARGS=--disable-OMR_GC_MODRON_SCAVENGER')

babsingh · 2022-09-29T17:18:29Z

All builds, with OMR_GC_MODRON_SCAVENGER disabled, have passed. Starting jobs with the flag enabled:

jenkins build all

babsingh · 2022-09-29T19:13:48Z

jenkins build zos

babsingh · 2022-09-29T19:14:23Z

jenkins build zos

babsingh · 2022-09-29T19:55:05Z

jenkins build zos

babsingh · 2022-09-29T20:31:11Z

jenkins build zos

babsingh · 2022-09-29T20:49:22Z

jenkins build zos

babsingh

In addition to #6715 (comment), all builds, with OMR_GC_MODRON_SCAVENGER enabled, have passed.

dmitripivkine requested review from charliegracie, youngar and rwy7 as code owners September 20, 2022 17:43

dmitripivkine mentioned this pull request Sep 20, 2022

Fix caller of getGuaranteedNurseryRange() and use new functions eclipse-openj9/openj9#15928

Merged

dmitripivkine commented Sep 20, 2022

View reviewed changes

github-actions bot added the comp:gc label Sep 20, 2022

dmitripivkine force-pushed the master branch from 68f4b08 to aa81390 Compare September 20, 2022 17:59

amicic reviewed Sep 20, 2022

View reviewed changes

dmitripivkine force-pushed the master branch from 3a30274 to 15def8d Compare September 20, 2022 18:41

amicic approved these changes Sep 20, 2022

View reviewed changes

dmitripivkine force-pushed the master branch from ea59888 to 8b5f404 Compare September 20, 2022 20:29

amicic approved these changes Sep 20, 2022

View reviewed changes

babsingh reviewed Sep 21, 2022

View reviewed changes

dmitripivkine force-pushed the master branch from 3d40c25 to d1bf1a5 Compare September 21, 2022 12:52

babsingh mentioned this pull request Sep 24, 2022

Build failure on Linux and Mac - member name not found #6725

Closed

dmitripivkine requested review from mstoodle and 0xdaryl as code owners September 27, 2022 21:06

dmitripivkine force-pushed the master branch from 2cbf0dc to 3473148 Compare September 27, 2022 21:28

babsingh requested changes Sep 28, 2022

View reviewed changes

gc/base/GCExtensionsBase.hpp Outdated Show resolved Hide resolved

gc/base/GCExtensionsBase.hpp Outdated Show resolved Hide resolved

dmitripivkine force-pushed the master branch from 0234adb to ff58cb7 Compare September 28, 2022 17:22

dmitripivkine changed the title ~~Fix discrepancy in definition of functions in GC Extensions~~ Fix discrepancy in GC Extensions Base functions Sep 28, 2022

babsingh approved these changes Sep 29, 2022

View reviewed changes

babsingh merged commit 9260611 into eclipse-omr:master Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix discrepancy in GC Extensions Base functions #6715

Fix discrepancy in GC Extensions Base functions #6715

dmitripivkine commented Sep 20, 2022 •

edited

Loading

dmitripivkine Sep 20, 2022

amicic Sep 20, 2022

dmitripivkine Sep 20, 2022

dmitripivkine commented Sep 20, 2022

dmitripivkine commented Sep 20, 2022

babsingh Sep 21, 2022 •

edited

Loading

babsingh commented Sep 21, 2022 •

edited

Loading

babsingh commented Sep 21, 2022

dmitripivkine commented Sep 21, 2022

babsingh commented Sep 21, 2022 •

edited

Loading

babsingh commented Sep 27, 2022 •

edited

Loading

dmitripivkine commented Sep 27, 2022

babsingh commented Sep 28, 2022

babsingh commented Sep 28, 2022

babsingh commented Sep 28, 2022

babsingh commented Sep 28, 2022

babsingh commented Sep 28, 2022

babsingh left a comment •

edited

Loading

babsingh commented Sep 28, 2022

dmitripivkine commented Sep 28, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022 •

edited

Loading

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh left a comment

	#elif /* OMR_GC_MODRON_SCAVENGER */
	#else /* OMR_GC_MODRON_SCAVENGER */

Fix discrepancy in GC Extensions Base functions #6715

Fix discrepancy in GC Extensions Base functions #6715

Conversation

dmitripivkine commented Sep 20, 2022 • edited Loading

dmitripivkine Sep 20, 2022

Choose a reason for hiding this comment

amicic Sep 20, 2022

Choose a reason for hiding this comment

dmitripivkine Sep 20, 2022

Choose a reason for hiding this comment

dmitripivkine commented Sep 20, 2022

dmitripivkine commented Sep 20, 2022

babsingh Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

babsingh commented Sep 21, 2022 • edited Loading

babsingh commented Sep 21, 2022

dmitripivkine commented Sep 21, 2022

babsingh commented Sep 21, 2022 • edited Loading

babsingh commented Sep 27, 2022 • edited Loading

dmitripivkine commented Sep 27, 2022

babsingh commented Sep 28, 2022

babsingh commented Sep 28, 2022

babsingh commented Sep 28, 2022

babsingh commented Sep 28, 2022

babsingh commented Sep 28, 2022

babsingh left a comment • edited Loading

Choose a reason for hiding this comment

babsingh commented Sep 28, 2022

dmitripivkine commented Sep 28, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022 • edited Loading

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh commented Sep 29, 2022

babsingh left a comment

Choose a reason for hiding this comment

dmitripivkine commented Sep 20, 2022 •

edited

Loading

babsingh Sep 21, 2022 •

edited

Loading

babsingh commented Sep 21, 2022 •

edited

Loading

babsingh commented Sep 21, 2022 •

edited

Loading

babsingh commented Sep 27, 2022 •

edited

Loading

babsingh left a comment •

edited

Loading

babsingh commented Sep 29, 2022 •

edited

Loading