Generate instanceOf checks in Block Versioner for index field objects and introduce throttling heuristics #4598

klangman · 2019-11-26T15:36:32Z

When versioning blocks, it's possible for the generated checks to load
index variables from objects by duplicating code from within blocks that
are protected by inlining guards. Therefore the object might not be of
the expected type possibly resulting in loads that reach past the end
of the object. In rare cases this load can result in a SEGV when the
object is allocated at the very end of the heap. In addition, this change
will abort versioning of blocks when the estimated cost of executing the
required tests is higher then the savings from removing bound-checks.

Signed-off-by: Kevin Langman langman@ca.ibm.com

klangman · 2019-11-26T15:37:39Z

@vijaysun-omr - This is the Block Versioner fix we talked about earlier.

vijaysun-omr · 2019-11-26T19:29:02Z

I would like a review from @andrewcraik for this change.

vijaysun-omr · 2019-11-28T00:03:14Z

I would suggest making sure we have more than 3 bound checks being versioned against in the case when we add one instanceof (and maybe more than 4 bound checks in the case when we add 2 instanceofs). I have a hard time believing the block versioning tranformation would be a net win when replacing a bound check with an instanceof.

klangman · 2019-11-28T16:18:03Z

I would suggest making sure we have more than 3 bound checks being versioned against in the case when we add one instanceof

One issue in implementing that suggestion is the fact that we rely on a future VP pass to remove unnecessary instanceof checks that this code is adding to the IL. So, we don't know how many of the new instanceof checks are going to survive VP and therefore can't make an accurate decision on when to abandon the opt for fear of making performance worse.

vijaysun-omr · 2019-11-28T16:58:02Z

While it's certainly possible that some change in the IL trees means that only a future VP pass is able to derive some information necessary for eliminating the instanceof, it is also the case that the checking could be done in the pass of VP that is doing block versioner transformation i.e. if we know that the instanceof is not needed, then why emit it in the first place (i.e. in most cases, we ought to have enough information in the current pass of VP.

andrewcraik · 2019-12-02T15:55:03Z

compiler/optimizer/OMRValuePropagation.cpp


            temp.add(nextComparisonNode);
+
+            if (arrayIndex->_baseNode && arrayIndex->_instanceOfClass && !comp()->compileRelocatableCode() && 


The !compileRelocatableCode seems suboptimal - if we can relocate the class we can do the opt. @dsouzai is there an easy way with the new infrastructure to allow this to work under AOT?

// InstanceOf check for the object were we load the array index from TR::Node *objectRefChild = arrayIndex->_baseNode->getFirstChild();

In order for this tree to have been created, I'm guessing we must have made a frontend query to know the instanceof relationship. If that's true, then the validation would've already been created for AOT. If using non-SVM AOT, then the frontend query would've returned NULL, in which case the compiler would not know any instanceof information.

As such, I'm not sure if the AOT guard is needed. That said, if there really is a concern, you can always make a call to the frontend to do the instanceof check, and if it passes, then go down the code path.

Any reply to this @klangman ?

The instanceOf-check generated below (one that existed before I changed anything here) has this AOT check as well. I can't say I fully understand why, it seems to me that the instanceOf-checks are needed for correctness. I would be happy to disable this for AOT, but I guess we might need someone from AOT land to help understand why we can get away without the instanceOf-check in the case of AOT.

Hmm - if the check is for correctness I can't see why they can be omitted for AOT without risking violating the correctness. Would you agree @dsouzai ?

I had a phone call with @klangman to understand the full picture here, and we both agreed that the old code that didn't emit the instanceof checks under AOT was wrong. That said, I know why the code was guarded:

TR::Node *duplicateClassPtr = TR::Node::createWithSymRef(objectRefChild, TR::loadaddr, 0, comp()->getSymRefTab()->findOrCreateClassSymbol(objectRef->getSymbolReference()->getOwningMethodSymbol(comp()), -1, arrayLength->_instanceOfClass, false));

passes in a cpIndex of -1, which non SVM AOT can't handle. My guess is that whoever implemented this saw weird crashes under AOT and did the usual "I'm just gonna disable this under AOT".

Technically there should be no problem in SVM AOT; however, to keep the scope of this PR from getting bigger and bigger, both Kevin and I agreed that the best course for now is to prevent versioning if AOT && instanceOf checks were added.

In a future PR, we can enable versioning in AOT when the SVM is enabled.

andrewcraik · 2019-12-02T16:13:40Z

Given the higher cost of an instanceof some kind of heuristic should be applied to make sure the benefit of versioning is high enough to make the instanceof worth it overall - as Vijay suggested. We should also have reasonable confidence that the test should pass at runtime when we create it. There should be tracing to document this choice and why.

The name indexInstanceOfClass really threw me - the class is the element type right? It certainly isn't the class of the index which is what the name suggested. Could you try and generate a better name for the variable to better convey what the class is that you are talking about?

klangman · 2019-12-04T20:12:53Z

The name indexInstanceOfClass really threw me - the class is the element type right?

@andrewcraik It's the class of the object where the index is being loaded from. Maybe "indexObjInstanceOfClass" would be less confusing?

andrewcraik · 2019-12-04T20:36:52Z

how about indexSourceObjectClass or something like that?

andrewcraik · 2019-12-11T14:53:37Z

@klangman any update on a rename so we can keep this moving forward since the issue it is addressing is real...

klangman · 2019-12-11T16:04:47Z

@klangman any update on a rename so we can keep this moving forward since the issue it is addressing is real...

@andrewcraik Other issues getting in my way. Hope to get back to this in a few days.

andrewcraik · 2020-02-26T14:59:52Z

@klangman any further updates on this?

klangman · 2020-02-26T21:15:59Z

@klangman any further updates on this?

Still looking to find the time to get back to this one.

klangman · 2020-03-17T15:41:35Z

@vijaysun-omr @andrewcraik Pushed changes to address your comments (I hope). It now does compile-time instanceOf checks, does not generate redundant instanceOf checks and aborts versioning of blocks where the cost of the required tests is to high.

vijaysun-omr · 2020-03-18T17:20:52Z

Thanks @klangman could you please update the commit and PR messages to reflect the changes that have been added in ? i.e. around the heuristic for when to allow the versioning to proceed. This was not part of the original change in this PR and so we do not have a description of it I believe.

klangman · 2020-03-18T17:38:17Z

Thanks @klangman could you please update the commit and PR messages to reflect the changes that have been added in?

Done!

vijaysun-omr · 2020-03-18T18:23:24Z

Thanks, please update the commit message too.

klangman · 2020-03-18T18:49:45Z

Thanks, please update the commit message too.

Better now?

vijaysun-omr · 2020-03-18T19:24:01Z

@genie-omr build all

vijaysun-omr · 2020-03-18T19:24:42Z

I will wait for a review from @andrewcraik to see if his earlier comments have been addressed (and the overall logic change).

vijaysun-omr · 2020-03-20T19:02:59Z

friendly reminder, @andrewcraik to review this

andrewcraik · 2020-03-25T14:52:08Z

compiler/optimizer/OMRValuePropagation.cpp

+                  TR::Node *instanceofNode = TR::Node::createWithSymRef(TR::instanceof, 2, 2, objectRefChild->duplicateTree(),  duplicateClassPtr, comp()->getSymRefTab()->findOrCreateInstanceOfSymbolRef(comp()->getMethodSymbol()));
+                  TR::Node *ificmpeqNode =  TR::Node::createif(TR::ificmpeq, instanceofNode, TR::Node::create(objectRef, TR::iconst, 0, 0));
+                  comparisonNodes->add(ificmpeqNode);
+                  requestOpt(OMR::localCSE,  true);


Should these be requested before we commit to doing the transform below?

andrewcraik · 2020-03-25T14:52:27Z

compiler/optimizer/OMRValuePropagation.cpp

+                     TR::Node *instanceofNode = TR::Node::createWithSymRef(TR::instanceof, 2, 2, indexObjectRefChild->duplicateTree(),  duplicateClassPtr, comp()->getSymRefTab()->findOrCreateInstanceOfSymbolRef(comp()->getMethodSymbol()));
+                     TR::Node *ificmpeqNode =  TR::Node::createif(TR::ificmpeq, instanceofNode, TR::Node::create(arrayIndex->_baseNode, TR::iconst, 0, 0));
+                     temp.add(ificmpeqNode);
+                     requestOpt(OMR::localCSE,  true);


Same should these be requested before we commit below?

I wonder if requesting the opts are redundant now that this code will only add instanceOf checks when a compile time instanceOf is not possible or fails.

If they were only added to clean up needless instanceofs then they may well be able to be omitted...

andrewcraik · 2020-03-25T14:57:11Z

I think we should have a debug envvar to disable these additional tests for performance debugging purposes in case we see any regressions from these additional tests. It would also be good to have the scaling factor for the number of instanceof checks and number of null tests pulled out into static local variables which can be overridden with an env var if necessary.

andrewcraik · 2020-03-27T15:09:54Z

@klangman any update since I know you were keen to try and get this merged?

klangman · 2020-03-27T15:23:02Z

@klangman any update since I know you were keen to try and get this merged?

I will move the opt requests so they they are only requested when we decide not to abort the compile. But what do you think we should do about the AOT case? Do we more data to decide?

vijaysun-omr · 2020-03-27T20:45:13Z

@genie-omr build all

andrewcraik · 2020-03-30T12:21:41Z

The question about AOT is not a case of performance - if the instance of test is not done we could end up reading past the end of an object/array which could a) cause the VM to crash if the object were allocated at the end of a page of memory without the next page being allocated or b) cause the VM to read past the end of an object/array which could be a security vulnerability. I can't think of a reason why we would need an instance of check when compiling in process and not need the same check when running AOT code - AOT needs more checks not less.

Thoughts @klangman and @dsouzai ?

klangman · 2020-03-30T12:53:49Z

AOT needs more checks not less.

Yes, I think Irwin and I agree on that. We are just suggesting that we abort versioning of the block when doing an AOT compile ( compileRelocatableCode() )

andrewcraik · 2020-03-30T14:44:54Z

I am fine with that since anything performance critical should get recompiled. Any concerns @vijaysun-omr ?

vijaysun-omr · 2020-03-30T14:59:17Z

That sounds okay to me, since there is a decent chance of recompilation out of AOT as Andrew suggests but even otherwise, I feel we could take the approach of waiting for an actual opportunity (that I feel will be somewhat unlikely) where this optimization is important to do in AOT compilations before doing the work to make that combination work.

klangman · 2020-03-30T15:41:53Z

OK.. Sounds like we are all on the same page regarding AOT. I'll make the changes (hopefully today).

klangman · 2020-03-30T16:26:59Z

Oh... To be clear, the change will be, if we need instanceOf checks and compileRelocatableCode() then abort versioning the block. Without instanceOf checks there are no AOT issues to worry about.

When versioning blocks, it's possible for the generated checks to load index variables from objects by duplicating code from within blocks that are protected by inlining guards. Therefore the object might not be of the expected type possibly resulting in loads that reach past the end of the object. In rare cases this load can result in a SEGV when the object is allocated at the very end of the heap. In addition, this change will abort versioning of blocks when the estimated cost of executing the required tests is higher then the savings from removing bound-checks. Signed-off-by: Kevin Langman <langman@ca.ibm.com>

klangman · 2020-03-30T23:14:24Z

@andrewcraik @vijaysun-omr Pushed the changes to abort versioning a block when it's an AOT compile and we would have needed to add an instanceOf check.

andrewcraik · 2020-03-30T23:49:20Z

I think that makes sense - over to @vijaysun-omr for final review

vijaysun-omr · 2020-03-31T00:13:48Z

@genie-omr build all

vijaysun-omr · 2020-03-31T11:56:55Z

@genie-omr build all

vijaysun-omr · 2020-03-31T13:00:05Z

Looks comprehensive to me after all the reviews. Merging.

klangman requested review from andrewcraik and vijaysun-omr as code owners November 26, 2019 15:36

0xdaryl added the comp:compiler label Nov 26, 2019

vijaysun-omr self-assigned this Nov 26, 2019

andrewcraik reviewed Dec 2, 2019

View reviewed changes

fjeremic mentioned this pull request Dec 3, 2019

Committer vote on using clang-format to reformat the OMR repository #4631

Closed

klangman force-pushed the 142541 branch from 0c0ed40 to 3d99f0a Compare March 17, 2020 15:14

klangman changed the title ~~Generate instanceOf checks in Block Versioner for index field objects~~ Generate instanceOf checks in Block Versioner for index field objects and introduce throttling heuristics Mar 18, 2020

klangman force-pushed the 142541 branch 2 times, most recently from 19d70e7 to 1c5c04a Compare March 18, 2020 18:48

andrewcraik reviewed Mar 25, 2020

View reviewed changes

klangman force-pushed the 142541 branch from 1c5c04a to 1ec65af Compare March 27, 2020 16:49

klangman force-pushed the 142541 branch from 1ec65af to 5ca2707 Compare March 30, 2020 23:11

vijaysun-omr approved these changes Mar 31, 2020

View reviewed changes

vijaysun-omr merged commit d2d585f into eclipse-omr:master Mar 31, 2020

klangman mentioned this pull request Apr 2, 2020

Build-Break fix caused by Pull Request 4598 #5013

Merged

fjeremic mentioned this pull request Apr 7, 2020

Crash in OMR::ValuePropagation::buildBoundCheckComparisonNodes in a PR build eclipse-openj9/openj9#9115

Closed


		temp.add(nextComparisonNode);

		if (arrayIndex->_baseNode && arrayIndex->_instanceOfClass && !comp()->compileRelocatableCode() &&

Generate instanceOf checks in Block Versioner for index field objects and introduce throttling heuristics #4598

Generate instanceOf checks in Block Versioner for index field objects and introduce throttling heuristics #4598

Uh oh!

Conversation

klangman commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klangman commented Nov 26, 2019

Uh oh!

vijaysun-omr commented Nov 26, 2019

Uh oh!

vijaysun-omr commented Nov 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klangman commented Nov 28, 2019

Uh oh!

vijaysun-omr commented Nov 28, 2019

Uh oh!

andrewcraik Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsouzai Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

andrewcraik commented Dec 2, 2019

Uh oh!

klangman commented Dec 4, 2019

Uh oh!

andrewcraik commented Dec 4, 2019

Uh oh!

andrewcraik commented Dec 11, 2019

Uh oh!

klangman commented Dec 11, 2019

Uh oh!

andrewcraik commented Feb 26, 2020

Uh oh!

klangman commented Feb 26, 2020

Uh oh!

klangman commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vijaysun-omr commented Mar 18, 2020

Uh oh!

klangman commented Mar 18, 2020

Uh oh!

vijaysun-omr commented Mar 18, 2020

Uh oh!

klangman commented Mar 18, 2020

Uh oh!

vijaysun-omr commented Mar 18, 2020

Uh oh!

vijaysun-omr commented Mar 18, 2020

Uh oh!

vijaysun-omr commented Mar 20, 2020

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!

andrewcraik commented Mar 25, 2020

Uh oh!

andrewcraik commented Mar 27, 2020

Uh oh!

klangman commented Mar 27, 2020

Uh oh!

vijaysun-omr commented Mar 27, 2020

klangman commented Nov 26, 2019 •

edited

Loading

vijaysun-omr commented Nov 28, 2019 •

edited

Loading

andrewcraik Dec 2, 2019 •

edited

Loading

dsouzai Dec 2, 2019 •

edited

Loading

klangman commented Mar 17, 2020 •

edited

Loading