Runtime compressed refs work #8324

gacholio · 2020-01-16T18:28:30Z

Remove assumptions about the compile-time size of various types (f9object_t/fomrobject_t/object headers).

[ci skip]

Signed-off-by: Graham Chapman graham_chapman@ca.ibm.com

gacholio · 2020-01-16T18:29:00Z

@dmitripivkine @amicic

This is not complete, but there's enough here to start having a look.

runtime/gc_glue_java/PointerArrayObjectScanner.hpp

dmitripivkine · 2020-01-16T21:05:13Z

I walked through the changes and they seems correct, however probably I will need another look focus on possible missed changed in pointers math

amicic · 2020-01-21T19:24:51Z

runtime/gc_base/IndexableObjectAllocationModel.cpp

@@ -195,7 +195,7 @@ MM_IndexableObjectAllocationModel::layoutContiguousArraylet(MM_EnvironmentBase *
 		GC_SlotObject slotObject(env->getOmrVM(), arrayoidPtr);
 		slotObject.writeReferenceToSlot((omrobjectptr_t)leafOffset);
 		leafOffset += arrayletLeafSize;
-		arrayoidPtr += 1;
+		arrayoidPtr = GC_SlotObject::addToSlotAddress(arrayoidPtr, 1, compressed);


We could have a non-static variant of this method and call for each iteration slotObject.addToSlotAddress(1, compressed); while constructing slotObject only once before entering the loop. It would be slightly more readable to me, but I'm not pushing for it.

amicic · 2020-01-21T19:37:34Z

runtime/gc_base/ObjectAccessBarrier.cpp

-	return convertPointerFromToken(*srcAddress);
+	mm_j9object_t result = NULL;
+	if (compressObjectReferences()) {
+		result = convertPointerFromToken(*(uint32_t*)srcAddress);


hm... convertPointerFromToken itself has already a runtime 'if CR' check.

runtime/gc_base/ObjectAccessBarrier.cpp

amicic · 2020-01-21T19:54:24Z

runtime/gc_base/ObjectAccessBarrier.hpp

+			arrayletLeafBase = (UDATA)convertPointerFromToken(*(U_32*)arrayletLeafSlot);
+		} else {
+			arrayletLeafBase = *(UDATA*)arrayletLeafSlot;
+		}


We have up to 3 'if CR' runtime checks, while it's probably doable with only one :(

Something like this?

if (compressObjectReferences()) { arrayletLeafBase = (UDATA)convertCompressedPointerFromToken(*((U_32*)arrayoidPointer) + arrayletIndex); } else { arrayletLeafBase = *(((UDATA*)arrayoidPointer) + arrayletIndex); }

where convertCompressedPointerFromToken() does just shifting and no 'if CR' check. This helper could be used elsewhere (in readImpl() )

amicic · 2020-01-21T20:13:14Z

runtime/gc_glue_java/MetronomeDelegate.hpp

@@ -200,8 +200,9 @@ class MM_MetronomeDelegate : public MM_BaseNonVirtual
 	MMINLINE uintptr_t
 	scanPointerArraylet(MM_EnvironmentRealtime *env, fomrobject_t *arraylet)
 	{
+		UDATA const referenceSize = env->compressObjectReferences() ? sizeof(uint32_t) : sizeof(uintptr_t);


this pattern repeats often. we should just have something like env->getObjectReferenceSize()

amicic · 2020-01-21T20:19:46Z

runtime/gc_glue_java/MetronomeDelegate.hpp

+				GC_SlotObject slotObject(_javaVM->omrVM, (fj9object_t*)scanPtr);
+				_markingScheme->markObject(env, slotObject.readReferenceFromSlot());
+				scanPtr++;
+			}


This pass is somewhat perf critical and extra runtime checks in readReferenceFromSlot() could be tangible (sparsly populated arrays). Would it make sense to have specialized GC_SlotObject::readCRReferenceFromSlot/readNonCRReferenceFromSlot APIs?

amicic · 2020-01-21T21:08:24Z

runtime/gc_include/ObjectAccessBarrierAPI.hpp

 #endif /* OMR_GC_COMPRESSED_POINTERS */
+		{
+			return (fj9object_t)(UDATA)pointer;
+		}


This itself is good, but similarly to slow barrier code, this is invoked from a site where there is already a runtime CR check (see readObjectImpl).

As mentioned before, we could specialize this method further into two methods for CR and nonCR, and use proper one from the call site that already knows if it's CR or not.

We could keep this one too, for sites where we don't know if it's CR.

amicic · 2020-01-21T21:11:38Z

runtime/gc_structs/ArrayletLeafIterator.hpp

 	/**
 	 * @return the next leaf reference slot in the arraylet
 	 * @return NULL if there are no more reference slots in the object
 	 */
 	MMINLINE GC_SlotObject *nextLeafPointer()
 	{
 		if (_numLeafsCounted < _numLeafs) {
-			_slotObject.writeAddressToSlot(&_arrayoid[_numLeafsCounted]);
+			_slotObject.writeAddressToSlot(GC_SlotObject::addToSlotAddress(_arrayoid, _numLeafsCounted, compressObjectReferences()));


as proposed before we could use a non-static addToSlotAddress

gacholio · 2020-01-21T21:17:01Z

@amicic My current goal is to avoid regressing the single-mode builds. Optimizing multiple compressed checks is in plan for later on (likely by specializing entire code paths for compressed/full rather than microoptimizing the individual functions).

gacholio · 2020-02-03T04:26:09Z

@dmitripivkine @amicic With this PR, mixed mode now works on xa64 (metronome and balanced work is not complete).

I'd prefer to make the suggested optimizations/clarifications in future PRs (as they require some OMR work first).

gacholio · 2020-02-03T04:27:18Z

@andrewcraik @fjeremic Can you please find someone to review the JIT changes?

gacholio · 2020-02-03T05:34:17Z

I believe I've found all of the places doing fobj pointer math (I temporarily replaced fobj* with a new type defined as a pointer to an undefined struct, which makes all pointer math fail to compile), so there should be no need to look for missed changes, just correctness of the current ones.

Some math changes remain in metronome and balanced, but I know where they are now.

fjeremic

JIT changes look straightforward. I'd advise another x86 expert to take a look at the NASM changes.

gacholio · 2020-02-03T19:11:15Z

Metronome is now working in mixed mode as well.

dmitripivkine · 2020-02-04T23:39:08Z

runtime/gc_glue_java/PointerArrayObjectScanner.hpp

@@ -58,7 +58,7 @@ class GC_PointerArrayObjectScanner : public GC_IndexableObjectScanner
 	 * @param[in] flags Scanning context flags
 	 */
 	MMINLINE GC_PointerArrayObjectScanner(MM_EnvironmentBase *env, omrobjectptr_t arrayPtr, fomrobject_t *basePtr, fomrobject_t *limitPtr, fomrobject_t *scanPtr, fomrobject_t *endPtr, uintptr_t flags)
-		: GC_IndexableObjectScanner(env, arrayPtr, basePtr, limitPtr, scanPtr, endPtr, ((endPtr - scanPtr) < _bitsPerScanMap) ? (((uintptr_t)1 << (endPtr - scanPtr)) - 1) : UDATA_MAX, sizeof(fomrobject_t), flags)
+		: GC_IndexableObjectScanner(env, arrayPtr, basePtr, limitPtr, scanPtr, endPtr, (GC_SlotObject::subtractSlotAddresses(endPtr, scanPtr, env->compressObjectReferences()) < _bitsPerScanMap) ? ((uintptr_t)1 << GC_SlotObject::subtractSlotAddresses(endPtr, scanPtr, env->compressObjectReferences())) - 1 : UDATA_MAX, env->compressObjectReferences() ? sizeof(uint32_t) : sizeof(uintptr_t), flags)


This line is too long now... Can it be split somehow to be more read friendly?

See if it's better now...

fjeremic · 2020-02-05T14:29:10Z

Jenkins test sanity all jdk8

gacholio · 2020-02-05T16:24:51Z

Jenkins test sanity all jdk8

Remove assumptions about the compile-time size of various types (f9object_t/fomrobject_t/object headers). [ci skip] Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>

gacholio · 2020-02-05T17:25:12Z

Jenkins test sanity all jdk8

gacholio · 2020-02-05T20:27:55Z

The failing testing looks like a build issue (JIT can't load due to protobuf stuff) - I suggest ignoring it and merging once the rest completes.

pshipton · 2020-02-05T20:31:04Z

The failing testing looks like a build issue (JIT can't load due to protobuf stuff)

Ya, it's a problem on ub16-ppcle-2 #8257. I had disabled, but every time jenkins restarts the machine comes back online. I'll disable the machine again.

gacholio force-pushed the compress branch from 9f5d01e to 97000a7 Compare January 16, 2020 18:32

dmitripivkine added the comp:gc label Jan 16, 2020

dmitripivkine requested review from amicic and dmitripivkine January 16, 2020 20:25

dmitripivkine reviewed Jan 16, 2020

View reviewed changes

runtime/gc_glue_java/PointerArrayObjectScanner.hpp Outdated Show resolved Hide resolved

dmitripivkine reviewed Jan 16, 2020

View reviewed changes

runtime/gc_glue_java/PointerArrayObjectScanner.hpp Outdated Show resolved Hide resolved

dmitripivkine reviewed Jan 16, 2020

View reviewed changes

runtime/gc_glue_java/PointerArrayObjectScanner.hpp Outdated Show resolved Hide resolved

gacholio force-pushed the compress branch 7 times, most recently from c42b2c5 to 69227f0 Compare January 20, 2020 22:11

amicic reviewed Jan 21, 2020

View reviewed changes

runtime/gc_base/ObjectAccessBarrier.cpp Outdated Show resolved Hide resolved

amicic reviewed Jan 21, 2020

View reviewed changes

gacholio force-pushed the compress branch 4 times, most recently from 706232d to e5a7217 Compare January 27, 2020 21:17

gacholio added the comp:jit label Jan 27, 2020

gacholio force-pushed the compress branch 5 times, most recently from 90b19a6 to a039663 Compare February 3, 2020 04:24

gacholio added the comp:vm label Feb 3, 2020

fjeremic approved these changes Feb 3, 2020

View reviewed changes

gacholio force-pushed the compress branch 3 times, most recently from 448e7b9 to 5f169d3 Compare February 3, 2020 19:10

dmitripivkine reviewed Feb 4, 2020

View reviewed changes

dmitripivkine approved these changes Feb 5, 2020

View reviewed changes

gacholio changed the title ~~WIP: Runtime compressed refs work~~ Runtime compressed refs work Feb 5, 2020

gacholio force-pushed the compress branch from b5d8473 to d48831f Compare February 5, 2020 01:58

amicic approved these changes Feb 5, 2020

View reviewed changes

pshipton mentioned this pull request Feb 5, 2020

Add compare and branch JIT Test #8402

Merged

gacholio force-pushed the compress branch from d48831f to e862004 Compare February 5, 2020 17:24

Runtime compressed refs work

e862004

Remove assumptions about the compile-time size of various types (f9object_t/fomrobject_t/object headers). [ci skip] Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>

amicic merged commit b33493c into eclipse-openj9:master Feb 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime compressed refs work #8324

Runtime compressed refs work #8324

gacholio commented Jan 16, 2020 •

edited

Loading

gacholio commented Jan 16, 2020

dmitripivkine commented Jan 16, 2020

amicic Jan 21, 2020

amicic Jan 21, 2020 •

edited

Loading

amicic Jan 21, 2020

amicic Jan 21, 2020

amicic Jan 21, 2020

amicic Jan 21, 2020

amicic Jan 21, 2020 •

edited

Loading

amicic Jan 21, 2020

gacholio commented Jan 21, 2020

gacholio commented Feb 3, 2020

gacholio commented Feb 3, 2020

gacholio commented Feb 3, 2020

fjeremic left a comment

gacholio commented Feb 3, 2020

dmitripivkine Feb 4, 2020

gacholio Feb 5, 2020

gacholio Feb 5, 2020

fjeremic commented Feb 5, 2020

gacholio commented Feb 5, 2020

gacholio commented Feb 5, 2020

gacholio commented Feb 5, 2020

pshipton commented Feb 5, 2020

Runtime compressed refs work #8324

Runtime compressed refs work #8324

Conversation

gacholio commented Jan 16, 2020 • edited Loading

gacholio commented Jan 16, 2020

dmitripivkine commented Jan 16, 2020

Choose a reason for hiding this comment

amicic Jan 21, 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

amicic Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gacholio commented Jan 21, 2020

gacholio commented Feb 3, 2020

gacholio commented Feb 3, 2020

gacholio commented Feb 3, 2020

fjeremic left a comment

Choose a reason for hiding this comment

gacholio commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjeremic commented Feb 5, 2020

gacholio commented Feb 5, 2020

gacholio commented Feb 5, 2020

gacholio commented Feb 5, 2020

pshipton commented Feb 5, 2020

gacholio commented Jan 16, 2020 •

edited

Loading

amicic Jan 21, 2020 •

edited

Loading

amicic Jan 21, 2020 •

edited

Loading