Deprecate MM_ScavengerForwardedHeader over MM_ForwardedHeader

[bragaigor]

Substitute MM_ScavengerForwardedHeader usage with
MM_ForwardedHeader since both APIs are meant to accomplish
the same goal and contain overlap functionality.

Depends: eclipse/omr#5769

Signed-off-by: Igor Braga higorb1@gmail.com

[bragaigor]

cc: @amicic @dmitripivkine

[gacholio]

Big thumbs up - it was unfortunate to have to go through the pain twice when I was doing the runtime compressed refs work.

[amicic]

why not removing SFH files?

[bragaigor]

It seems that DDR needs them here: https://github.com/eclipse/openj9/blob/master/debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/gc/GCScavengerForwardedHeader_V1.java#L31 and here https://github.com/eclipse/openj9/blob/master/runtime/ddr/gcddr.cpp#L75
Could I get rid of them? For instance could I substitute FORWARDED_TAG and GROW_TAG with locals constants? And delete line 75 from gcddr.cpp?

[amicic]

Ok, I see....

Well, GCScavengerForwardedHeader is already somewhat mismatching the C code. For midScavenge heap iteration case in DDR GCCheck we should be using (even though we know the name does not suggest it properly) GCForwardedHeader. But that's just naming, so we can mostly ignore it (although it would be nice to move the code to GCForwardedHeader to match C code)

The problem as you already observed is mostly about GROW_TAG. If we debug older core files, with newer DDR, we must remove the tag from forwarded header, but we do not have to do it for newer files (but it's benign if we do).

So, yes, we could just manually define GROW_TAG or better just ALL_TAG, and always strip them. Alternatively, we could define ALL_FORWARDED_TAG in ForwardedHeader.hpp and use existing OMR_FORWARDED_TAG.

[bragaigor]

If we rename GCScavengerForwardedHeader everywhere with GCForwardedHeader DDR should still be happy right? As we're not changing any logic with that, or would we need to leave the existing GCScavengerForwardedHeader files and create new ones for GCForwardedHeader?

Regarding the GROW_TAG, your suggestion worked by adding ALL_FORWARDED_TAG in ForwardedHeader.hpp. In this case I'll need to open a PR in OMR project side.

And with that, can we safely remove MM_ScavengerForwardedHeader from runtime/ddr/gcddr.cpp? Because that would be only usage left of SFH.

[amicic]

If we rename GCScavengerForwardedHeader everywhere with GCForwardedHeader DDR should still be happy right? As we're not changing any logic with that, or would we need to leave the existing GCScavengerForwardedHeader files and create new ones for GCForwardedHeader?

I don't know how hard it's to rename it. It's only nice to do, so it if it causes any pain (you have at least 2 files to deal with), just give up.

Regarding the GROW_TAG, your suggestion worked by adding ALL_FORWARDED_TAG in ForwardedHeader.hpp. In this case I'll need to open a PR in OMR project side.

Yes, we need an OMR change to introduce ALL tag, first.

And with that, can we safely remove MM_ScavengerForwardedHeader from runtime/ddr/gcddr.cpp? Because that would be only usage left of SFH.

Hope so, do know for sure. Try before proceeding with OMR PR.

[bragaigor]

I removed all occurrences and usages of MM_ScavengerForwardedHeader (including the files) and all seems to be working. I'll run some more extended tests before opening the ALL tag PR in OMR side just to be double sure

[dmitripivkine]

I removed all occurrences and usages of MM_ScavengerForwardedHeader (including the files) and all seems to be working. I'll run some more extended tests before opening the ALL tag PR in OMR side just to be double sure

I hope it is not applicable in this case but would you please double check? DDR required to support backwards compatibility so new DDR code should handle cores created by old code. So we must be sure we did not lose possibility to decode old cores by new DDR code.

[amicic]

OK, VM compiles and runs, and DDR compiles, but as Dmitri suggested you should test DDR a bit.... You need to create a core dump with old (before these changes) and new (with these changes, specifically, with forwarded header without GROW bit being set) VM and see that new DDR/jextract will be able to iterate the heap (by for example doing a GC check).
The core file must be generated in a middle PGC.

Anyhow, since in the newest implementation of DDR, we would strip both forwarded tag (0x4) and GROW flag (0x2) tag though ALL tag (0x6), we should be ok.

Similar test you can do with a Concurrent Scavenge (CS), what is a more complex case than STW Scavenge, which also sets FH tags other than just FORWARDED_TAG. Tt sometime (with large arrays) uses COPY HINT what is is same as GROW, 0x2 (that case was already covered by old DDR, by luck)

Additionally, to make DDR with CS fully happy (which is somewhat broken, currently) ALL tag should be 0x7, since CS can set OMR_SELF_FORWARDED_TAG (0x1), too. This is even harder to encounter/test. Not only core has to be generated in a middle of CS, but also that Scavenge cycle has to abort!

[bragaigor]

I’ve run several tests since yesterday and most of them passed, but DDR is not happy. In order to be able to remove ScavengerForwardedHeader completely the main challenge is in GCScavengerForwardedHeader_V1.java where DDR makes use of ScavengerForwardedHeader. I tried updating this file to use ForwardedHeader instead but DDR was not happy. I tested the scenario above where I generated a core file with core dump with old (before these changes) in the middle of a PGC and ran jdmpview with new version. When I ran gccheck with midscavenge, DDR was looking for MM_ForwardedHeader.ALL_FORWARDED_TAG (accessing existing MM_ForwardedHeader.OMR_FORWARDED_TAG also doesn't work) in the old DDR version which is non existent and it crashed with:

Checking CLASS HEAP...Error executing DDR command !gccheck all,noobjectheap:all:midscavenge,quiet : null
java.lang.reflect.InvocationTargetException
…
Caused by: java.lang.NoClassDefFoundError: com.ibm.j9ddr.vm29.j9.gc.GCScavengerForwardedHeader_V1 (initialization failure)
	at java.base/java.lang.J9VMInternals.initializationAlreadyFailed(J9VMInternals.java:135)
	at com.ibm.j9ddr.vm29.j9.gc.GCScavengerForwardedHeader.fromJ9Object(GCScavengerForwardedHeader.java:53)
…
Caused by: java.lang.NoSuchFieldError: com/ibm/j9ddr/vm29/structure/MM_ForwardedHeader.ALL_FORWARDED_TAG
	at com.ibm.j9ddr.vm29.j9.gc.GCScavengerForwardedHeader_V1.<clinit>(GCScavengerForwardedHeader_V1.java:39)

I then tried the following:

• We keep ScavengerForwardedHeader files and version GCScavengerForwardedHeader_V1 for .*_V2
• Remove ScavengerForwardedHeader files and update GCScavengerForwardedHeader_V1 to use local variables for ALL_TAGS and FORWARDED_TAG. And version GCScavengerForwardedHeader_V1 for .*_V2
• We could even get away without versioning and without adding new flags to ForwardedHeader, but I don't think that would be correct since DDR would only be aware of ScavengerForwardedHeader and not ForwardedHeader which is what we want.

Bottom line is how to access FORWARDED_TAG from GCScavengerForwardedHeader_V1. All three options worked but I would say the second one is a better option since we’re deprecating ScavengerForwardedHeader and we want DDR to be aware of ForwardedHeader instead of ScavengerForwardedHeader?

[bragaigor]

@keithc-ca would it be okay to go ahead with removing all usages of ScavengerForwardedHeader including the files and updating GCScavengerForwardedHeader_V1 to use local variables since we're deprecating ScavengerForwardedHeader? I did run some backward compatibility tests and they all worked. Or would you prefer to keep those files and GCScavengerForwardedHeader_V1 unchanged? Either way, I would be introducing a new version of GCScavengerForwardedHeader_V2.

[keithc-ca]

@keithc-ca would it be okay to go ahead with removing all usages of ScavengerForwardedHeader including the files and updating GCScavengerForwardedHeader_V1 to use local variables since we're deprecating ScavengerForwardedHeader? I did run some backward compatibility tests and they all worked. Or would you prefer to keep those files and GCScavengerForwardedHeader_V1 unchanged? Either way, I would be introducing a new version of GCScavengerForwardedHeader_V2.

I think GCScavengerForwardedHeader_V1 must remain for accessing old core files. The DDR code relies on the declaration of MM_ScavengerForwardedHeader so that must remain as well (it's fine if no native code uses it).

[amicic]

Well, I prefer removing native MM_ScavengerForwardedHeader altogether, and instead introducing DDR locally defined consts to have V1 compile/run. Does not make sense to drag around not so small native files for the sake of 2 const.

A compromise is to remove most of content of ScavengerForwardedHeader.hpp/cpp (probably we can completely remove *.cpp) except a couple of const definitions to make DDR happy. But again, it's cleaner to me to just define them in DDR.

[keithc-ca]

I agree it would be nice to remove the type altogether, but how do you propose that DDR will acquire the information it needs (field names, types and offsets)?

[amicic]

Right, ideally we need to dump content of ScavengerForwardedHeader in old cores, as well (I was mostly thinking about how to keep getForwardedObject working, what is important for tools like GC check).

Ok, so this should work: we keep the ScavengerForwardedHeader.hpp and we have the class declaration with their fields and consts, but we remove all methods (inlined in hpp and remove completely cpp files)?

[bragaigor]

Did some quick backward compatibility tests and Aleks idea works. I'll run some more tests to be double sure, and once everything works I'll update this PR.

[amicic]

This assumes that preserved flags are lowest bits of preserved slot, what actually is true. Still, it's more future proof to extract the flags and then provide them later to hasBeenMoved and other APIs:

forwardedHeaderPreservedFlags = getPreservedFlags(forwardedHeader);
....

[keithc-ca]

I expect this will break DDR support for older core files.

[amicic]

Please squash the commits

[bragaigor]

I did a last walk-through, rebased, squashed and all major tests are passing

[amicic]

Jenkins test sanity all jdk11

[amicic]

I was holding the merge of this change intentionally, since we had an independent problem walking heap in DDR mid-PGC, what seemed related.

Specifically, a part of this change is removal of Grow bit in FP (Forwarded Pointer in source object), seemingly useless, but we believe we now better understand its original intention. It was to recognize a special case when a hashed object was moved first time, in which case the object would grow by a hash slot. DDR when walking evacuate region needs to skip forwarded objects, and to determine size it would get info from destination object. If that size just grown this cycle, it would incorrectly skip more (by hash slot) in evacuate. If it has grown more than 1 PGC ago, both src and dst object would be same, larger size, and the skip would be correct. So, to distinguish this special case, grow bit would be set in FP, so that an iterator would know when it needs to adjust/reduce by hash slot size, relative from destination object size.

This same problem exists in Scavenger too, but it's worked around differently, using a special middle state in destination object (without going into detals how). DDR code is aware of this special state (GCScavengerForwardedHeader_V1::getObjectSize()) should properly adjust the size to get proper src size object to skip.

DDR is however not fully aware of Grow bit (other than it just needs to strip it to get pure forwarded address) and hence, DDR is generally not capable of walking heap dumped mid PGC.

Overall, removing Grow bit is still reasonable, since it's not regressing anything. We, however, believe we can unify behavior of destination object bits for Scavenger and PGC (in a separate change) so that DDR is eventually able to walk heap mid PGC, too.