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

Add alloc_no_gc #1218

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add alloc_no_gc #1218

wants to merge 9 commits into from

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Oct 29, 2024

This PR adds allocation functions that are not GC safe points. On failed allocation, they will not attempt a GC and will just return a null address.

Our current alloc functions assume they are GC safe points and will trigger GCs internally. A runtime may have different assumptions. We see GHC has allocateMightFail (https://gitlab.haskell.org/ghc/ghc/-/blob/90746a591919fc51a0ec9dec58d8f1c8397040e3/rts/sm/Storage.c?page=2#L1089). Also Julia assumes perm alloc will not trigger a GC (mmtk/mmtk-julia#172).
Having a variant of alloc that is not GC safepoints could be generally useful.

@qinsoon qinsoon marked this pull request as ready for review October 30, 2024 02:37
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.

I changed parts of the doc comments of alloc, alloc_no_gc, alloc_slow and alloc_slow_no_gc. Particularly, I rewrote the comment of alloc to make it more ordered.

src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
@qinsoon
Copy link
Member Author

qinsoon commented Oct 30, 2024

I changed the documentation based on the suggestions.

/// normally without panicking or throwing exceptions, this function will return zero.
///
/// This function in most cases returns a valid memory address.
/// This function may return a zero address iif 1. MMTk attempts at least one GC,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// This function may return a zero address iif 1. MMTk attempts at least one GC,
/// This function may return a zero address if 1. MMTk attempts at least one GC,

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this paragraph should be removed.

src/util/alloc/allocator.rs Outdated Show resolved Hide resolved
src/plan/mutator_context.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Show resolved Hide resolved
src/util/alloc/allocator.rs Outdated Show resolved Hide resolved
@qinsoon qinsoon added the PR-extended-testing Run extended tests for the pull request label Oct 31, 2024
src/policy/space.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
qinsoon and others added 2 commits November 1, 2024 15:01
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
Copy link
Member Author

qinsoon commented Nov 1, 2024

I’m awaiting feedback from @JunmingZhao42 and Ben before merging this PR, as they requested this feature.

src/policy/space.rs Outdated Show resolved Hide resolved
Co-authored-by: Ben Gamari <ben@smart-cactus.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-extended-testing Run extended tests for the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants