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.

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

Change Java native methods to static #11882

Closed
wants to merge 4 commits into from

Conversation

rhubner
Copy link
Contributor

@rhubner rhubner commented Sep 25, 2023

This should give us some performance benefit calling native C++ code.
Closes #4786
See https://github.com/evolvedbinary/jni-benchmarks/ for more info.

@rhubner rhubner changed the title Change native methods to static Change Java native methods to static Sep 25, 2023
Copy link
Contributor

@alanpaxton alanpaxton left a comment

Choose a reason for hiding this comment

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

LGTM. Appears on reading through to do what is intended, and I am not aware of any omissions. Are we sure that UTs are comprehensive, i.e. all native methods are covered by the UTs ? Could we look at having a ticket based on #11221 which ensures that ? (not something we can do now, just an idea for reference)

@rhubner rhubner marked this pull request as ready for review October 4, 2023 06:03
@alanpaxton alanpaxton force-pushed the jni-static branch 2 times, most recently from c78147f to 1c00673 Compare October 25, 2023 16:45
@rhubner rhubner force-pushed the jni-static branch 2 times, most recently from 915c1c1 to f1f69f3 Compare December 20, 2023 07:51
@adamretter
Copy link
Collaborator

@rhubner Can we get this rebased please?

Copy link
Collaborator

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

LGTM. @rhubner can you rebase please? @ajkr @pdillinger the changes in this PR are mostly mechanical and follow the same single pattern over-and-over again, as soon as it is rebased and CI passes can we get this one merged please?

@adamretter
Copy link
Collaborator

@ajkr @pdillinger can we get this imported ASAP please?

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ajkr merged this pull request in 0bf9079.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate performance of static vs instance JNI methods
5 participants