Skip to content

Use atomic operations for SFT map and remove unsafe code #931

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

Merged
merged 7 commits into from
Sep 5, 2023

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Aug 31, 2023

This PR removes unsafe code from SFT map.

  • For most SFT map implementations, we only need mutability for the cells in the table that stores SFT pointers. We do not need mutability for the table itself. In this case, using atomic operations solves the problem.
  • For DenseChunkMap, we do need to mutate the table. But we only mutate the table during plan creation. We can use unsafe code to get a mutable reference to SFT map during plan creation (and it is easy to reason that getting a mutable reference here is correct).

Changes in the PR

  • Add a type SFTRefStorage which is internally implemented as a double word atomic integer (u128 or u64). We store SFTRefStorage in SFT maps.
  • Repurpose SFTMap::eager_initialize to SFTMap::initialize_for_space. It has to be called for each space created during plan creation time, and it takes a mutable reference to the SFTMap to allow mutating. start and size is changed to optional now, as `Option<(Address, usize)>. If the range is given, we will eagerly initialize SFT map for the region.
  • Space::initialize_sft(&self) is changed to Space::initialize_sft(&self, sft_map: &mut dyn SFTMap) to allow mutating SFT map.
  • Add InitializeOnce::get_mut.
  • Bump MSRV to 1.70 due to Use atomic operations for SFT map and remove unsafe code #931 (comment).

@qinsoon
Copy link
Member Author

qinsoon commented Aug 31, 2023

I bumped MSRV in this PR to 1.70, as Rust with a version below 1.70 failed to build this PR with the following error on x86_64-apple-darwin.

cargo build --release --features vm_space,ro_space,code_space,vo_bit,is_mmtk_object,object_pinning,immix_non_moving,immix_smaller_block,immix_zero_on_release,sanity,analysis,nogc_lock_free,nogc_no_zeroing,single_worker,extreme_assertions,nogc_multi_space,work_packet_stats,malloc_counted_size,count_live_bytes_in_gc
   Compiling proc-macro2 v1.0.66
   Compiling unicode-ident v1.0.11
   Compiling autocfg v1.1.0
   Compiling libc v0.2.147
   Compiling pkg-config v0.3.27
   Compiling quote v1.0.33
   Compiling jobserver v0.1.26
   Compiling cfg-if v1.0.0
   Compiling syn v2.0.29
   Compiling cc v1.0.83
   Compiling crossbeam-utils v0.8.16
   Compiling vcpkg v0.2.15
   Compiling thiserror v1.0.47
   Compiling tinyvec_macros v0.1.1
   Compiling tinyvec v1.6.0
   Compiling libz-sys v1.1.12
   Compiling thiserror-impl v1.0.47
   Compiling memoffset v0.9.0
   Compiling scopeguard v1.2.0
   Compiling serde v1.0.188
   Compiling serde_derive v1.0.188
   Compiling unicode-normalization v0.1.22
   Compiling crossbeam-epoch v0.9.15
   Compiling memchr v2.6.2
   Compiling ucd-trie v0.1.6
   Compiling unicode-bidi v0.3.13
   Compiling version_check v0.9.4
   Compiling percent-encoding v2.3.0
   Compiling form_urlencoded v1.2.0
   Compiling idna v0.4.0
   Compiling pest v2.7.3
   Compiling libgit2-sys v0.14.2+1.5.1
   Compiling semver v1.0.18
   Compiling syn v1.0.109
   Compiling semver-parser v0.10.2
   Compiling url v2.4.1
   Compiling semver v0.11.0
   Compiling crossbeam-deque v0.8.3
   Compiling proc-macro-error-attr v1.0.4
   Compiling crossbeam-channel v0.5.8
   Compiling log v0.4.20
   Compiling toml v0.5.11
   Compiling rayon-core v1.11.0
   Compiling bitflags v1.3.2
   Compiling rustix v0.38.10
   Compiling cargo-lock v8.0.3
   Compiling aho-corasick v1.0.5
   Compiling rustc_version v0.3.3
   Compiling proc-macro-error v1.0.4
   Compiling errno v0.3.3
   Compiling num_cpus v1.16.0
   Compiling lock_api v0.4.10
   Compiling rustversion v1.0.14
   Compiling regex-syntax v0.7.5
   Compiling bitflags v2.4.0
   Compiling either v1.9.0
   Compiling crossbeam-queue v0.3.8
   Compiling regex-automata v0.3.7
   Compiling atomic-traits v0.3.0
   Compiling num-traits v0.2.16
   Compiling portable-atomic v1.4.3
   Compiling rayon v1.7.0
   Compiling regex v1.9.4
   Compiling is-terminal v0.4.9
   Compiling enum-map-derive v0.13.0
   Compiling core-foundation-sys v0.8.4
   Compiling termcolor v1.2.0
   Compiling humantime v2.1.0
   Compiling heck v0.4.1
   Compiling env_logger v0.10.0
   Compiling strum_macros v0.24.3
   Compiling enum-map v2.6.1
   Compiling sysinfo v0.29.9
   Compiling crossbeam v0.8.2
   Compiling spin v0.9.8
   Compiling mmtk-macros v0.19.0 (/Users/runner/work/mmtk-core/mmtk-core/macros)
   Compiling itertools v0.10.5
   Compiling delegate v0.9.0
   Compiling static_assertions v1.1.0
   Compiling atomic_refcell v0.1.11
   Compiling atomic v0.5.3
   Compiling strum v0.24.1
   Compiling downcast-rs v1.2.0
   Compiling lazy_static v1.4.0
   Compiling probe v0.5.1
   Compiling git2 v0.16.1
   Compiling built v0.6.0
   Compiling mmtk v0.19.0 (/Users/runner/work/mmtk-core/mmtk-core)
LLVM ERROR: Unsupported expression in static initializer: or (i128 shl (i128 zext (i64 ptrtoint (ptr @anon.9972499f6654e220d[244](https://github.com/mmtk/mmtk-core/actions/runs/6033168965/job/16369468550?pr=931#step:5:245)6f93ea31492b.6.llvm.7316330882300979583 to i64) to i128), i128 64), i128 zext (i64 ptrtoint (ptr @anon.997[249](https://github.com/mmtk/mmtk-core/actions/runs/6033168965/job/16369468550?pr=931#step:5:250)9f6654e220d2446f93ea31492b.1.llvm.7316330882[300](https://github.com/mmtk/mmtk-core/actions/runs/6033168965/job/16369468550?pr=931#step:5:301)979583 to i64) to i128))

@qinsoon
Copy link
Member Author

qinsoon commented Sep 1, 2023

Performance-wise, this PR slows down SFT slightly, but should not introduce any measurable overhead.

DaCapo

Measuring with Immix at 2.5x MC min heap (20 invocations). plotty
atomic-sft

The huge slowdown for lusearch should be noise. I did a rerun for lusearch with the same configuration and the same build (with 40 invocations). The second run shows no performance difference. plotty
atomic-sft-lusearch

Micro benchmarks

Using criterion (see #933) to measure the performance of is_in_mmtk_spaces (which uses SFT map). There is a slight slowdown for this benchmark.

  • Before the PR:
    sft read time: [2.7674 ns 2.7683 ns 2.7695 ns]
  • In this PR
    sft read time: [3.2146 ns 3.2263 ns 3.2380 ns]

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Sep 1, 2023
@qinsoon
Copy link
Member Author

qinsoon commented Sep 1, 2023

The Julia test is failing right now, as mmtk/mmtk-julia#71 is not yet merged (I enabled auto merge for the PR). Once it is merged, the Julia tests will probably time out due to mmtk/mmtk-julia#94 (comment).

@qinsoon qinsoon marked this pull request as ready for review September 1, 2023 05:55
@qinsoon qinsoon requested review from wks and removed request for wks September 1, 2023 05:55
@qinsoon qinsoon mentioned this pull request Sep 4, 2023
@qinsoon
Copy link
Member Author

qinsoon commented Sep 4, 2023

This PR is ready for review now.

@qinsoon qinsoon requested a review from wks September 4, 2023 05:51
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM. But the place where it goes through each space to call notify_space_creation conflicts with #934, and need to be rebased.

p.s. #934 allows spaces to be enumerated mutably (with &mut dyn Space to each space). It is applicable to the call site in create_plan. I am not sure if it can provide new ways to implement some of the updatings in this PR, but if you need that, you can call mmtk.for_each_space_mut.


/// Get a mutable reference to the value.
/// This is currently only used for SFTMap during plan creation (single threaded),
/// and before the plan creation is done, the binding cannot use MMTK at all.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. This is an evidence that VMMap should be owned by an MMTk instance so that we have mutable access to the VMMap instance when creating MMTk and the concrete Plan. It should be part of the refactoring discussed in #932

@qinsoon qinsoon added this pull request to the merge queue Sep 5, 2023
Merged via the queue into mmtk:master with commit 253e882 Sep 5, 2023
@qinsoon qinsoon deleted the sft-atomic-double-word branch September 5, 2023 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants