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

Expose alloc_slow. Add a section in user guide about allocation optimization #967

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Oct 3, 2023

This PR exposes alloc_slow() to the bindings, adds a few public methods to allow bindings to implement allocation efficiently without duplicating mmtk-core code, and adds a section in the user guide to discuss allocation optimization.

The changes in this PR includes:

  1. Expose alloc_slow() in memory_manager.
  2. Add Mutator::allocator() to allow bindings to get a specific allocator from an allocator selector. Add Mutator::allocator_impl() to allow bindings to get a typed allocator from a selector.
  3. Add Mutator::get_allocator_base_offset() to allow bindings to use a specific allocator without selector (for performance).
  4. Add a section in the user guide about allocation optimization. Remove some unused SUMMARY.md in the user guide.
  5. Add Address::as_mut_ref().
  6. Expose the field for the fastpath bump pointer in some allocators.

Related discussion on Zulip: https://mmtk.zulipchat.com/#narrow/stream/262679-General/topic/Refilling.20BumpPointer.20using.20AllocatorInfo/near/394142997

@qinsoon qinsoon marked this pull request as ready for review October 9, 2023 22:12
@qinsoon qinsoon requested a review from wks October 9, 2023 22:12
@playXE
Copy link
Contributor

playXE commented Oct 10, 2023

This looks good to me. I wonder if alloc_slow could be enhanced to accept &mut BumpPointer as well? So binding does not have to manually update cursor and limit fields. But I guess this won't work if binding does not embed BumpPointer in its own TLS.

docs/userguide/src/portingguide/perf_tuning/alloc.md Outdated Show resolved Hide resolved
docs/userguide/src/portingguide/perf_tuning/alloc.md Outdated Show resolved Hide resolved
vmbindings/dummyvm/src/tests/doc_mutator_storage.rs Outdated Show resolved Hide resolved
// Do slow path allocation with MMTk
let addr = default_allocator.alloc_slow(size, 8, 0);
// Copy bump pointer values to the fastpath BumpPointer
storage.default_bump_pointer = default_allocator.bump_pointer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: If the default_bump_pointer is initialized with (0, 0), it will be updated with meaningful values here in the first allocation.

docs/userguide/src/portingguide/perf_tuning/alloc.md Outdated Show resolved Hide resolved
docs/userguide/src/portingguide/perf_tuning/alloc.md Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
vmbindings/dummyvm/src/tests/allocator_info.rs Outdated Show resolved Hide resolved
@wks
Copy link
Collaborator

wks commented Oct 10, 2023

This looks good to me. I wonder if alloc_slow could be enhanced to accept &mut BumpPointer as well? So binding does not have to manually update cursor and limit fields. But I guess this won't work if binding does not embed BumpPointer in its own TLS.

That's right. VMs may store the cursor and limit in its own style, and may not have the same layout as our struct BumpPointer. I guess some VMs already has bump-pointer allocators in their own GC before adopting MMTk. The JIT compiler may even put the cursor and the limit in registers.

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

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Oct 10, 2023
@qinsoon qinsoon added this pull request to the merge queue Oct 11, 2023
Merged via the queue into mmtk:master with commit 0328b05 Oct 12, 2023
22 of 26 checks passed
@qinsoon qinsoon deleted the alloc-slow-api branch October 12, 2023 00:30
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.

3 participants