GC CRIU: Reinit HeapRegionDescriptorExtensions (Region Obj Lists) by RSalman · Pull Request #17614 · eclipse-openj9/openj9

RSalman · 2023-06-16T19:49:13Z

HeapRegionDescriptorExtensions contain multiple array of obj lists which are inited at startup based on the startup thread count. These must be reinitialized during restore (CRIU) according to the new GC thread
count. Specifically, when the thread count is increased, it is necessary to divide the region object lists. This is required to improve parallelism with GC threads and ultimately improve GC performance for the restore environment. For background, see eclipse-omr/omr#6888 (Compensate for Thread Count Change).

Signed-off-by: Salman Rana salman.rana@ibm.com

RSalman · 2023-06-16T20:35:29Z

@amicic could you please have a look

Integrate with downstream OpenJ9 restore reinit APIs: - Thread Local (env) Obj Buffers - Region Extensions Object Lists Depends on eclipse-openj9/openj9#17614 Signed-off-by: Salman Rana <salman.rana@ibm.com>

amicic · 2023-06-20T18:12:27Z

not setting ref list to null?

There's no global ref list

there is global referenceObjectLists but only used for Metronome. for completeness, I'd clear it here, as well

i did notice that it was never cleared in its constructor. while there to clear it, put referenceObjectLists and unfinalizedObjectLists next to each other

There's no global ref list

Ahh yes there is! I assumed not because its never built while initing the lists (like the other 3 lists)

Could you please clarify what you mean by

i did notice that it was never cleared in its constructor. while there to clear it, put referenceObjectLists and unfinalizedObjectLists next to each other

in MM_GCExtensions referenceObjectLists is declared by never inited/cleared. actually, not just that one, but a few other fileds

the 4 ObjectLists fields are scattered in that structure. would be nice to have them next to each other

we should also start incrementally prepending '_' to those fields in Extensions/ExtensionsBase, but if that would require a lot more file to touch (just to fix those 4), you can ignore it

Integrate with downstream OpenJ9 restore reinit APIs: - Thread Local (env) Obj Buffers - Region Extensions Object Lists Depends on eclipse-openj9/openj9#17614 Signed-off-by: Salman Rana <salman.rana@ibm.com>

HeapRegionDescriptorExtensions contain multiple array of obj lists which are inited at startup based on the startup thread count. These must be reinitialized during restore (CRIU) according to the new GC thread count. Specifically, when the thread count is increased, it is necessary to divide the region object lists. This is required to improve parallelism with GC threads and ultimately improve GC performance for the restore environment. For background, see eclipse-omr/omr#6888 (Compensate for Thread Count Change). Signed-off-by: Salman Rana <salman.rana@ibm.com>

amicic · 2023-06-20T19:58:19Z

jenkins test sanity aix jdk11

amicic · 2023-06-20T19:59:39Z

jenkins test sanity xlinuxcriu jdk17

amicic · 2023-06-20T19:59:54Z

jenkins compile win32 jdk8

amicic · 2023-06-20T20:00:06Z

jenkins test sanity win jdk11

keithc-ca · 2023-06-21T15:05:26Z

-		for (uintptr_t index = 0; index < arrayElements; index++) {
+		Assert_MM_true(arrayElementsTotal >= arrayElementsToCopy);
+		/* Check whether a new array instance in being created from an existing array. If so, copy over the elements first. */
+		if (arrayElementsToCopy > 0) {


I see this a number of places: what the point of if statements like the one here? If arrayElementsToCopy == 0, the for loop won't execute.

agree, could be removed

Addressed as part of #17629

Integrate with downstream OpenJ9 restore reinit APIs: - Thread Local (env) Obj Buffers - Region Extensions Object Lists Depends on eclipse-openj9/openj9#17614 Signed-off-by: Salman Rana <salman.rana@ibm.com>

RSalman force-pushed the reinit-region branch 3 times, most recently from b4ac4d5 to b65dfd0 Compare June 16, 2023 20:18

dmitripivkine added comp:gc criu Used to track CRIU snapshot related work labels Jun 19, 2023

RSalman force-pushed the reinit-region branch from b65dfd0 to 0e9dd0d Compare June 19, 2023 16:35

amicic reviewed Jun 19, 2023

View reviewed changes

Comment thread runtime/gc_base/ContinuationObjectList.cpp Outdated

amicic reviewed Jun 19, 2023

View reviewed changes

Comment thread runtime/gc_base/ContinuationObjectList.cpp Outdated

This was referenced Jun 19, 2023

GC CRIU: Reinit HeapRegionDescriptorExtensions (Region Obj Lists) #17388

Closed

Integrate GC Restore With Delegate Reini APIs eclipse-omr/omr#7038

Draft

RSalman force-pushed the reinit-region branch from 0e9dd0d to e4194da Compare June 19, 2023 20:41

RSalman requested a review from amicic June 19, 2023 20:42

amicic reviewed Jun 19, 2023

View reviewed changes

Comment thread runtime/gc_glue_java/HeapRegionDescriptorStandardExtension.hpp Outdated

RSalman force-pushed the reinit-region branch 2 times, most recently from b1e0cdc to c724f86 Compare June 19, 2023 22:58

RSalman requested a review from amicic June 19, 2023 23:03

RSalman force-pushed the reinit-region branch from c724f86 to d4ef11b Compare June 20, 2023 16:05

amicic reviewed Jun 20, 2023

View reviewed changes

Comment thread runtime/gc_glue_java/HeapRegionDescriptorStandardExtension.hpp Outdated

amicic reviewed Jun 20, 2023

View reviewed changes

RSalman force-pushed the reinit-region branch 2 times, most recently from 9e63614 to 57ba037 Compare June 20, 2023 18:25

RSalman requested a review from amicic June 20, 2023 18:26

amicic reviewed Jun 20, 2023

View reviewed changes

Comment thread runtime/gc_base/ContinuationObjectList.cpp Outdated

RSalman force-pushed the reinit-region branch from 57ba037 to 6a1b378 Compare June 20, 2023 18:59

amicic merged commit ba637b2 into eclipse-openj9:master Jun 21, 2023

keithc-ca reviewed Jun 21, 2023

View reviewed changes

RSalman mentioned this pull request Jun 22, 2023

(0.40) GC CRIU: Reinit HeapRegionDescriptorExtensions (Region Obj Lists) #17645

Merged

Uh oh!

Conversation

RSalman commented Jun 16, 2023

Uh oh!

RSalman commented Jun 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amicic commented Jun 20, 2023

Uh oh!

amicic commented Jun 20, 2023

Uh oh!

amicic commented Jun 20, 2023

Uh oh!

amicic commented Jun 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants