Implement AllocatorInfo #889

playXE · 2023-08-05T08:49:47Z

This type adds AllocatorInfo enum that can be used to query offset of allocator fields for fast-path generation. Currently all bindings have to re-declare *Allocator types with the same layout as in mmtk-core in order to get offset of these fields. This enum simplifies that, all you need to do is invoke get_allocator_info(selector).

qinsoon

I think the changes look good. This is something we need to properly support compiler generated allocation fastpath.

src/util/alloc/mod.rs

src/memory_manager.rs

qinsoon · 2023-08-07T01:47:46Z

src/util/alloc/allocators.rs

+        limit_offset: usize,
+        top_offset: usize,
+    },
+    Immix {


I think you can just remove the Immix variant as well. Immix's overflow_alloc (which uses the large cursor/limit) and alloc_slow_hot (which tries to find a recyclable line) is considered as the 'intermediate' path (i.e. between fastpath and slowpath). Normally the compiler don't need to inline it. In that case, generating allocation fastpath for Immix would be the same as BumpPointer fastpath (just checking cursor/limit, and adjust cursor).

Should not small limit still be checked? Or am I wrong here?

You don't have to check "small_limit". If the allocation can fit into the normal buffer (cursor/limit), it will be allocated in the buffer. However, the buffer can be as small as one line (depending on how fragment the heap is). So sometimes the allocation that is larger than one line can't fit into the normal buffer, then the large cursor/limit is used. But that is not in the fastpath path any more.

The actual code that is considered as Immix's fastpath is (which is as simple as a normal bump pointer allocation):

mmtk-core/src/util/alloc/immix_allocator.rs

Lines 75 to 78 in 4c1e58a

let result = align_allocation_no_fill::<VM>(self.cursor, align, offset);

let new_cursor = result + size;

if new_cursor > self.limit {

and

mmtk-core/src/util/alloc/immix_allocator.rs

Lines 90 to 102 in 4c1e58a

} else {

// Simple bump allocation.

fill_alignment_gap::<VM>(self.cursor, result);

self.cursor = new_cursor;

trace!(

"{:?}: Bump allocation size: {}, result: {}, new_cursor: {}, limit: {}",

self.tls,

size,

result,

self.cursor,

self.limit

);

result

I remembered @wenyuzhao mentioning something about this in-person to me (Re: limit for Immix allocator being 256 bytes). Maybe he can chime in and clarify expectations.

src/memory_manager.rs

src/util/alloc/allocators.rs

wks · 2023-08-07T02:42:02Z

src/util/alloc/bumpallocator.rs

+    pub(crate) cursor: Address,
    /// Limit for bump pointer
-    limit: Address,
+    pub(crate) limit: Address,


We can make a #[repr(C)] struct for these two fields since it appears so often. Then we only need to return one offset for those two fields.

Should this be done in this PR as well?

Yes, please. This is the best chance to do it.

How this structure should be named? And also, where should I put it?

Good question. I think we should discuss on Zulip and let everyone vote.

It should be defined in allocator.rs because it is common to many concrete allocators.

This PR seems a step in the right direction. Thank you!

Placing cursor and limit in a struct seems sensible.

The cursor and limit should be called a bump pointer, because that is literally what it is (edit: at least that's what I think you're doing, from my read). I teach people how GCs are built and this is literally how I describe a bump pointer: a cursor and a limit. Inventing a new term to describe this most basic of data structures is not a good idea.

Meta point: I don't think "voting" on conceptual points like this on Zulip is a great idea in general.

src/memory_manager.rs

playXE · 2023-08-14T14:06:17Z

So far what would be the next proper steps to get this PR merged? I am actually now all in for idea of bindings storing bump-pointers/free-lists on their side and syncing with MMTk when necessary. BDWGC works this way by providing support for inline allocation by allowing user-code to create free-lists.

cc @steveblackburn @qinsoon

qinsoon · 2023-08-14T23:33:05Z

So far what would be the next proper steps to get this PR merged? I am actually now all in for idea of bindings storing bump-pointers/free-lists on their side and syncing with MMTk when necessary. BDWGC works this way by providing support for inline allocation by allowing user-code to create free-lists.

cc @steveblackburn @qinsoon

It seems there are two options:

Expose the offset of cursor/limit (and similar fields for other allocators), or
Expose the values of cursor/limit.

My preference is Option 1. It covers what Option2 does, and is more flexible for the bindings. Option 2 enforces the binding to create their own fastpath data structures, and sync values with MMTk. I am not sure what others think about. @steveblackburn @wks

We will get this resolved no later than tomorrow (we have a meeting tomorrow and I will raise this if necessary).

qinsoon · 2023-08-16T03:52:30Z

So far what would be the next proper steps to get this PR merged? I am actually now all in for idea of bindings storing bump-pointers/free-lists on their side and syncing with MMTk when necessary. BDWGC works this way by providing support for inline allocation by allowing user-code to create free-lists.
cc @steveblackburn @qinsoon

It seems there are two options:

Expose the offset of cursor/limit (and similar fields for other allocators), or

Expose the values of cursor/limit.

My preference is Option 1. It covers what Option2 does, and is more flexible for the bindings. Option 2 enforces the binding to create their own fastpath data structures, and sync values with MMTk. I am not sure what others think about. @steveblackburn @wks

We will get this resolved no later than tomorrow (we have a meeting tomorrow and I will raise this if necessary).

@playXE

We discussed this. Exposing field offsets to the binding is a good idea. It allows the JIT compiler to generate code, and allows the runtime to load/store the values.

A refinement on the current PR we plan to do is that we could have public fastpath structs with #[repr(C)], and expose the offset to the struct instead of exposing offsets to separate fields. Such as

#[repr(C)]
pub struct BumpPointer {
  pub cursor: Address,
  pub limit: Address,
}

struct BumpAllocator {
  bump_pointer: BumpPointer
  ...
}

pub enum AllocatorInfo {
    BumpPointer(usize), // offset from mutator to the BumpPointer struct
    // FIXME: Add free-list fast-path
    Unimplemented,
    None,
}

This change does not have to be included in the PR. Just FYI, as we may change the actual interface in the future. But we will follow the general idea of exposing offsets to fields for bindings.

If this sounds good to you, you can make the PR ready, and we can proceed from there. Thanks for the PR.

playXE · 2023-08-17T12:00:41Z

@qinsoon I see, I'll implement BumpPointer in this PR as well, I need to get fast-path allocations in my runtime working now. Will do that on weekends probably, right now busy with getting into new job.

Cargo.toml

src/util/alloc/allocators.rs

src/util/alloc/immix_allocator.rs

qinsoon · 2023-08-21T00:42:05Z

I am trying to push some changes to the PR. Mainly about the new method BumpPointer::alloc. Though it is better to provide such a method that can be reused, it introduce an extra conditional check in the fastpath that the Rust compiler does not optimize away. I think it would be a better idea to revert that part.

fields only public in util::alloc

qinsoon

LGTM

playXE added 4 commits August 5, 2023 12:48

add AllocatorInfo

85f34e3

Fix offset calculation

7c726ff

fix style

748e28f

fix style 2

3d4752b

qinsoon reviewed Aug 7, 2023

View reviewed changes

wks reviewed Aug 7, 2023

View reviewed changes

move get_allocator_info to allocators.rs

5dc7d32

caizixian mentioned this pull request Aug 14, 2023

Only expose essential fields of Mutator as repr(C) #862

Closed

playXE added 2 commits August 20, 2023 11:02

Merge branch 'master' into allocator-info

e51b8cd

implement BumpPointer type

96d3c79

playXE marked this pull request as ready for review August 20, 2023 08:12

playXE requested a review from qinsoon August 20, 2023 08:12

k-sareen reviewed Aug 21, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/util/alloc/allocators.rs Outdated Show resolved Hide resolved

src/util/alloc/immix_allocator.rs Outdated Show resolved Hide resolved

qinsoon added 2 commits August 21, 2023 00:52

Remove BumpPointer.alloc. Address some comments.

d8c9eba

Remove some code that was accidentally included. Make bump pointer

ff456e5

fields only public in util::alloc

qinsoon force-pushed the allocator-info branch from 84b6b61 to ff456e5 Compare August 21, 2023 01:22

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

qinsoon approved these changes Aug 22, 2023

View reviewed changes

qinsoon merged commit 9175e68 into mmtk:master Aug 22, 2023
21 of 22 checks passed

caizixian mentioned this pull request Oct 26, 2023

Make Mutator struct smaller #1001

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement AllocatorInfo #889

Implement AllocatorInfo #889

playXE commented Aug 5, 2023

qinsoon left a comment

qinsoon Aug 7, 2023

playXE Aug 7, 2023

qinsoon Aug 7, 2023

k-sareen Aug 7, 2023 •

edited

wks Aug 7, 2023

playXE Aug 7, 2023

wks Aug 7, 2023

playXE Aug 7, 2023 •

edited

wks Aug 7, 2023

steveblackburn Aug 8, 2023 •

edited

playXE commented Aug 14, 2023

qinsoon commented Aug 14, 2023

qinsoon commented Aug 16, 2023

playXE commented Aug 17, 2023

qinsoon commented Aug 21, 2023

qinsoon left a comment

	let result = align_allocation_no_fill::<VM>(self.cursor, align, offset);
	let new_cursor = result + size;

	if new_cursor > self.limit {

	} else {
	// Simple bump allocation.
	fill_alignment_gap::<VM>(self.cursor, result);
	self.cursor = new_cursor;
	trace!(
	"{:?}: Bump allocation size: {}, result: {}, new_cursor: {}, limit: {}",
	self.tls,
	size,
	result,
	self.cursor,
	self.limit
	);
	result

Implement AllocatorInfo #889

Implement AllocatorInfo #889

Conversation

playXE commented Aug 5, 2023

qinsoon left a comment

Choose a reason for hiding this comment

qinsoon Aug 7, 2023

Choose a reason for hiding this comment

playXE Aug 7, 2023

Choose a reason for hiding this comment

qinsoon Aug 7, 2023

Choose a reason for hiding this comment

k-sareen Aug 7, 2023 • edited

Choose a reason for hiding this comment

wks Aug 7, 2023

Choose a reason for hiding this comment

playXE Aug 7, 2023

Choose a reason for hiding this comment

wks Aug 7, 2023

Choose a reason for hiding this comment

playXE Aug 7, 2023 • edited

Choose a reason for hiding this comment

wks Aug 7, 2023

Choose a reason for hiding this comment

steveblackburn Aug 8, 2023 • edited

Choose a reason for hiding this comment

playXE commented Aug 14, 2023

qinsoon commented Aug 14, 2023

qinsoon commented Aug 16, 2023

playXE commented Aug 17, 2023

qinsoon commented Aug 21, 2023

qinsoon left a comment

Choose a reason for hiding this comment

k-sareen Aug 7, 2023 •

edited

playXE Aug 7, 2023 •

edited

steveblackburn Aug 8, 2023 •

edited