Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Comment and reword assert in Recompilation for jProfiling #2349

Merged
merged 1 commit into from
Jul 13, 2018
Merged

Comment and reword assert in Recompilation for jProfiling #2349

merged 1 commit into from
Jul 13, 2018

Conversation

r30shah
Copy link
Contributor

@r30shah r30shah commented Jul 9, 2018 •
edited

jProfiling should not use either sampling or counting to trip recompilation. For this kind of method bodies, we have emitted recompilation test to check method invocation count and loop frequency against tuned threshold to trip recompilation.

Signed-off-by: Rahil Shah rahil@ca.ibm.com

@r30shah
Copy link
Contributor Author

r30shah commented Jul 9, 2018

@andrewcraik @fjeremic This PR contains changes related to switching to counting based recompilation for jProfiling body similar to JIT Profiling. Can you please review this?

andrewcraik
andrewcraik reviewed Jul 9, 2018
View reviewed changes
runtime/compiler/p/codegen/PPCRecompilation.cpp Outdated
@@ -167,7 +167,7 @@ TR::Instruction *TR_PPCRecompilation::generatePrologue(TR::Instruction *cursor)
else
{
// This only applies to JitProfiling, as JProfiling uses sampling
TR_ASSERT(_compilation->getProfilingMode() == JitProfiling, "JProfiling should use sampling to trigger recompilation");
//TR_ASSERT(_compilation->getProfilingMode() == JitProfiling, "JProfiling should use sampling to trigger recompilation");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't comment out asserts - if it is supposed to be removed please delete it

andrewcraik
andrewcraik reviewed Jul 9, 2018
View reviewed changes
runtime/compiler/x/codegen/X86Recompilation.cpp Outdated
@@ -182,7 +182,7 @@ TR::Instruction *TR_X86Recompilation::generatePrologue(TR::Instruction *cursor)
else
{
// This only applies to JitProfiling, as JProfiling uses sampling
TR_ASSERT(_compilation->getProfilingMode() == JitProfiling, "JProfiling should use sampling to trigger recompilation");
//TR_ASSERT(_compilation->getProfilingMode() == JitProfiling, "JProfiling should use sampling to trigger recompilation");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

andrewcraik
andrewcraik reviewed Jul 9, 2018
View reviewed changes
runtime/compiler/control/J9Recompilation.cpp Outdated
@@ -207,7 +207,8 @@ J9::Recompilation::beforeOptimization()
//
if (self()->isProfilingCompilation()) // this asks the bodyInfo
{
_useSampling = _compilation->getProfilingMode() != JitProfiling;
//_useSampling = _compilation->getProfilingMode() != JitProfiling;
_useSampling = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't leave old code around - if this should be false make it false.

@andrewcraik
Copy link
Contributor

The expert on compilation control is @mpirvu so we should get his thoughts on this change. It seems reasonable to me, but I am too close to the change to make an objective assessment.

@andrewcraik
Copy link
Contributor

@r30shah please fix the copyright check

@r30shah r30shah force-pushed the UseCountingRecompilationPR branch from 68fbbf1 to ce84ae6 Compare July 9, 2018 13:33
@andrewcraik
Copy link
Contributor

@mpirvu I think a review from you on this would be best.

@mpirvu
Copy link
Contributor

mpirvu commented Jul 9, 2018

On the surface the code looks good, but I remember that long time ago we wanted to retire counting compilations completely.
What is the problem that this commit tries to solve? The recompilation is triggered with jitRetranslateCallerWithPrep and not based on actual counting. If sampling gets in the way of the logic we could still create a "sampling" body (the choice between "sampling" and "counting" changes the prologue of the method) but mark the sampling as disabled.

@andrewcraik
Copy link
Contributor

@mpirvu So JProfiling wants to use its own counters to determine when the method should be recompiled. We don't want sampling to see the slower profiling body and trigger a compilation before we are ready if that makes sense.

@mpirvu
Copy link
Contributor

mpirvu commented Jul 9, 2018

Sampling for "sampling body" can be disabled with bodyInfo->setDisableSampling(true); we do this in a few places in the code.
Note that I am not opposed to using counting bodies; I just don't find a compelling reason to do so.

@r30shah
Copy link
Contributor Author

r30shah commented Jul 9, 2018 •
edited

Looking in to the J9Recompilation code : https://github.com/eclipse/openj9/blob/91b1c6a3bfe90390f4fdc59f230794be2e692b17/runtime/compiler/control/J9Recompilation.cpp#L297
I am seeing that for the jProfiling compilations we do disable sampling, so We do not need to set _useSampling to false (It will generate counting body which is bigger in size). The only thing we need to change is the assert Statement in the x86 and PPC prePrologue functions which is misleading as jProfiling should only rely on the test added in the method bodies to trip recompilation.
I believe this would reduce the size of method body as well. Rather than closing this PR, going to make changes in the Assert statements.

@r30shah r30shah force-pushed the UseCountingRecompilationPR branch 3 times, most recently from 12ba774 to 708f02b Compare July 10, 2018 13:49
andrewcraik
andrewcraik approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@andrewcraik andrewcraik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewcraik
Copy link
Contributor

@r30shah please fix the copyright issue so this can be merged

@andrewcraik
Copy link
Contributor

@mpirvu could you handle this one since I'm going to be away?

@r30shah r30shah force-pushed the UseCountingRecompilationPR branch from 708f02b to 15f1fb0 Compare July 13, 2018 11:52
@r30shah
Comment and reword assert in Recompilation for jProfiling
9163a4f 
jProfiling should not use either sampling or counting to trip
recompilation. For this kind of method bodies, we have emitted
recompilation test to check method invocation count and loop
frequency against tuned threshold to trip recompilation.

Signed-off-by: Rahil Shah <rahil@ca.ibm.com>
@r30shah r30shah force-pushed the UseCountingRecompilationPR branch from 15f1fb0 to 9163a4f Compare July 13, 2018 11:58
@r30shah
Copy link
Contributor Author

r30shah commented Jul 13, 2018

Fixwed copyright and also title. This just rewords the Assert we had in Recompilation for both x86 and PPC platform in jProfiling.

@mpirvu mpirvu self-assigned this Jul 13, 2018
@mpirvu
Copy link
Contributor

mpirvu commented Jul 13, 2018

jenkins test all

@r30shah r30shah changed the title Use Counting Recomp for JProfiling Comment and reword assert in Recompilation for jProfiling Jul 13, 2018
@r30shah
Copy link
Contributor Author

r30shah commented Jul 13, 2018

Windows error is because of #2129

@mpirvu mpirvu merged commit 0f74948 into eclipse-openj9:master Jul 13, 2018
@r30shah r30shah deleted the UseCountingRecompilationPR branch July 23, 2018 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewcraik andrewcraik andrewcraik approved these changes

Assignees

@mpirvu mpirvu

Labels
comp:jit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@r30shah @andrewcraik @mpirvu @fjeremic