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

Add recompilation test in loop in jProfiling #2348

Merged
merged 1 commit into from
Jul 17, 2018
Merged

Add recompilation test in loop in jProfiling #2348

merged 1 commit into from
Jul 17, 2018

Conversation

r30shah
Copy link
Contributor

@r30shah r30shah commented Jul 9, 2018

When jProfiling in compilation is enabled, add a test after aynchcheck node in loop to calculate the frequency and trip up recompilation of the method if reached certain threshold.

@r30shah r30shah changed the title WIP: Add recompilation test pr WIP: Add recompilation test in loop in jProfiling Jul 9, 2018
@r30shah r30shah force-pushed the AddRecompilationTestPR branch from 67df786 to 90298b2 Compare July 9, 2018 12:47
@r30shah
Copy link
Contributor Author

r30shah commented Jul 9, 2018

This PR depends on eclipse-omr/omr#2731

@r30shah
Copy link
Contributor Author

r30shah commented Jul 9, 2018

@andrewcraik @fjeremic Can you please review this.

@r30shah r30shah mentioned this pull request Jul 9, 2018
Merged
@fjeremic
Copy link
Contributor

fjeremic commented Jul 9, 2018

@r30shah can you please fix the copyright checks?

@andrewcraik andrewcraik added the depends:omr Pull request is dependent on a corresponding change in OMR label Jul 9, 2018
@r30shah
Copy link
Contributor Author

r30shah commented Jul 9, 2018

@fjeremic this complains about copyright issue with runtime/compiler/runtime/J9Profiler.cpp .
As I have fixed that in PR, if I change copyright here as well then it will cause merge conflicts. I have kept both PR as WIP. I will update them and remove WIP in sequence s.t we can avoid conflicts.

@r30shah r30shah force-pushed the AddRecompilationTestPR branch from 3296fd8 to cfd65b3 Compare July 10, 2018 17:56
@r30shah r30shah force-pushed the AddRecompilationTestPR branch from c220a48 to 849a86a Compare July 11, 2018 15:40
@r30shah
Copy link
Contributor Author

r30shah commented Jul 11, 2018

@andrewcraik Tried to address most of the comments via separate commits and comments. Let me know if I am missing something.

@r30shah r30shah force-pushed the AddRecompilationTestPR branch from 5e68232 to 7a7af8c Compare July 12, 2018 19:21
@r30shah r30shah force-pushed the AddRecompilationTestPR branch from 9b00b65 to d95ed63 Compare July 12, 2018 20:51
@andrewcraik
Copy link
Contributor

@r30shah can you please fix the copyright check and address the last few comments. Code looks good. Once done could you ask @fjeremic or another committer to launch sanity? I will try and check tomorrow night to review any revisions and hopefully merge this.

@r30shah
Copy link
Contributor Author

r30shah commented Jul 13, 2018

@andrewcraik Thanks a lot for the review. Addressing your comments will be my first priority in the morning.
I will squash commits as well.

@r30shah r30shah force-pushed the AddRecompilationTestPR branch from 113d828 to 6245458 Compare July 13, 2018 12:23
@r30shah
Copy link
Contributor Author

r30shah commented Jul 13, 2018

This can be merged after merging #2346. That pull request contains some of the supporting functions used here. Current Copyright failure is in runtime/compiler/runtime/J9Profiler.cpp which is also addressed in above PR so once that is merged , I will rebase ans squash all commits.

@r30shah r30shah changed the title WIP: Add recompilation test in loop in jProfiling Add recompilation test in loop in jProfiling Jul 13, 2018
@r30shah r30shah changed the title Add recompilation test in loop in jProfiling WIP: Add recompilation test in loop in jProfiling Jul 13, 2018
@r30shah
Copy link
Contributor Author

r30shah commented Jul 13, 2018 •
edited
Loading

@andrewcraik I made all changes suggested by you in last comments. Thanks for clarifying about the improper regions.
I will wait for #2346 to be merged and then squash commits and remove WIP

@andrewcraik
Copy link
Contributor

@fjeremic can you handle this one too - I think the code should be good - if could give it a quick once over once updated the merge I would be much obliged.

@fjeremic fjeremic self-assigned this Jul 14, 2018
@r30shah
Add jProfilingRecompilation test in loop
d59c5e4 
When jProfiling in compilation is enabled, add a test after aynchcheck node
in loop to calculate the frequency and trip up recompilation of the method
if reached certain threshold.

Signed-off-by: Rahil Shah <rahil@ca.ibm.com>
@r30shah r30shah force-pushed the AddRecompilationTestPR branch from 6245458 to d59c5e4 Compare July 16, 2018 21:41
@r30shah
Copy link
Contributor Author

r30shah commented Jul 16, 2018

@fjeremic I have rebased and squashed all the commits. Can you please take a look?

@r30shah r30shah changed the title WIP: Add recompilation test in loop in jProfiling Add recompilation test in loop in jProfiling Jul 16, 2018
@fjeremic
Copy link
Contributor

Approving based off @andrewcraik's review.

@fjeremic
Copy link
Contributor

Jenkins test sanity

@fjeremic
Copy link
Contributor

Windows failures due to #2129.

@fjeremic fjeremic merged commit f497aa4 into eclipse-openj9:master Jul 17, 2018
@r30shah r30shah deleted the AddRecompilationTestPR 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 left review comments

@mstoodle mstoodle mstoodle left review comments

@fjeremic fjeremic fjeremic approved these changes

Assignees

@fjeremic fjeremic

Labels
comp:jit depends:omr Pull request is dependent on a corresponding change in OMR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@r30shah @fjeremic @andrewcraik @mstoodle