Upgrading inlined code to virtual methods for register scavenger #6781

LinHu2016 · 2022-10-20T16:03:18Z

Upgrading inlined code to virtual methods for register scavenger
so that subclasses could override behavior.

LinHu2016 · 2022-10-20T16:03:56Z

@amicic @dmitripivkine please review the change, Thanks

amicic · 2022-10-20T16:13:11Z

gc/base/standard/Scavenger.cpp

+	/* register the scavenger in GCExtensionsBase */
+	_extensions->registerScavenger(this);
+#endif /* OMR_GC_MODRON_SCAVENGER */
+


minor difference, but i'd keep it at original spot (higher level entity knows better to whom it should be registered)

comment is redundant (the method name is self explanatory)

amicic · 2022-10-20T16:14:36Z

gc/base/GCExtensionsBase.hpp

@@ -906,6 +906,9 @@ class MM_GCExtensionsBase : public MM_BaseVirtual {
 	MMINLINE OMR_VM* getOmrVM() { return _omrVM; }
 	MMINLINE void setOmrVM(OMR_VM* omrVM) { _omrVM = omrVM; }

+#if defined(OMR_GC_MODRON_SCAVENGER)
+	virtual void registerScavenger(MM_Scavenger *scavenger) { this->scavenger = scavenger; }


for symmetry, let's introduce unregister, too

it'd be used in MM_ConfigurationGenerational::tearDown

See https://github.com/eclipse/omr/blob/master/doc/CodingStandard.md#variables.

In C++, member variable names should start with an underscore: _scavenger. This is just a note and does not need to be addressed in this PR since the scavenger member field was introduced in the past.

Unfortunately whole that class/file does not follow that standard, although by far most of other GC classes do.

Being a top level global GC class, conforming it to the standard would likely touch almost every GC file.

That's probably why nobody so far have tried to do it, but it should have really be done during the major OMR/J9 separation.

amicic · 2022-10-20T17:27:49Z

gc/base/standard/ConfigurationGenerational.cpp

@@ -202,8 +201,7 @@ MM_ConfigurationGenerational::createDefaultMemorySpace(MM_EnvironmentBase *envBa
 		return NULL;
 	}

-	/* register the scavenger in GCExtensionsBase */
-	ext->scavenger = scavenger;
+	_extensions->registerScavenger(this);



not 'this' but scavenger

does _extensions really exist?

amicic · 2022-10-20T17:53:47Z

subject and comment are not really correct

you are not changing any behavior

just upgrading inlined code to virtual methods so that subclasses could override behavior

LinHu2016 · 2022-10-20T19:16:54Z

@babsingh Could you please review the change, Thanks

babsingh

Minor nitpicks.

gc/base/GCExtensionsBase.hpp

babsingh · 2022-10-20T20:22:00Z

gc/base/GCExtensionsBase.hpp

@@ -906,6 +906,9 @@ class MM_GCExtensionsBase : public MM_BaseVirtual {
 	MMINLINE OMR_VM* getOmrVM() { return _omrVM; }
 	MMINLINE void setOmrVM(OMR_VM* omrVM) { _omrVM = omrVM; }

+#if defined(OMR_GC_MODRON_SCAVENGER)
+	virtual void registerScavenger(MM_Scavenger *scavenger) { this->scavenger = scavenger; }


See https://github.com/eclipse/omr/blob/master/doc/CodingStandard.md#variables.

In C++, member variable names should start with an underscore: _scavenger. This is just a note and does not need to be addressed in this PR since the scavenger member field was introduced in the past.

babsingh · 2022-10-21T12:08:50Z

@LinHu2016 There is a standalone commit for the new change. There should be only one commit and it should include all the changes.

babsingh · 2022-10-21T14:12:59Z

jenkins build all

gc/base/GCExtensionsBase.hpp

babsingh · 2022-10-21T15:06:39Z

jenkins build all

babsingh · 2022-10-21T15:08:42Z

Old zLinux PR build: https://ci.eclipse.org/omr/job/PullRequest-linux_390-64/3942.
Also, running the zLinux PR build with DOMR_GC_MODRON_SCAVENGER disabled.

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

babsingh · 2022-10-21T15:16:19Z

https://ci.eclipse.org/omr/job/PullRequest-linux_390-64/3943/console

In the zLinux PR build with OMR_GC_MODRON_SCAVENGER disabled, the following error is seen:

11:09:42  /home/omr/workspace/Build/gc/base/GCExtensionsBase.hpp:989:33: error: ‘MM_Scavenger’ has not been declared
11:09:42    virtual void registerScavenger(MM_Scavenger *scavenger)
11:09:42                                   ^
11:09:42  gc/CMakeFiles/omrgc.dir/build.make:62: recipe for target 'gc/CMakeFiles/omrgc.dir/base/AddressOrderedListPopulator.cpp.o' failed
11:09:42  make[2]: *** [gc/CMakeFiles/omrgc.dir/base/AddressOrderedListPopulator.cpp.o] Error 1
11:09:42  CMakeFiles/Makefile2:1984: recipe for target 'gc/CMakeFiles/omrgc.dir/all' failed

amicic · 2022-10-21T15:20:06Z

https://ci.eclipse.org/omr/job/PullRequest-linux_390-64/3943/console

In the zLinux PR build with OMR_GC_MODRON_SCAVENGER disabled, the following error is seen:

11:09:42  /home/omr/workspace/Build/gc/base/GCExtensionsBase.hpp:989:33: error: ‘MM_Scavenger’ has not been declared
11:09:42    virtual void registerScavenger(MM_Scavenger *scavenger)
11:09:42                                   ^
11:09:42  gc/CMakeFiles/omrgc.dir/build.make:62: recipe for target 'gc/CMakeFiles/omrgc.dir/base/AddressOrderedListPopulator.cpp.o' failed
11:09:42  make[2]: *** [gc/CMakeFiles/omrgc.dir/base/AddressOrderedListPopulator.cpp.o] Error 1
11:09:42  CMakeFiles/Makefile2:1984: recipe for target 'gc/CMakeFiles/omrgc.dir/all' failed

Argh, we either have to revert to having #ifdef outside or use void * (and upcast later)
I'm slightly more inclined to revert this part, but i'm open for the other option (or anything else new).

amicic · 2022-10-21T15:59:49Z

Argh, we either have to revert to having #ifdef outside or use void * (and upcast later) I'm slightly more inclined to revert this part, but i'm open for the other option (or anything else new).

Third option is to remove #ifdef around forward declaration

#if defined(OMR_GC_MODRON_SCAVENGER)
class MM_Scavenger;
#endif /* defined(OMR_GC_MODRON_SCAVENGER) */

Still, I slightly prefer the least radical (but ugliest to my eye) option to keep (un)registerScavenger under #ifdef, but again open to hear opinions.

LinHu2016 · 2022-10-21T17:10:37Z

I will remove #ifdef around forward declaration

Upgrading inlined code to virtual methods for register scavenger so that subclasses could override behavior Signed-off-by: Lin Hu <linhu@ca.ibm.com>

amicic · 2022-10-21T18:35:59Z

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

babsingh · 2022-10-21T19:11:00Z

zLinux PR build with OMR_GC_MODRON_SCAVENGER disabled passed: https://ci.eclipse.org/omr/job/PullRequest-linux_390-64/3945/consoleFull. Running PR builds with OMR_GC_MODRON_SCAVENGER enabled.

jenkins build all

babsingh

https://ci.eclipse.org/omr/job/PullRequest-linux_ppc-64_le_gcc/3855/console

Known failure: #6571

All other PR builds passed successfully.

LinHu2016 requested review from charliegracie, youngar and rwy7 as code owners October 20, 2022 16:03

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

amicic reviewed Oct 20, 2022

View reviewed changes

LinHu2016 force-pushed the master branch 2 times, most recently from 68bb805 to 5e9f7c6 Compare October 20, 2022 17:23

amicic reviewed Oct 20, 2022

View reviewed changes

LinHu2016 force-pushed the master branch 2 times, most recently from a15acfa to 41e4080 Compare October 20, 2022 17:43

amicic approved these changes Oct 20, 2022

View reviewed changes

LinHu2016 force-pushed the master branch from 41e4080 to 97b5a9f Compare October 20, 2022 18:02

LinHu2016 changed the title ~~Register scavenger instance in GCExtensionsBase earlier~~ Upgrading inlined code to virtual methods for register scavenger Oct 20, 2022

babsingh requested changes Oct 20, 2022

View reviewed changes

LinHu2016 mentioned this pull request Oct 20, 2022

Handle Continuation scan during Concurrent Scavenger and Metronome eclipse-openj9/openj9#16157

Merged

LinHu2016 force-pushed the master branch from 5ffc2a7 to f6999a1 Compare October 21, 2022 14:02

amicic reviewed Oct 21, 2022

View reviewed changes

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

LinHu2016 force-pushed the master branch 2 times, most recently from 5c6e5b9 to 2b7ff60 Compare October 21, 2022 14:50

amicic approved these changes Oct 21, 2022

View reviewed changes

Upgrading inlined code to virtual methods for register scavenger

d612e91

Upgrading inlined code to virtual methods for register scavenger so that subclasses could override behavior Signed-off-by: Lin Hu <linhu@ca.ibm.com>

LinHu2016 force-pushed the master branch from 2b7ff60 to d612e91 Compare October 21, 2022 17:11

babsingh approved these changes Oct 21, 2022

View reviewed changes

babsingh merged commit 0ae182f into eclipse-omr:master Oct 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrading inlined code to virtual methods for register scavenger #6781

Upgrading inlined code to virtual methods for register scavenger #6781

LinHu2016 commented Oct 20, 2022 •

edited

Loading

LinHu2016 commented Oct 20, 2022

amicic Oct 20, 2022 •

edited

Loading

amicic Oct 20, 2022 •

edited

Loading

babsingh Oct 20, 2022

amicic Oct 21, 2022

amicic Oct 20, 2022

amicic Oct 20, 2022

amicic commented Oct 20, 2022

LinHu2016 commented Oct 20, 2022

babsingh left a comment

babsingh Oct 20, 2022

babsingh commented Oct 21, 2022

babsingh commented Oct 21, 2022

babsingh commented Oct 21, 2022

babsingh commented Oct 21, 2022 •

edited

Loading

babsingh commented Oct 21, 2022

amicic commented Oct 21, 2022

amicic commented Oct 21, 2022 •

edited

Loading

LinHu2016 commented Oct 21, 2022

amicic commented Oct 21, 2022

babsingh commented Oct 21, 2022 •

edited

Loading

babsingh left a comment

Upgrading inlined code to virtual methods for register scavenger #6781

Upgrading inlined code to virtual methods for register scavenger #6781

Conversation

LinHu2016 commented Oct 20, 2022 • edited Loading

LinHu2016 commented Oct 20, 2022

amicic Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

amicic Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

babsingh Oct 20, 2022

Choose a reason for hiding this comment

amicic Oct 21, 2022

Choose a reason for hiding this comment

amicic Oct 20, 2022

Choose a reason for hiding this comment

amicic Oct 20, 2022

Choose a reason for hiding this comment

amicic commented Oct 20, 2022

LinHu2016 commented Oct 20, 2022

babsingh left a comment

Choose a reason for hiding this comment

babsingh Oct 20, 2022

Choose a reason for hiding this comment

babsingh commented Oct 21, 2022

babsingh commented Oct 21, 2022

babsingh commented Oct 21, 2022

babsingh commented Oct 21, 2022 • edited Loading

babsingh commented Oct 21, 2022

amicic commented Oct 21, 2022

amicic commented Oct 21, 2022 • edited Loading

LinHu2016 commented Oct 21, 2022

amicic commented Oct 21, 2022

babsingh commented Oct 21, 2022 • edited Loading

babsingh left a comment

Choose a reason for hiding this comment

LinHu2016 commented Oct 20, 2022 •

edited

Loading

amicic Oct 20, 2022 •

edited

Loading

amicic Oct 20, 2022 •

edited

Loading

babsingh commented Oct 21, 2022 •

edited

Loading

amicic commented Oct 21, 2022 •

edited

Loading

babsingh commented Oct 21, 2022 •

edited

Loading