JDK-8343703 : Symbol name cleanups after JEP 479
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Priority: P4
  • Status: In Progress
  • Resolution: Unresolved
  • Submitted: 2024-11-06
  • Updated: 2024-11-26
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 24
24Unresolved
Related Reports
Relates :  
JDK-8339783 - Implement JEP 479: Remove the Windows 32-bit x86 Port
Relates :  
JDK-8345012 - os::build_agent_function_name potentially wastes a byte when allocating the buffer
Description
After JEP 479 is implemented (or more precisely, JDK-8339783 is integrated), the handling of certain symbol lookup code can be simplified. The old code needed to support 32-bit Windows, where names had a trailing `@<number>`. When this special case now is removed, some streamlining is possible. Specifically these arrays:

#define JNI_ONLOAD_SYMBOLS   {"JNI_OnLoad"}
#define JNI_ONUNLOAD_SYMBOLS {"JNI_OnUnload"}
#define JVM_ONLOAD_SYMBOLS      {"JVM_OnLoad"}
#define AGENT_ONLOAD_SYMBOLS    {"Agent_OnLoad"}
#define AGENT_ONUNLOAD_SYMBOLS  {"Agent_OnUnload"}
#define AGENT_ONATTACH_SYMBOLS  {"Agent_OnAttach"}

are all singletons and so the actual strings can just be inlined directly into the code that uses them.
Comments
A pull request was submitted for review. Branch: master URL: https://git.openjdk.org/jdk/pull/22380 Date: 2024-11-26 06:36:44 +0000
26-11-2024
I agree the os_windows.cpp change can be split out on its own. > Also, it looks like all callers of findJniFunction ensure the cname argument is non-null. No they pass either a real cname or else NULL e.g JNI_OnLoad = (JNI_OnLoad_t)findJniFunction(env, handle, isBuiltin ? cname : NULL, JNI_TRUE); So this change boils down to removing the arrays and directly inlining the applicable string constants at their individual call sites.
26-11-2024
Moving to hotspot/runtime. Changes to libjava is arguably core-libs territory; delegating the decision to split this issue into two, or to let NativeLibrary.c changes "tag along" to whomever fixes this bug.
08-11-2024
The jvm.h cleanup probably ought to be separate from the two string concat changes.
06-11-2024
There are essentially identical calculations of the new concatenated string length in the refered to code in os_windows.cpp and in NativeLibraries.c. In the NativeLibraries.c case, it looks like some existing conditionalization isn't needed, because the relevant component is never null, by design. In the os_windows.cpp case, I didn't check whether a similar situation holds. It could be that there's no space wastage there after all, if the relevant component is never null.
06-11-2024
More specifically, [~kbarrett] pointed out three areas during code review: In src/hotspot/os/windows/os_windows.cpp, around line 5863: https://github.com/openjdk/jdk/pull/21744#discussion_r1828969105 "[pre-existing, and can't comment on line 5858 because it's not sufficiently near a change.] The calculation of len is wasting a byte when lib_name is null. The +2 accounts for the terminating NUL and the underscore separator between the sym_name part and the lib_name part. That underscore isn't added when there isn't a lib_name part. I think the simplest fix would be to change name_len to (name_len +1) and +2 to +1 in that calculation. And add some commentary. This might be deemed not worth fixing as there is likely often no actual wastage, due to alignment padding, and it slightly further complicates the calculation. But additional commentary would still be desirable, to guide the next careful reader. In which case it might be simpler to describe the fixed version." In src/hotspot/share/include/jvm.h, around line 1162: https://github.com/openjdk/jdk/pull/21744#discussion_r1829478432 "There is more cleanup that can be done here. These definitions are used as array initializers (hence the surrounding curly braces). They are now always singleton, rather than sometimes having 2 elements. The uses iterate over the now always singleton arrays. Those iterations are no longer needed and could be eliminated. And these macros could be eliminated, using the corresponding string directly in each use. " In src/java.base/share/native/libjava/NativeLibraries.c, around line 64: https://github.com/openjdk/jdk/pull/21744#discussion_r1829684759 "I would prefer this be directly inlined at the sole call (in findJniFunction), to make it easier to verify there aren't any buffer overrun problems. (I don't think there are, but looking at this in isolation triggered warnings in my head.) Also, it looks like all callers of findJniFunction ensure the cname argument is non-null. So there should be no need to handle the null case in findJniFunction (other than perhaps an assert or something). That could be addressed in a followup. (I've already implicitly suggested elsewhere in this PR revising this function in a followup because of the JNI_ON[UN]LOAD_SYMBOLS thing.)"
06-11-2024