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

Make j9localmap_DebugLocalBitsForPC the default mapper #20569

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

babsingh
Copy link
Contributor

j9localmap_LocalBitsForPC is more aggressive than
j9localmap_DebugLocalBitsForPC in the way it analyzes and marks local
variables as object references. Aggressiveness relates to determining
whether a local object is being used at a specific point in the
bytecode execution, and this helps optimize garbage collection and
runtime performance. In contrast, j9localmap_DebugLocalBitsForPC
adopts a conservative approach; it overestimates and plays safe by
marking objects alive even if they are not being used.

j9localmap_DebugLocalBitsForPC resolves the issue seen in
ibmruntimes/Semeru-Runtimes#93.

We have run the following benchmarks: Liberty DT8/DT10 Startup &
Footprint, JRuby and Nashorn. No performance difference is seen between
j9localmap_DebugLocalBitsForPC and j9localmap_LocalBitsForPC.

Since there are no side-effects of j9localmap_DebugLocalBitsForPC,
this PR makes it OpenJ9's default mapper.

j9localmap_LocalBitsForPC is more aggressive than
j9localmap_DebugLocalBitsForPC in the way it analyzes and marks local
variables as object references. Aggressiveness relates to determining
whether a local object is being used at a specific point in the
bytecode execution, and this helps optimize garbage collection and
runtime performance. In contrast, j9localmap_DebugLocalBitsForPC
adopts a conservative approach; it overestimates and plays safe by
marking objects alive even if they are not being used.

j9localmap_DebugLocalBitsForPC resolves the issue seen in
ibmruntimes/Semeru-Runtimes#93.

We have run the following benchmarks: Liberty DT8/DT10 Startup &
Footprint, JRuby and Nashorn. No performance difference is seen between
j9localmap_DebugLocalBitsForPC and j9localmap_LocalBitsForPC.

Since there are no side-effects of j9localmap_DebugLocalBitsForPC,
this PR makes it OpenJ9's default mapper.

Signed-off-by: Babneet Singh <[email protected]>
@babsingh babsingh marked this pull request as ready for review November 12, 2024 01:11
@babsingh babsingh requested a review from gacholio November 12, 2024 01:12
@babsingh
Copy link
Contributor Author

This PR includes a potentially high-risk change that may affect performance. I've run some benchmarks and haven't observed any performance regressions. To ensure ongoing monitoring, we’d like to alert the perf team to track the impact of these changes in the weekly performance builds after merging. @jdekonin, could you assist with this?

fyi @tajila

@gacholio
Copy link
Contributor

jenkins test sanity zlinux jdk21

@pshipton
Copy link
Member

Not sure they need alerting. I've seen it, and if a regression is reported that includes this change, I'll call it out.

@gacholio
Copy link
Contributor

Test failure is CRIU which would not be affected by this change.

@gacholio gacholio merged commit a88773c into eclipse-openj9:master Nov 12, 2024
4 of 6 checks passed
@pshipton
Copy link
Member

This is causing asserts, I'm reverting it.
Failures are http://vmfarm.rtp.raleigh.ibm.com/jobs_by_status.php?build_id=81473&status=FAILED&categories=17

j> 10:41:51 000001002A258100: Object neither in heap nor stack-allocated in thread Pooled Thread #141 running com.ibm.jtc.svt.tests.invoke.BasicTest2
j> 10:35:49 15:35:49.674 0x30125a00 j9mm.479 * ** ASSERTION FAILED ** at ../../gc_glue_java/MarkingSchemeRootMarker.cpp:53: ((MM_StackSlotValidator(MM_StackSlotValidator::NOT_ON_HEAP, object, stackLocation, walkState).validate(_env)))

j> 10:49:53 3D053000: Object neither in heap nor stack-allocated in thread Primary|SimpleDriver|com.ibm.jtc.svt.tests.invoke.AsTypeTest.testFloat|3344|Default Invocant
15:41:36.409 0x3001b100 j9mm.479 * ** ASSERTION FAILED ** at ../../../../gc_glue_java/ScavengerRootScanner.hpp:109: ((MM_StackSlotValidator(MM_StackSlotValidator::NOT_ON_HEAP, *slotPtr, stackLocation, walkState).validate(_env)))

000000000001E700: Object neither in heap nor stack-allocated in thread main
15:07:05.299 0x121100 j9mm.479 * ** ASSERTION FAILED ** at ../../../../gc_glue_java/ScavengerRootScanner.hpp:109: ((MM_StackSlotValidator(MM_StackSlotValidator::NOT_ON_HEAP, *slotPtr, stackLocation, walkState).validate(_env)))

000000003001B100: Invalid class pointer in stack allocated object in thread main
15:41:36.409 0x3001b100 j9mm.479 * ** ASSERTION FAILED ** at ../../../../gc_glue_java/ScavengerRootScanner.hpp:109: ((MM_StackSlotValidator(MM_StackSlotValidator::NOT_ON_HEAP, *slotPtr, stackLocation, walkState).validate(_env)))

@gacholio
Copy link
Contributor

This doesn't make much sense - if the debug mapper is so flawed, we would certainly have aeen problems in the field.

@pshipton
Copy link
Member

pshipton commented Nov 12, 2024

This was the only change in the build, so it seemed the cause. It failed enough that we should have seen it in earlier builds even if it's intermittent and caused by something else.

@pshipton
Copy link
Member

Is the JIT usually in a different state when the debug mapper is used?

@gacholio
Copy link
Contributor

gacholio commented Nov 12, 2024

The mapper is only used for interpreted frames. The JIT maintains it's own metadata for compiled methods.

@gacholio
Copy link
Contributor

gacholio commented Nov 12, 2024

Looking through the code, there is an interaction with DLT. If the problem can be reproduced, it may be worth disabling that in some runs to see if that's the issue.

@gacholio
Copy link
Contributor

I don't know if DLT is disabled for debug mode runs.

@pshipton
Copy link
Member

I assumed the JIT was involved since stack allocated objects are mentioned.

@gacholio
Copy link
Contributor

There won't be any stack-allocated objects in the DLT case.

@babsingh
Copy link
Contributor Author

@gacholio Do the below flags need to be set before using j9localmap_DebugLocalBitsForPC?

vm->requiredDebugAttributes |= J9VM_DEBUG_ATTRIBUTE_LOCAL_VARIABLE_TABLE;
vm->requiredDebugAttributes |= J9VM_DEBUG_ATTRIBUTE_CAN_ACCESS_LOCALS;
vm->localMapFunction = j9localmap_DebugLocalBitsForPC;

@gacholio
Copy link
Contributor

From the interpreter's POV. no. The CAN_ACCESS_LOCALS almost certainly impacts JIT code generation (I think it may actually force FSD). I still don't understand why conservative object lifetimes would cause problems, but I'm pretty sure that FSD disables stack allocation, so that would explain the difference in behaviours.

@gacholio
Copy link
Contributor

For the record, the difference in the local mappers is read vs write. The normal mapper looks forward from the target PC along all paths to see if the a slot is ever read again as an object. The debug mapper is the opposite - it looks from the entry points to the method forward to the PC to see what was stored in the slot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants