Rework getJimModules() #2465
Rework getJimModules() #2465
Conversation
runtime/vm/classloadersearch.c
Outdated
| * but it won't keep the class alive. That's OK in this case since the class in | ||
| * question is not a candidate for unloading. | ||
| */ | ||
| vm->jimModules = jimModules = (jclass)&j9JimModules->classObject; |
There was a problem hiding this comment.
This is iffy as it assumes the J9Class will never be freed (today's behaviour). We've been talking about freeing the obsolete classes and this cause a hard to find bug.
There was a problem hiding this comment.
Ah, this is just refactored existing code... something to be cleaned up, not necessarily as part of this PR. If you're not going to fix this now, let me know and I'll open an issue for it.
There was a problem hiding this comment.
It's iffy for a class that we don't know everything about - in this case, it's a boot class (or whatever the JDK9 equivalent is), so we are sure it's not a candidate for unloading.
There was a problem hiding this comment.
Note, in JDK9+ not every java.* class is loaded by the bootloader. Some have been moved to the app loader. (This one is loaded by the bootloader)
My concern is more with future bugs when we allow redefined J9Classes to be GC'd.... though this may not be a concern with FastHCR due to keeping the J9Class stable.
All that to say, we should at minimum open a issue to remember to look at this again,
There was a problem hiding this comment.
So, I don't want an issue for it - this was a change we made to avoid the need to lock and allocate a global ref.
There was a problem hiding this comment.
Why are we trying to avoid creating the global ref? Is there a usecase that demonstrates the current solution is better? I would expect the extra cost of the global ref to be in the noise.
There was a problem hiding this comment.
It was to avoid the need for a single-use monitor. Since you're clearly upset by this, I'll change it back (though I'll reuse an existing monitor).
| (*env)->DeleteLocalRef(env, classLoaderClass); | ||
| (*env)->DeleteLocalRef(env, filenameString); | ||
| (*env)->DeleteLocalRef(env, classLoaderRef); |
There was a problem hiding this comment.
For my own education: why were these calls to DeleteLocalRef reordered?
There was a problem hiding this comment.
I reordered them to be in reverse order of creation, in case that were to ever matter for future optimization of the local ref space (I don't think it does at the moment).
- inline single-use function - handle errors consistently - load instrument module before adding to classpath - remove unnecessary use of VM constant pool Fixes: eclipse-openj9#2437 Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>
|
Jenkins test sanity xlinux jdk10 |
|
Jenkins test sanity xlinux jdk8 |
Fixes: #2437
Signed-off-by: Graham Chapman graham_chapman@ca.ibm.com