Resolve static field ref in CP if possible #20770
Conversation
|
Attn @mpirvu. This seems to eliminate the |
| // This CP entry could still be unresolved, which would cause | ||
| // getClassOfStaticFromCP() to fail. Attempt to resolve it manually to reduce | ||
| // the chances of failure. | ||
| if (cpIndex != -1) |
There was a problem hiding this comment.
Should we call TR_ResolvedJ9Method::getClassOfStaticFromCP() first and only if that fails try to resolve at compile time?
I am assuming that most of the time getClassOfStaticFromCP() will succeed so we don't pay the price of getting VM access.
There was a problem hiding this comment.
TR_ResolvedJ9Method::getClassOfStaticFromCP acquires vmaccess, so as the code stands, we'd be doing it twice. Three times if the approach is going to be:
if (!TR_ResolvedJ9Method::getClassOfStaticFromCP) {
resolveStaticFieldRef;
getClassOfStaticFromCP;
}
it would be nice if J9::VMAccessCriticalSection checked if we already have vmaccess and do an early return, though that would have the side effect of not checking if the compilation should be interrupted as often.
There was a problem hiding this comment.
Would it make sense to put this in getClassOfStaticFromCP instead?
There was a problem hiding this comment.
Hm that is a question for @jdmpapin :)
There was a problem hiding this comment.
If the code already has VM access it won't acquire it again, so we can do
if (cp != -1)
{
TR::VMAccessCriticalSection getClassFromConstantPool(_fej9);
TR_OpaqueClassBlock * clazz = TR_ResolvedJ9Method::getClassOfStaticFromCP(_fej9, beholderCP, cpIndex);
if (!clazz)
{
_fej9->_vmFunctionTable->resolveStaticFieldRef(_fej9->vmThread(), NULL, beholderCP, cpIndex, J9_RESOLVE_FLAG_JIT_COMPILE_TIME, NULL);
clazz = TR_ResolvedJ9Method::getClassOfStaticFromCP(_fej9, beholderCP, cpIndex);
}
}
return validateSymbol(classID, clazz);
There was a problem hiding this comment.
Would it make sense to put this in
getClassOfStaticFromCPinstead?
I think we might as well. I don't know of any reason why we shouldn't try to compile-time resolve if we call getClassOfStaticFromCP() from somewhere else
|
@dsouzai I would appreciate your review on this on this PR as well. |
|
There is also a chance these failures are really due to #20730 (comment), which I will double-check. |
| if (cpIndex != -1) | ||
| { | ||
| TR::VMAccessCriticalSection getClassFromConstantPool(_fej9); | ||
| _fej9->_vmFunctionTable->resolveStaticFieldRef(_fej9->vmThread(), NULL, beholderCP, cpIndex, J9_RESOLVE_FLAG_JIT_COMPILE_TIME, NULL); |
There was a problem hiding this comment.
From the code, it looks like we call jitCTResolveStaticFieldRefWithMethod to do compile time resolution of static fields. Ideally we'd call the same thing, but we do have the problem of needing to poof up a J9Method, so resolveStaticFieldRef seems ok.
There was a problem hiding this comment.
This made me worry about whether the J9Method could affect the result, but I think it's fine.
The method is only used to determine whether it's OK to store to a static final field. If at compile-time we generated a resolved static field store, then the store was allowed at the time, so one of the following will hold:
- It was non-final. On (successful) load, it will still be non-final because we validate the class chain of the defining class.
- It was final and defined by the same class as the method containing the
putstatic. On (successful) load, this will still be the case because the class ref in the static field ref CP entry names the containing class (i.e. the class that owns the CP entry), and we validate the class chain of the defining class of each inlined method. In this case the class file version must be <53 or theputstaticmust occur within<clinit>, both of which are also captured in the class chain.
Also, when generating IL for putstatic, all of the above is independent of how we validate classOfStatic(), which is only used for generating a write barrier
The failures are still present with the corrected version of that PR. |
cjjdespres
force-pushed
the
relo-static-ref
branch
from
2e24894 to
87fc68a
Compare
There was a problem hiding this comment.
LGTM. Comment for this one and for TR_ResolvedJ9Method::getClassOfStaticFromCP needs to be changed/added saying that if field is not resolved, the code will attempt to resolve at compile time.
I was worried about using this method to determine which blocks are cold based on seeing unresolved references, but I don't see such usage.
|
@gacholio Could you please review this PR as well since it touches VM code? Thanks |
cjjdespres
force-pushed
the
relo-static-ref
branch
from
87fc68a to
9b19b5a
Compare
|
I will start testing proactively while while waiting for other potential reviewer. |
|
jenkins test sanity all jdk21 |
|
Compilations errors: |
Signed-off-by: Christian Despres <[email protected]>
cjjdespres
force-pushed
the
relo-static-ref
branch
from
9b19b5a to
9f4ca45
Compare
|
jenkins test sanity all jdk21 |
|
jenkins test extended,sanity.system,extended.system xlinux jdk21 |
|
aarch64 failed jdk_vector_double128_j9_0 On AIX we have an infra problem: |
|
That aarch64 error in looks like #20389. |
|
FYI: I compared nightly builds OpenJ9-JDK17-x86-64_linux-20241211-000850 and OpenJ9-JDK17-x86-64_linux-20241212-001148 from AcmeAirEE8 rampup point of view. The latter (which includes this change) had a significantly worse rampup for the first warm run. The subsequent warm runs were good and even slightly better than the baseline from Dec11. Looking at compilation stats I observed that the warm run that had bad rampup also had many AOT compilations. This happens because my runs are short (2 minutes) and with just 1P there isn't enough time to perform all the needed AOT compilations in the cold run. In another experiment I ran with no AOT and just warm compilations, but I didn't see an increase in CompCPU, so the effecr is limited to AOT compilations. |
|
I can move it back to the SVM, changed to use a single VM access. I don't think it's critical to have it be active during code generation. |
Some constant pool entries may be unresolved when attempting to relocate a method. If the classes involved are all loaded or initialized properly and the entry is a static field ref, then resolveStaticFieldRef() will be able to resolve the entry, allowing relocation to proceed.