Typed access to metadata #647

qinsoon · 2022-08-23T01:26:53Z

This PR refactors metadata, and makes all its access associated with a num type. This PR also greatly reduces code duplication in the metadata module, and adds tests for both header metadata and side metadata access.

Metadata in the following context refers to both side metadata and header metadata.

All metadata access functions are moved to HeaderMetadata/SideMetadata as methods.
Introduce the MetadataValue trait so we can handle u8/16/32/64/size uniformly.
All metadata access methods needs a number type for the get/set value type. It needs to match what is defined in the spec, otherwise it will panic.
Fixe the issue for u64 metadata on 32 bits. It should work properly now.
Add unit tests.
Header metadata methods in ObjectModel now have a default implementation that uses our header metadata module.

Related PRs:

src/util/metadata/header_metadata.rs

src/util/metadata/mod.rs

qinsoon · 2022-08-31T06:50:26Z

I pushed some draft code here. It is not yet ready for review.

qinsoon · 2022-09-08T00:06:59Z

@wks The PR is ready for review again. The focus of the PR changed and there were some major changes since it was reviewed last time.

src/plan/barriers.rs

src/util/metadata/global.rs

wks

The advantage of atomic bit-and and bit-or is that they do not need a cmpxchg loop to implement, so can be more efficient than store or cmpxchg.

src/util/metadata/global.rs

src/util/metadata/side_metadata/global.rs

wks · 2022-09-08T07:28:32Z

src/util/metadata/side_metadata/global.rs

+                    FromPrimitive::from_u8(self.fetch_ops_on_bits(
+                        data_addr,
+                        meta_addr,
+                        order,
+                        order,
+                        |x: u8| x & val.to_u8().unwrap(),
+                    ))


fetch_and doesn't need cmpxchg. Just set irrelevant bits to 1 and they will not be affected.

Suggested change

FromPrimitive::from_u8(self.fetch_ops_on_bits(

data_addr,

meta_addr,

order,

order,

|x: u8| x & val.to_u8().unwrap(),

))

let mask = meta_byte_mask(self) << lshift;

let opnd = val | !mask;

unsafe { T::fetch_and(meta_addr, opnd, order) };

Sounds good. In case you may be interested, the following is the assembly code of fetch_and() for 1 bit side metadata when using fetch_ops_on_bits (internally uses fetch_update), and directly using fetch_and:

with fetch_ops_on_bits

mmtk::util::alloc_bit::unset_alloc_bit_unsafe: mov rcx, rdi shr rdi, 6 shr cl, 3 movabs rsi, 13194139533312 mov al, byte, ptr, [rdi, +, rsi] mov dl, -2 rol dl, cl .LBB225_1: mov ecx, eax and cl, dl lock cmpxchg, byte, ptr, [rdi, +, rsi], cl jne .LBB225_1 ret

with fetch_and

mmtk::util::alloc_bit::unset_alloc_bit_unsafe: mov rcx, rdi mov rax, rdi shr rax, 6 shr cl, 3 mov dl, -2 rol dl, cl movabs rcx, 13194139533312 lock and, byte, ptr, [rax, +, rcx], dl

src/util/metadata/side_metadata/global.rs

wks

With the introduction of fetch_and and fetch_or, some bit operations can be done more efficiently using them, especially set bits and clear bits.

We may change all call sites of store_atomic into fetch_and_atomic or fetch_or_atomic.

Alternatively, we can add a special case for store_atomic if bits_num_log == 1. In this case, we can turn store_atomic internally into fetch_and or fetch_or, depending on whether it is storing 1 or 0.

wks · 2022-09-08T07:38:43Z

src/util/metadata/log_bit.rs

@@ -7,6 +6,6 @@ use std::sync::atomic::Ordering;
 impl VMGlobalLogBitSpec {
    /// Mark the log bit as unlogged (1 means unlogged)
    pub fn mark_as_unlogged<VM: VMBinding>(&self, object: ObjectReference, order: Ordering) {
-        store_metadata::<VM>(self, object, 1, None, Some(order))
+        self.store_atomic::<VM, u8>(object, 1, None, order)


One-bit store_atomic has to use fetch_update or cmpxchg. fetch_or_atomic is more efficient for this.

src/util/metadata/header_metadata.rs

wks · 2022-09-08T08:31:16Z

src/policy/immortalspace.rs

            );
            if old_value == value {
                return false;
            }

-            if compare_exchange_metadata::<VM>(
-                &VM::VMObjectModel::LOCAL_MARK_BIT_SPEC,
+            if VM::VMObjectModel::LOCAL_MARK_BIT_SPEC.compare_exchange_metadata::<VM, u8>(


This may be refactored to use fetch_update.

wks · 2022-09-08T08:33:22Z

src/policy/largeobjectspace.rs

            );
            let mark_bit = old_value & mask;
            if mark_bit == value {
                return false;
            }
-            if compare_exchange_metadata::<VM>(
-                &VM::VMObjectModel::LOCAL_LOS_MARK_NURSERY_SPEC,
+            if VM::VMObjectModel::LOCAL_LOS_MARK_NURSERY_SPEC.compare_exchange_metadata::<VM, u8>(


Can be refactored to fetch_update

wks · 2022-09-08T08:34:18Z

src/policy/largeobjectspace.rs

            );
            let new_val = old_val & !NURSERY_BIT;
-            if compare_exchange_metadata::<VM>(
-                &VM::VMObjectModel::LOCAL_LOS_MARK_NURSERY_SPEC,
+            if VM::VMObjectModel::LOCAL_LOS_MARK_NURSERY_SPEC.compare_exchange_metadata::<VM, u8>(


Consider refactoring to fetch_update.

wks · 2022-09-08T08:36:47Z

src/policy/mallocspace/global.rs

@@ -316,9 +316,10 @@ impl<VM: VMBinding> MallocSpace<VM> {
            address,
        );

-        if !is_marked::<VM>(object, None) {
+        // TODO: Why do we use non-atomic load here?
+        if !unsafe { is_marked_unsafe::<VM>(object) } {


I think it is a bug. If it is not atomic, it is guaranteed to cause data race because tracing is parallel.

I assume it should be a benign race, and it should not cause any actual bug. There are some discussions here: #313

When @steveblackburn said "atomic operation" in #313 (comment), I think he meant atomic read-modify-write (RMW) operation (such as cmpxchg, swap, fetch_xxx, etc.), and atomic RMW operations do prevent duplicate edges. In Java, all write operations to 32-bit variables and reference variables (even not volatile) are atomic, in the sense that it disallows word tearing, and if a read does not have happens-before relationship with a write, the read always sees some benign values (written by an actual store). So we seldom use the phrase "atomic read" or "atomic write" in Java.

It is not true for C++ and Rust. In C++ and Rust, if a non-atomic load has no happens-before relationship with a store, it has undefined behaviour. The Rust counterpart of "ordinary" load/store in Java are load_atomic and store_atomic with the Relaxed order (actually the Java memory order is even weaker than Relaxed, but must prevent word tearing).

We can change this to a relaxed atomic load, which is more reasonable.

But just for the sake of argument, I don't think this causes any actual bug. The issue #313 is clear that the if block may be executed multiple times for the same object. So:

if is_marked() returns false, but the object is actually marked: we will just execute the block again, and it is fine.

if is_marked() returns true, but the object is not actually marked: I don't think this will happen, as we are monotonically set the bit from 0 to 1 at this stage.

I have changed this to a relaxed atomic load.

wks · 2022-09-08T08:38:49Z

src/policy/mallocspace/metadata.rs

-        ordering,
-    );
+pub fn set_mark_bit<VM: VMBinding>(object: ObjectReference, ordering: Ordering) {
+    VM::VMObjectModel::LOCAL_MARK_BIT_SPEC.store_atomic::<VM, u8>(object, 1, None, ordering);


This is a one-bit metadata. May use fetch_or_atomic.

wks

About the return value of cmpxchg...

wks · 2022-09-08T08:53:56Z

src/util/metadata/global.rs

+    /// * `success_order`: is the atomic ordering used if the operation is successful.
+    /// * `failure_order`: is the atomic ordering used if the operation fails.
+    ///
+    /// # Returns `true` if the operation is successful, and `false` otherwise.


Cmpxchg should return the old value and whether it is successful or not. The AtomicU8::compare_exchange method returns Result<u8, u8>. We should do it similarly here. Some algorithm can benefit from it and eliminate a subsequent load.

I will change this to Result<T, T>. For its use cases, I will leave it as compare_exchange().is_ok().

qinsoon · 2022-09-09T00:26:56Z

@wks My plan is to fix whatever is about the implementation the metadata. For the usage of the metadata and any possible optimization on that, I will leave it to another PR.

being poisoned

fetch_and/or for bits.

Use fetch_and/or to implement fetch_and/or on bits.

qinsoon · 2022-09-12T05:05:04Z

This pull request is ready for another review. @wks

src/util/metadata/header_metadata.rs

src/util/metadata/side_metadata/global.rs

fetch_update.

wks

LGTM

qinsoon added 2 commits August 23, 2022 11:26

Use u64 instead usize for 8bytes side metadata

729d990

Merge branch 'master' into fix-side-metadata-usize

1d1a9bc

qinsoon force-pushed the fix-side-metadata-usize branch from 5763187 to 6b8d05f Compare August 23, 2022 02:05

Use u64 instead usize for 8bytes side metadata

4a0d8ff

qinsoon force-pushed the fix-side-metadata-usize branch from 6b8d05f to 4a0d8ff Compare August 23, 2022 02:09

qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Aug 23, 2022

Apply same change to header metadata

76debb8

qinsoon marked this pull request as ready for review August 23, 2022 23:20

qinsoon requested a review from wks August 23, 2022 23:20

wks requested changes Aug 24, 2022

View reviewed changes

src/util/metadata/header_metadata.rs Outdated Show resolved Hide resolved

src/util/metadata/header_metadata.rs Outdated Show resolved Hide resolved

src/util/metadata/mod.rs Outdated Show resolved Hide resolved

qinsoon added 11 commits August 25, 2022 16:34

MetadataValue Prototype

39fb2c1

Refactor side metadata methods with MetadataValue

241c6df

Refactor Metadata and HeaderMetadata with MetadataValue

5079d01

Provide a default impl for header metadata access in ObjectModel

4296583

Tidy up side metadata sanity: use verify_update instead verify_<op>

5c28dc2

Add fetch_and/or/update

c432b50

Fix current tests

ec838ab

Move a few methods to SideMetadata

4b63bf2

WIP: Add some tests

4078e90

WIP: more tests

b19e242

More tests on SideMetadata

44a21c0

qinsoon added 3 commits September 1, 2022 14:14

Metadata ops should call ObjectModel methods

cf3d204

Add test for header metadata

967fe5f

Separate atomic/non-atomic load/store for metadata and header metadata

fe11527

qinsoon removed the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Sep 6, 2022

qinsoon added 4 commits September 6, 2022 13:05

Fix test/style

21ba305

Use u64 for side metadata sanity map

9ecb707

Remove code that was commented out

01054e7

Merge branch 'master' into fix-side-metadata-usize

43188f4

qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Sep 8, 2022

This was referenced Sep 8, 2022

Update to mmtk-core PR#647 mmtk/mmtk-openjdk#178

Merged

Update to mmtk-core PR #647 mmtk/mmtk-jikesrvm#122

Merged

Update mmtk-core PR #647 mmtk/mmtk-v8#67

Merged

wks requested changes Sep 8, 2022

View reviewed changes

src/plan/barriers.rs Outdated Show resolved Hide resolved

Use atomic load for object barrier

8526afc

wks reviewed Sep 8, 2022

View reviewed changes

src/util/metadata/global.rs Outdated Show resolved Hide resolved

wks requested changes Sep 8, 2022

View reviewed changes

wks reviewed Sep 8, 2022

View reviewed changes

qinsoon added 2 commits September 9, 2022 09:53

Move return description to the end of the description.

0c6ff4a

Outdated comments

ed6e905

qinsoon added 7 commits September 9, 2022 10:58

Assert input value for side metadata access. Allow serial_test lock

7c51e72

being poisoned

Use fetch_update for atomic_store and fetch_ops for bits. Use

9e58b7b

fetch_and/or for bits.

Use fetch_update for fetch_update_atomic of bits. cargo fmt

32355d2

Use fetch_update over compare_and_exchange on header metadata as well.

ff95893

Use fetch_and/or to implement fetch_and/or on bits.

Use fetch_update for HeaderMetadataSpec.fetch_ops_on_bits.

02d7b99

compare_exchange returns Result

06d6395

Merge branch 'master' into fix-side-metadata-usize

1c4cc86

wks reviewed Sep 12, 2022

View reviewed changes

src/util/metadata/header_metadata.rs Outdated Show resolved Hide resolved

wks reviewed Sep 12, 2022

View reviewed changes

src/util/metadata/side_metadata/global.rs Outdated Show resolved Hide resolved

qinsoon added 2 commits September 13, 2022 09:58

Use relaxed atomic load for mark sweep trace_object. Use Option.map for

99a9840

fetch_update.

Use explicit type parameter for fetch_update

24bb41b

wks approved these changes Sep 13, 2022

View reviewed changes

qinsoon merged commit 4ae23b1 into mmtk:master Sep 13, 2022

qinsoon mentioned this pull request Oct 12, 2022

Refactor typed access to SideMetadataSpec<MetadataValue> #679

Open

wks mentioned this pull request Mar 8, 2023

Add fetch_xxx and fetch_update API for metadata. #651

Closed

Typed access to metadata #647

Typed access to metadata #647

Uh oh!

Conversation

qinsoon commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qinsoon commented Aug 31, 2022

Uh oh!

qinsoon commented Sep 8, 2022

Uh oh!

Uh oh!

Uh oh!

wks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wks Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

with fetch_ops_on_bits

with fetch_and

Uh oh!

Uh oh!

Uh oh!

wks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wks Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinsoon commented Sep 9, 2022

Uh oh!

qinsoon commented Sep 12, 2022

Uh oh!

Uh oh!

Uh oh!

wks left a comment

Choose a reason for hiding this comment

qinsoon commented Aug 23, 2022 •

edited

Loading

wks Sep 8, 2022 •

edited

Loading

wks Sep 12, 2022 •

edited

Loading