Improvements for Relocatable Compilations #2919

dsouzai · 2018-09-18T15:04:45Z

This PR introduces changes in OpenJ9 required to enable optimizations that are currently implicitly disabled for relocatable compilations. It does so by introducing the Symbol Validation Manager infrastructure. The fundamental idea for the Symbol Validation Manager was thought up by @jdmpapin ; the current implementation is a collaboration between myself and @jdmpapin .

Issue tracking all the PRs: #2921

vijaysun-omr · 2018-09-28T20:22:51Z

runtime/compiler/optimizer/J9ValuePropagation.cpp

-                        TR::VPNonNullObject::create(this), NULL, NULL,
-                        TR::VPObjectLocation::create(this, TR::VPObjectLocation::JavaLangClassObject)),
-                     classChildGlobal);
+               if (knot)


This change does not match the commit message. This seems to be a change to check for the existence of a known object table whereas the commit message is about counts.

ugh, I guess I must've missed this when squashing changes. I'll fix it soon.

vijaysun-omr · 2018-09-28T20:48:45Z

runtime/compiler/env/VMJ9.cpp

@@ -8524,6 +8541,12 @@ TR_J9SharedCacheVM::isClassVisible(TR_OpaqueClassBlock * sourceClass, TR_OpaqueC
          TR_J9VMBase::isClassVisible(sourceClass, destClass);
   }

+bool


Is there some inherent reason this optimization to skip frames cannot be done under AOT or is it just something you are not handling at this point in time ?

Oh you're viewing this commit by commit; in 5e6be87f7e9f4847742cbb2ebc8b8442b669caa4 (Add AOT Validations) is where I actually add the necessary validations to make sure this works under AOT

vijaysun-omr · 2018-09-28T21:09:02Z

runtime/compiler/x/codegen/J9AheadOfTimeCompile.cpp

-            if (guard->getSymbolReference()->getSymbol()->getMethodSymbol()->isVirtual())
-               flags = inlinedMethodIsVirtual;
-            }
+         // Setup flags field with type of method that needs to be validated at relocation time


Does this get impacted in any way by nest mates in Java11 ? i.e. can we get the "type of method" reliably the way we are getting it here when that feature is in play ?

@jdmpapin do you know?

The symbol on a call generated for a private invokevirtual or invokeinterface claims to be special, although the constant pool entry isn't a J9RAMSpecialMethodRef. If we think that we had an invokespecial and interpret the constant pool entry accordingly, it might happen to work for private invokevirtual, since its entry is the same shape, but if it does we probably shouldn't rely on it. It definitely won't work for private invokeinterface.

However, the only use of inlinedMethodIsSpecial that I can spot is just preventing TR_RelocationRecordInlinedMethod::validateClassesSame() from checking whether virtualMethodIsOverridden(). Is it used anywhere else?

While we're on the topic of nestmates, IIRC code gen has been changed to stop forcing unresolved dispatch for direct calls, which I think should work for calls to nestmates' private methods that have not been inlined.

vijaysun-omr · 2018-09-28T21:12:33Z

runtime/compiler/x/codegen/J9AheadOfTimeCompile.cpp

+            TR_OpaqueClassBlock *thisClass = guard->getThisClass();
+            uint16_t methodID = symValManager->getIDFromSymbol(static_cast<void *>(method));
+            uint16_t receiverClassID = symValManager->getIDFromSymbol(static_cast<void *>(thisClass));
+


Just checking why 16 bits for the ID is enough for every compiled method (with inlining) ?

These IDs are per compilation; ID=1 for the compilation of method A.foo() is not going to match ID=1 for the compilation of method B.bar(). Thus, @jdmpapin and I figure that 64k unique symbols should be sufficient for any compilation.

64k should be enough (distinct symbols) for any one (AOT compilation) 😅

Okay the reason I asked was that it seemed like an ID gets assigned every time we compile time query something that is of relevance from a validation perspective. While the number of distinct symbols in a compilation may be well under 64k, it seemed to me that the number of IDs were more dependent on how many times we queried and I was wondering if in a large method with many passes of different optimizations we might get close to 64k queries. Is there some simple safety valve possible anyway to detect and fail the AOT compilation if the ID goes above 64k ?

While the number of distinct symbols in a compilation may be well under 64k, it seemed to me that the number of IDs were more dependent on how many times we queried

That's not true, if we queried for class B from the constant pool of class C at cpIndex 5, then regardless of how many times times we ran that query, once a class has been assigned an ID, it is unique to that ID; there is a 1-1 mapping between an ID and a J9Class. Additionally, there is only record per unique query made. So, as in the example above, even if the query was made 10 times, there would only be one validation record stored, since we only need to check once whether class B can be retrieved from the constant pool of class C at cpIndex 5. However, if we retrieved class B from the constant pool of class C at cpIndices 3, 4, and 5, then we'd need three validation records. but the validation records would look something like this (assuming class B <--> ID 2 and class C <--> ID 1):

ClassFromCP,ID=2,BeholderID=1,cpIndex=3
ClassFromCP,ID=2,BeholderID=1,cpIndex=4
ClassFromCP,ID=2,BeholderID=1,cpIndex=5

All that said, I do have a check to ensure that we don't overflow the IDs just in case: 9a11e2f#diff-3e7f7c3bf56a075860c3736280f74b05R108

Okay thanks for the clarification and the check for overflow.

vijaysun-omr · 2018-09-28T21:27:06Z

runtime/compiler/x/codegen/J9AheadOfTimeCompile.cpp

+         if (comp->getOption(TR_UseSymbolValidationManager))
+            {
+            TR_OpaqueClassBlock *clazz = (TR_OpaqueClassBlock *)recordInfo->data5;
+            uintptrj_t classID = (uintptrj_t)symValManager->getIDFromSymbol(static_cast<void *>(clazz));


Is this ID comparable in any way to the 16 bit IDs I asked about earlier ? Now we are using uintptrj_t which is why I wanted to check.

This is one of those pieces of code that @mstoodle is going to very much dislike. With the symbol validation manager, aot relocations no longer need the cpIndex to find the symbol to validate. However, in order to reuse existing relocation records and the validation work that they already do (and not duplicate code), the new aot infrastructure uses the field that was traditionally used to store the cpIndex to instead store the ID for the symbol. Hence, even though the ID is only 16 bits, it gets stored in a uintptrj_t field. (see https://github.com/eclipse/openj9/pull/2919/files/151e307854fc2ae62b7d3a1a399da9c05644589b..f9c180ad226c0aee6d98e8356979586ea9b9ed61#diff-3f1aba0052032c44a8f47c99b97e30e2R263 for an even more (unfortunately) egregious example of this).

yeah....yuck confirmed...

vijaysun-omr · 2018-09-28T21:35:06Z

Question about something you wrote in one of the commit messages:

For example, if the method being compiled is A.foo(), then before the
compilation begins, the compiler creates:

1. Root Class Record for A
2. Method From Class Record for foo()

This will result in A getting ID 1, and foo() getting ID 2. As the
compilation continues, if the compiler gets class B in two different
ways, namely once by name and once via CP, then two other records get
created:

3. Class by Name for B
4. Class from CP for B

What happens if we get class B by name again in some other spot in the compilation ? Will we create a separate record for that or are we going to detect that it is a record/query we have seen before i.e is the 3rd record above going to be the only one stored in that case ?

vijaysun-omr · 2018-09-28T21:40:32Z

runtime/compiler/runtime/SymbolValidationManager.cpp

@@ -449,6 +475,8 @@ TR::SymbolValidationManager::addClassFromITableIndexCPRecord(TR_OpaqueClassBlock
   {
   if (!clazz)
      return false;
+   if (inHeuristicRegion())


Do we really need to check for heuristic region here ? I thought you were suggesting that we only specify heuristic regions in parts of the code where we are looking at classes in some controlled way, e.g.iterate over subclasses when looking up a single implementor.

I thought you were suggesting that we only specify heuristic regions in parts of the code where we are looking at classes in some controlled way, e.g.iterate over subclasses when looking up a single implementor.

Yes you're right. However, for the sake of consistency, all validation records check whether a heuristic region is active. it is also a proactive means of ensuring that future code that guards other heuristic regions don't have to worry about the kind of validations that might fail within that region since the manager guarantees that all validation records will pass within the region.

dsouzai · 2018-09-28T22:14:34Z

What happens if we get class B by name again in some other spot in the compilation ? Will we create a separate record for that or are we going to detect that it is a record/query we have seen before i.e is the 3rd record above going to be the only one stored in that case ?

Yes, we would create another record associated with that compilation. The reason is because if we acquired B by name via the parameter list of the method being compiled A.foo(LB;), and then in a different compilation, acquired B also by name via the parameter of the method being compiled C.bar(LB;), then in the load run, if A and C are loaded by different class loaders, it is possible for A and C not not agree on B. Therefore, even if B as seen by A and C have the same ROMClass and the same class chains, there could be two different J9Classs - one to describe B as seen by A, and the other to describe B as seen by C.

Having this seemingly redundant record to describe B by name is to guard against the case where A and C agree on B in the compile run, but disagree on B in the load run.

That said, if we were looking up bootstrap classes by name, then the records are definitely redundant because they are never ambiguous.

@jdmpapin anything to add/correct?

The Symbol Validation Manager is used to remember all the different ways the compiler acquired a symbol. However, there are regions where this information isn't necessary for the load run. For example, when asking for something from the Persistent CH Table, all that matters is the input to the query and the output; however, the query involves going through multiple classes before finally returning the answer. If the compiler were to store valiations for everything that occurs during this query, it would require consistency between the compile and load run for more than what is only required for the compilation. Therefore, this commit adds the means to denote regions that are not strictly necessary to validate for the load run. Signed-off-by: Irwin D'Souza <[email protected]>

Some front end queries do not need to add a new validation record under AOT using the symbol validation manager. However, it does need to verify that the new symbol has been validated. This query is used to encapsulate this logic. Signed-off-by: Irwin D'Souza <[email protected]>

This commit adds uses of the new Symbol Validation Manager infrastructure to do AOT validations (under an option). It also introduces the following two changes: 1. Use resolved interpreter dispatch for AOT Previously, because almost everything was "unresolved" under AOT, all calls would use the unresolved dispatch. However, with the new validations, very few unresolved pointers are only unresolved because of AOT. Therefore, generating an unresolved dispatch is incorrect. The solution to this problem is to assume, during an AOT compile, that all methods are interpreted. Thus, if a method is resolved, the compiler will generate a resolved interpreter dispatch snippet, which can patch the caller if/when the method is compiled. 2. Fix ObjectAllocationInlining under AOT Previously, Object Allocation Inlining under AOT maintained certain assumtpions about the trees that was only true if the class was unresolved. This commit fixes those assumptions. Signed-off-by: Irwin D'Souza

Signed-off-by: Irwin D'Souza <[email protected]>

Add query for when the compiler asks about whether a method is interpreted for heuristic reasons, and not for functional correctness reasons. Signed-off-by: Irwin D'Souzai <[email protected]>

Signed-off-by: Irwin D'Souza <[email protected]>

Disable the symbol validation manager during startup or if the method is compiled at cold. Signed-off-by: Irwin D'Souza <[email protected]>

Signed-off-by: Irwin D'Souza <[email protected]>

Replace TR_ASSERT_FATAL with TR_ASSERT and abort the compilation if there are any inconsistencies with the Symbol Validation Manager. Signed-off-by: Irwin D'Souza <[email protected]>

Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai · 2018-10-08T21:26:28Z

@mpirvu Updated the changes; hopefully there are no more linter issues...

mpirvu · 2018-10-08T22:31:55Z

jenkins test sanity all jdk8

mpirvu · 2018-10-09T00:53:15Z

All checks have passed, so merging.

dsouzai changed the title ~~Aot improvements part2~~ Improvements for Relocatable Compilations Part 2 Sep 18, 2018

This was referenced Sep 18, 2018

WIP: Improvements for Relocatable Compilations Part 3 #2920

Closed

Improvements for Relocatable Compilations #2921

Open

dsouzai changed the title ~~Improvements for Relocatable Compilations Part 2~~ WIP: Improvements for Relocatable Compilations Part 2 Sep 18, 2018

dsouzai mentioned this pull request Sep 18, 2018

Improvements for Relocatable Compilations Part 1 #2918

Closed

dsouzai force-pushed the AOTImprovementsPart2 branch 11 times, most recently from 22698e9 to b697ab8 Compare September 25, 2018 03:05

dsouzai force-pushed the AOTImprovementsPart2 branch 4 times, most recently from 983ab5b to 44c3f5b Compare September 28, 2018 14:49

dsouzai changed the title ~~WIP: Improvements for Relocatable Compilations Part 2~~ Improvements for Relocatable Compilations Sep 28, 2018

dsouzai force-pushed the AOTImprovementsPart2 branch from 44c3f5b to 665effa Compare September 28, 2018 15:54

vijaysun-omr reviewed Sep 28, 2018

View reviewed changes

Irwin D'Souza added 14 commits October 8, 2018 16:59

Enable String Peepholes under AOT

97b11cc

Signed-off-by: Irwin D'Souza <[email protected]>

Enable checkcast inlining under AOT

b460431

Signed-off-by: Irwin D'Souza <[email protected]>

Enable class lookahead optimization under AOT

2c2971a

Signed-off-by: Irwin D'Souza <[email protected]>

Improve string builder transformer under AOT

170d53c

Signed-off-by: Irwin D'Souza <[email protected]>

Improve ilgen under AOT

3fb75db

Signed-off-by: Irwin D'Souza <[email protected]>

Add isInterpretedForHeuristics

018bc14

Add query for when the compiler asks about whether a method is interpreted for heuristic reasons, and not for functional correctness reasons. Signed-off-by: Irwin D'Souzai <[email protected]>

Generate fewer validation records

8d62613

Signed-off-by: Irwin D'Souza <[email protected]>

Disable Symbol Validation Manager conditionally

394655f

Disable the symbol validation manager during startup or if the method is compiled at cold. Signed-off-by: Irwin D'Souza <[email protected]>

Enable Symbol Valdation Manager on 64-bit x86

8e2b8fc

Signed-off-by: Irwin D'Souza <[email protected]>

Remove Debug Code

c3f550f

Replace TR_ASSERT_FATAL with TR_ASSERT and abort the compilation if there are any inconsistencies with the Symbol Validation Manager. Signed-off-by: Irwin D'Souza <[email protected]>

Add Documentation

3eaaad0

Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai force-pushed the AOTImprovementsPart2 branch from e80660f to 3eaaad0 Compare October 8, 2018 21:26

mpirvu merged commit 436de15 into eclipse-openj9:master Oct 9, 2018

This was referenced Oct 10, 2018

Misc AOT Improvements #3015

Closed

Enable Check Cast Inlining under AOT #3014

Closed

Enable String Peepholes under AOT #3013

Closed

Exploit Symbol Validation Manager #3012

Closed

Add Symbol Validation Manager #3011

Closed

jdmpapin mentioned this pull request Nov 20, 2018

Make SVM generate few enough records to enable it during startup #3752

Closed

28 tasks

This was referenced Feb 12, 2019

Software Concurrent Scavenge Support on Power #4159

Merged

Use Symbol Validation Manager for Method Symbols #3029

Closed

Don't retry an AOT compilation if it fails for certain reasons #3033

Closed

Enable Symbol Validation Manager on all platforms #3028

Open

dsouzai deleted the AOTImprovementsPart2 branch October 15, 2019 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements for Relocatable Compilations #2919

Improvements for Relocatable Compilations #2919

dsouzai commented Sep 18, 2018 •

edited

Loading

vijaysun-omr Sep 28, 2018

dsouzai Sep 28, 2018

vijaysun-omr Sep 28, 2018 •

edited

Loading

dsouzai Sep 28, 2018

vijaysun-omr Sep 28, 2018

dsouzai Sep 28, 2018

jdmpapin Sep 28, 2018 •

edited

Loading

vijaysun-omr Sep 28, 2018

dsouzai Sep 28, 2018

jdmpapin Sep 28, 2018

vijaysun-omr Sep 29, 2018

dsouzai Sep 30, 2018 •

edited

Loading

vijaysun-omr Sep 30, 2018

vijaysun-omr Sep 28, 2018

dsouzai Sep 28, 2018

mstoodle Oct 2, 2018

vijaysun-omr commented Sep 28, 2018

vijaysun-omr Sep 28, 2018

dsouzai Sep 28, 2018

dsouzai commented Sep 28, 2018

dsouzai commented Oct 8, 2018

mpirvu commented Oct 8, 2018

mpirvu commented Oct 9, 2018

Improvements for Relocatable Compilations #2919

Improvements for Relocatable Compilations #2919

Conversation

dsouzai commented Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijaysun-omr Sep 28, 2018 • 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

jdmpapin Sep 28, 2018 • 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

dsouzai Sep 30, 2018 • 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

vijaysun-omr commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsouzai commented Sep 28, 2018

dsouzai commented Oct 8, 2018

mpirvu commented Oct 8, 2018

mpirvu commented Oct 9, 2018

dsouzai commented Sep 18, 2018 •

edited

Loading

vijaysun-omr Sep 28, 2018 •

edited

Loading

jdmpapin Sep 28, 2018 •

edited

Loading

dsouzai Sep 30, 2018 •

edited

Loading