Skip to content

Clean up Line/ObjectReference/Address conversions in RefCountHelper - #1558

Merged
qinsoon merged 2 commits into
mmtk:masterfrom
qinsoon:remove-address-to-object-ref
Aug 20, 2026
Merged

Clean up Line/ObjectReference/Address conversions in RefCountHelper#1558
qinsoon merged 2 commits into
mmtk:masterfrom
qinsoon:remove-address-to-object-ref

Conversation

@qinsoon

@qinsoon qinsoon commented Aug 20, 2026

Copy link
Copy Markdown
Member

This PR cleans up the conversions between Line/ObjectReference/Address in RefCountHelper. It does not address the confusion in LXR between object start and object ref.

Line/ObjectReference/Address conversions in RefCountHelper
Comment thread src/policy/immix/line.rs

/// Return the line that contains the raw address of the object reference.
pub fn containing_obj_ref(object: ObjectReference) -> Self {
Self(object.to_raw_address())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates an invalid Line (unaligned address). All the call sites of this method are now switched to object_is_in_straddle_line_no_rc_check

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the unused containing_obj_start method above this one, too.

@qinsoon

qinsoon commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

This tracks the correctness of this PR for LXR: mmtk/mmtk-openjdk#378

@qinsoon
qinsoon requested a review from wks August 20, 2026 05:01

@wks wks left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is good enough for cleaning up and removing Address::to_object_reference. But the predicate object_is_in_straddle_line may not make sense if the object_start != raw address because the start may be in one line and the raw address may be in another line. But that will require algorithm change, and we can do it later.

One change should be made in this PR. Because we removed Line::containing_obj_ref, we should remove Line::containing_obj_start, too for consistency. containing_obj_start is unused anyway.

if c != 0 {
// Safety: cur_cursor is either a valid object reference, or a straddle line
let o = unsafe { ObjectReference::from_raw_address_unchecked(cur_cursor) };
if !immix_space.is_marked(o) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this line, we have not checked whether o is a valid ObjectReference, yet. It actually checks the mark bit at the the raw address of o. This is only possible if the mark bit is on the side, which the ImmixSpace seems to require for now, but not formally specified.

We can eliminate this by introducing ImmixSpace::is_address_marked(addr) which only works if the mark bit is on the side. But I think it is OK for now. We can fix it in the future when we reach a consensus about whether mark bits must be on the side for Immix, whether LXR requires side mark bits, and whether we can implement this method better without this check.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It is unsound to check immix_space.is_marked(o) here -- we don't even know if o is an object. But this is the same code as the original LXR PR. I don't intend to address that issue in this PR.

@wks wks left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qinsoon

qinsoon commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

I saw this error with this PR in the binding CI: https://github.com/mmtk/mmtk-openjdk/actions/runs/32333739772/job/96323577549?pr=378

MMTK_NO_REFERENCE_TYPES=true MMTK_PLAN=LXR /home/runner/work/mmtk-openjdk/mmtk-openjdk/bundles/jdk/bin/java --add-exports java.base/jdk.internal.ref=ALL-UNNAMED -XX:MetaspaceSize=500M -XX:+DisableExplicitGC -server -XX:+CrashOnOutOfMemoryError -XX:+UseThirdPartyHeap -Xms120M -Xmx120M -jar /home/runner/work/mmtk-openjdk/mmtk-openjdk/dacapo/dacapo-23.11-MR2-chopin-minimal.jar -n 1 jython -preserve
...
===== DaCapo 23.11-MR2-chopin-minimal jython starting =====
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007f3993630183, pid=21276, tid=21289
#
# JRE version: OpenJDK Runtime Environment (21.0.9) (build 21.0.9-internal-adhoc.runner.openjdk)
# Java VM: OpenJDK 64-Bit Server VM (21.0.9-internal-adhoc.runner.openjdk, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, third-party gc, linux-amd64)
# Problematic frame:
# C  [libmmtk_openjdk.so+0x230183]  mmtk::plan::lxr::gc_work::rc::ProcessIncs$LT$VM$C$_$GT$::promote::h92b9737ec6a9a813+0x33
#
# Core dump will be written. Default location: Core dumps may be processed with "/lib/systemd/systemd-coredump %P %u %g %s %t 9223372036854775808 %h %d" (or dumping to /tmp/runbms-9945ylxj/core.21276)
#
# An error report file with more information is saved as:
# /tmp/runbms-9945ylxj/hs_err_pid21276.log
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
#

I haven't seen this error before. So there is a chance that this PR introduced the issue. But I checked the PR a few times, I don't see how this PR changed any LXR's behavior. And I couldn't reproduce the bug locally.

@wks Can you double check this PR and see you find any issue?

@wks

wks commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

@qinsoon I ran jython locally repeatedly for 20-30 minutes without reproducing this error. I didn't find any changes in this PR that can introduce new sigsegv, either.

@qinsoon

qinsoon commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

@qinsoon I ran jython locally repeatedly for 20-30 minutes without reproducing this error. I didn't find any changes in this PR that can introduce new sigsegv, either.

Thank you. I will open a bug report for the issue then.

@qinsoon
qinsoon added this pull request to the merge queue Aug 20, 2026
Merged via the queue into mmtk:master with commit fb806c0 Aug 20, 2026
32 of 34 checks passed
@qinsoon
qinsoon deleted the remove-address-to-object-ref branch August 20, 2026 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants