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

Remove Fast_java_lang_System_arraycopy #17140

Conversation

ehrenjulzert
Copy link

@ehrenjulzert ehrenjulzert commented Apr 10, 2023

Fast_java_lang_System_arraycopy is dead code,
as it is no longer called anywhere

@ehrenjulzert
Copy link
Author

@tajila Do you know if there's a reason FastJNI_java_lang_System.cpp exists? It doesn't even get compiled when I compile j9 normally, since it disables itself right at the start with #undef FAST_JNI_ARRAYCOPY

@tajila
Copy link
Contributor

tajila commented Apr 10, 2023

are currentmillis and nanotime still used?

@tajila
Copy link
Contributor

tajila commented Apr 10, 2023

@gacholio may know why FAST_JNI_ARRAYCOPY is undefined

@pshipton
Copy link
Member

pshipton commented Apr 10, 2023

The #undef was added by GAC in 2013 by
39436: More Fast JNI implementations - clean up code and implement Throwable.fillInStackTraceJ9VMInternals.getClassLoader, Array.newArrayImpl, String.intern, System.arraycopy (currently disabled due to JIT issue).

39436 is a Discussion item in RTC.

@ehrenjulzert
Copy link
Author

are currentmillis and nanotime still used?

Good question, originally I thought they weren't used because there's different implementations in BytecodeInterpreter.hpp, but I just realized that the other FastJNI_* files also have their functions implemented elsewhere. When are the fast versions used?

@gacholio
Copy link
Contributor

When are the fast versions used?

The fast versions are used by jitted code, not by the interpreter/VM. The System natives are referenced here:

J9_FAST_JNI_METHOD_TABLE_EXTERN(java_lang_System);

J9_FAST_JNI_CLASS("java/lang/System", java_lang_System)

@gacholio
Copy link
Contributor

I do not recall what the issue with arraycopy was/is. It's possible these natives are not necessary as the JIT has custom inlined verisons of them, but we would need to prove that with performance measurements across the platforms.

I think this item should be closed with no action taken.

@ehrenjulzert
Copy link
Author

The main reason I brought this up was because I'm editing System.arraycopy in #17048, and I figured that if one arrayCopy implementation is changed then Fast_java_lang_System_arraycopy should also either be updated or removed completely. I could also just not do anything to Fast_java_lang_System_arraycopy, but it seems like that could be a bad idea since it leaves it in kind of an unusable state and doesn't make it obvious that it's out of date. Which option do you think makes the most sense?

@gacholio
Copy link
Contributor

The fast version has been disabled going on 10 years now, so I'm fine with removing it - it would be simple enough to reimplement it if a need arises later.

@ehrenjulzert
Copy link
Author

Okay, I'll update this PR to just remove Fast_java_lang_System_arraycopy and keep the other functions.

Fast_java_lang_System_arraycopy is dead code,
as it is no longer called anywhere

Signed-off-by: Ehren Julien-Neitzert <ehren.julien-neitzert@ibm.com>
@ehrenjulzert ehrenjulzert force-pushed the delete_Fast_java_lang_System_arraycopy branch from 469f137 to ffa236f Compare April 11, 2023 19:49
@ehrenjulzert
Copy link
Author

Okay, only Fast_java_lang_System_arraycopy is removed now

@gacholio
Copy link
Contributor

Please update the PR description.

@gacholio
Copy link
Contributor

jenkins test sanity zlinux jdk19

@ehrenjulzert ehrenjulzert changed the title Remove FastJNI_java_lang_System.cpp Remove Fast_java_lang_System_arraycopy Apr 12, 2023
@gacholio gacholio merged commit b831fcc into eclipse-openj9:master Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants