Implement ROMMethodInfo caching support and stack walker integration #22918
Conversation
harship04
force-pushed
the
romcache
branch
9 times, most recently
from
19d1ee8 to
5f11894
Compare
|
Waiting for the changes we discussed before review (but at a glance, looks good so far). |
harship04
force-pushed
the
romcache
branch
3 times, most recently
from
24bce23 to
18af487
Compare
|
@gacholio , could you please review the PR |
runtime/vm/swalk.c
Outdated
| MARK_SLOT_AS_OBJECT(walkState, (j9object_t*) &(methodFrame->specialFrameFlags)); | ||
| walkState->method = methodFrame->method; | ||
| if (NULL != walkState->method) { | ||
| initializeBasicROMMethodInfo(walkState, J9_ROM_METHOD_FROM_RAM_METHOD(walkState->method)); |
There was a problem hiding this comment.
I don't think there should be any remaining callers of the basic init code (other than internally in mapcache.cpp) - they should all be replaced with the new ROM method call (other than the one for a bytecoded PC of course).
runtime/stackmap/mapcache.cpp
Outdated
| /* Always compute argument bits */ | ||
| j9localmap_ArgBitsForPC0(romClass, romMethod, newInfo.argbits); | ||
|
|
||
| if (computeStackAndLocals && pc < (UDATA)J9_BYTECODE_SIZE_FROM_ROM_METHOD(romMethod)) { |
There was a problem hiding this comment.
Please bracket the second clause. In fact, why is this necessary? I think we can assume a running PC is withing the bytecode range of its method.
There was a problem hiding this comment.
I removed the pc < (UDATA)J9_BYTECODE_SIZE_FROM_ROM_METHOD(romMethod) check in populateROMMethodInfo, but when I also remove the corresponding condition in getROMMethodInfoForBytecodePC—the one that returns if (pc <= J9SF_MAX_SPECIAL_FRAME_TYPE || pc >= (UDATA)J9_BYTECODE_SIZE_FROM_ROM_METHOD(romMethod)) then the JVM crashes with a segmentation faul
| NULL); | ||
| } | ||
|
|
||
| /* Copy metadata */ |
There was a problem hiding this comment.
Should this not be a call to the basic init code?
|
Looks good in general. When computing the maps, you are not checking the size, checking for failure, or setting the bits indicating that the maps have been cached. |
|
The argbits do not seem to be being computed at all. |
harship04
force-pushed
the
romcache
branch
2 times, most recently
from
495e826 to
b581cfd
Compare
| J9ROMMethod *romMethod = J9_ROM_METHOD_FROM_RAM_METHOD(method); | ||
|
|
||
| /* Use the ROMMethod pointer plus bytecode offset as key */ | ||
| void *key = (void *)((uintptr_t)J9_BYTECODE_START_FROM_ROM_METHOD(romMethod) + (uintptr_t)osrFrame->bytecodePCOffset); |
There was a problem hiding this comment.
This is problematic as it reads the ROM method before checking the cache. Perhaps we need to store the actual PC in the OSR frame. We could then derive the offset in the cache miss case.
There was a problem hiding this comment.
Actually, I'm wrong. The macro does simple addition and does not touch the ROM method memory itself.
Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>
|
The segmentation fault occurred because when pc == 0, the code returned early without initializing walkState->romMethodInfo; this was fixed by calling initializeBasicROMMethodInfo() for special-frame or out-of-bounds PC values. |
|
I added computePendingCountForBytecode() for bytecode-PC frames (argCount + tempCount, plus 1 for synchronized or non-empty methods). J9OSRFrame has pendingStackHeight and numberOfLocals, but J9ROMMethodInfo doesn’t store them. Currently, populateROMMethodInfo() receives these values for OSR frames but doesn’t save them. Should ROMMethodInfo eventually store them for caching, or are they only needed during map computation? |
2 participants
Implements a unified ROM method caching mechanism for consolidating ROM method metadata. Updates stack walking to use the cached information instead of repeated metadata lookups.