Fix object ref SFT #555

qinsoon · 2022-03-06T23:30:31Z

If a binding uses ObjectReference to point to an address that is not within our allocated memory, the SFT and the side metadata for the ObjectReference may not be initialized properly.

This PR introduces a solution by introducing a constant ObjectModel::OBJECT_REF_OFFSET_BEYOND_CELL. If a binding may use object reference to point to an address beyond the cell we allocate, they need to provide a Some value for the constant. MMTk allocators need to guarantee that the object reference for an allocation won't point to a new chunk (so its metadata is properly initialized and is the same as the allocation result).

This PR fixes mmtk/mmtk-jikesrvm#102.

This PR:

introduces a constant ObjectModel::OBJECT_REF_OFFSET_BEYOND_CELL. By default, it is None. If a binding may use ObjectReference to point to an address outside a cell, they need to set the constant to a proper Some value.
Introduces a module util::alloc::object_ref_guard, which includes a few methods that help allocators to check and avoid breaking the invariant.
adds a few assertions to check if our allocation addresses and object references have valid SFT entries.
introduces a space acquire lock so we can guarantee when pages are acquired from a page resource, the SFT for the pages are always initialized.
removes unnecessary methods in Map32.

ObjectModel::OBJECT_REF_OFFSET_BEYOND_CELL

qinsoon · 2022-03-09T00:12:27Z

binding-refs
JIKESRVM_BINDING_REF=fix-object-ref-sft

src/policy/space.rs

src/util/alloc/object_ref_guard.rs

src/util/heap/layout/vm_layout_constants.rs

src/util/alloc/object_ref_guard.rs

wks · 2022-03-09T12:45:42Z

src/util/alloc/malloc_allocator.rs

+        loop {
+            let ret = self.space.alloc(self.tls, size, align, offset);
+            if object_ref_may_cross_chunk::<VM>(ret) {
+                self.space.free(ret);


If the malloc implementation is clever enough to reuse the memory you just free-ed in the next malloc, this loop may go on forever. We need to think about a way to "disrupt" malloc a bit, for example, allocating a slightly bigger object, requiring a stricter alignment, do the second malloc before free-ing, etc.

I made changes to the code. If the first allocation does not pass the object ref check, a vec is created to store the 'bad' results, and the results are free'd when we have a 'good' result.

Kunshan’s concern is very valid. For example, the malloc in glibc has tcache, which can be problematic.

Yi’s new approach sounds better. But it still has one potential problem. For example, if you allocate A and it’s bad, so you allocate again, you get B, which is good, and then you free A. A goes into the tcache, and for next allocation of the same size class, you will get A again, which you need to withhold, and try again. That is, once you get a bad cell and free it, it stays forever in the tcache (and the list can grow the next time you hit a chunk boundary). I’m not sure whether this will manifest in real world scenarios, or whether this will introduce a performance hit (we assume that if people call into libraries for malloc, they probably don’t care too much about performance anyway)

qinsoon · 2022-03-10T03:51:53Z

@wks I have addressed the reviews. Can you take a look again?

wks

LGTM

This PR updates MMTk core to mmtk/mmtk-core#555, and fixes #102

If a binding uses `ObjectReference` to point to an address that is not within our allocated memory, the SFT and the side metadata for the `ObjectReference` may not be initialized properly. This PR introduces a solution by introducing a constant `ObjectModel::OBJECT_REF_OFFSET_BEYOND_CELL`. If a binding may use object reference to point to an address beyond the cell we allocate, they need to provide a `Some` value for the constant. MMTk allocators need to guarantee that the object reference for an allocation won't point to a new chunk (so its metadata is properly initialized and is the same as the allocation result). This PR fixes mmtk/mmtk-jikesrvm#102. This PR: * introduces a constant `ObjectModel::OBJECT_REF_OFFSET_BEYOND_CELL`. By default, it is `None`. If a binding may use `ObjectReference` to point to an address outside a cell, they need to set the constant to a proper `Some` value. * Introduces a module `util::alloc::object_ref_guard`, which includes a few methods that help allocators to check and avoid breaking the invariant. * adds a few assertions to check if our allocation addresses and object references have valid SFT entries. * introduces a space acquire lock so we can guarantee when pages are acquired from a page resource, the SFT for the pages are always initialized. * removes unnecessary methods in `Map32`.

This PR changes the scope of `Space.acquire_lock`. The lock was introduced in #555, and it caused bad allocation performance when we have many threads (e.g. more than 24 threads, mutator threads or GC threads with copying allocators). This pull request reduces the scope of this `acquire_lock`, and the overhead from it is mostly neglectable. This closes #610.

This PR removes most of our uses of `ObjectReference::to_address()`, and replace that with `ObjectModel::ref_to_address()`. With this change, the address we get from an object reference is always guaranteed to be within the allocation. Thus changes in #555 are reverted in this PR. This PR also addresses the comments raised in #643 (comment). Changes: * Replace our use of `ObjectReference::to_address()` to `ObjectModel::ref_to_address()` * Changes to `ObjectReference`: * Rename `ObjectReference::to_address()` to `ObjectReference::to_raw_address()`, and make it clear that we should avoid using it. * Remove `Address::to_object_reference()`, and add `ObjectReference::from_raw_address()`. Make it clear that we should avoid using it. * Changes to `ObjectModel`: * add `address_to_ref` which does the opposite of `ref_to_address` * add `ref_to_header` * rename `object_start_ref` to `ref_to_object_start`, to make it consistent with other methods. * Change `Treadmill` to store `ObjectReference` instead of `Address`. We previously stored object start address in `Treadmill` and we assumed alloc bit is set on the object start address. With changes in this PR, alloc bit is no longer set on object start address. I made changes accordingly. * Remove code about 'object ref guard' which was used to deal with the case that an object reference (address) may not be in the same chunk as the allocation. That should no longer happen. * `alloc_bit::is_alloced_object(address)` now returns an `Option<ObjectReference>`. We may consider return `Option<ObjectReference>` with our API `is_mmtk_object()` as well, but i did not include this change in the PR.

This PR removes most of our uses of `ObjectReference::to_address()`, and replace that with `ObjectModel::ref_to_address()`. With this change, the address we get from an object reference is always guaranteed to be within the allocation. Thus changes in mmtk#555 are reverted in this PR. This PR also addresses the comments raised in mmtk#643 (comment). Changes: * Replace our use of `ObjectReference::to_address()` to `ObjectModel::ref_to_address()` * Changes to `ObjectReference`: * Rename `ObjectReference::to_address()` to `ObjectReference::to_raw_address()`, and make it clear that we should avoid using it. * Remove `Address::to_object_reference()`, and add `ObjectReference::from_raw_address()`. Make it clear that we should avoid using it. * Changes to `ObjectModel`: * add `address_to_ref` which does the opposite of `ref_to_address` * add `ref_to_header` * rename `object_start_ref` to `ref_to_object_start`, to make it consistent with other methods. * Change `Treadmill` to store `ObjectReference` instead of `Address`. We previously stored object start address in `Treadmill` and we assumed alloc bit is set on the object start address. With changes in this PR, alloc bit is no longer set on object start address. I made changes accordingly. * Remove code about 'object ref guard' which was used to deal with the case that an object reference (address) may not be in the same chunk as the allocation. That should no longer happen. * `alloc_bit::is_alloced_object(address)` now returns an `Option<ObjectReference>`. We may consider return `Option<ObjectReference>` with our API `is_mmtk_object()` as well, but i did not include this change in the PR.

qinsoon added 10 commits March 2, 2022 09:07

wip

4a582b1

bug solved

844a067

debug

0f2b7ec

Add MAXIMUM_OBJ_REF_OFFSET

1067d09

Clean up

7af194b

cargo fmt

6b30c0d

Drop the acquire lock before calling to the VM

577ff1e

Set limit to avoid object ref cross chunks

691166d

Move some functions to util::alloc::object_ref_guard. Rename

b5fa6b6

ObjectModel::OBJECT_REF_OFFSET_BEYOND_CELL

Fix assert_object_ref_in_cell

f5b477f

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

Add some comments about adjust_thread_local_buffer_limit

cf27c21

qinsoon mentioned this pull request Mar 9, 2022

Set OBJECT_REF_OFFSET_BEYOND_CELL mmtk/mmtk-jikesrvm#103

Merged

Minor cleanup

6c4c29d

qinsoon marked this pull request as ready for review March 9, 2022 06:23

qinsoon requested a review from wks March 9, 2022 06:23

wks requested changes Mar 9, 2022

View reviewed changes

qinsoon added 3 commits March 10, 2022 09:55

Address issues in the reviews.

11130c6

Fix build for 32 bits

13f37d1

Fix style check

db492f2

wks approved these changes Mar 10, 2022

View reviewed changes

qinsoon merged commit afa2718 into master Mar 10, 2022

qinsoon deleted the fix-object-ref-sft branch March 10, 2022 08:08

qinsoon added a commit to mmtk/mmtk-jikesrvm that referenced this pull request Mar 10, 2022

Set OBJECT_REF_OFFSET_BEYOND_CELL (#103)

be5114a

This PR updates MMTk core to mmtk/mmtk-core#555, and fixes #102

This was referenced Jun 17, 2022

Performance regression on mutator time for semispace in PR#555 #610

Closed

Minimize the scope of acquire_lock #614

Merged

qinsoon mentioned this pull request Nov 17, 2022

Refactor object reference to address #699

Merged

wks mentioned this pull request Apr 18, 2024

Kill off-heap ObjectReference, or make a safer to_address method #1120

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix object ref SFT #555

Fix object ref SFT #555

qinsoon commented Mar 6, 2022 •

edited

Loading

qinsoon commented Mar 9, 2022

wks Mar 9, 2022

qinsoon Mar 9, 2022

caizixian Mar 9, 2022 •

edited

Loading

qinsoon commented Mar 10, 2022

wks left a comment

Fix object ref SFT #555

Fix object ref SFT #555

Conversation

qinsoon commented Mar 6, 2022 • edited Loading

qinsoon commented Mar 9, 2022

wks Mar 9, 2022

Choose a reason for hiding this comment

qinsoon Mar 9, 2022

Choose a reason for hiding this comment

caizixian Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

qinsoon commented Mar 10, 2022

wks left a comment

Choose a reason for hiding this comment

qinsoon commented Mar 6, 2022 •

edited

Loading

caizixian Mar 9, 2022 •

edited

Loading