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

Malloc API #608

Merged
merged 11 commits into from
Jun 25, 2022
Merged

Malloc API #608

merged 11 commits into from
Jun 25, 2022

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Jun 7, 2022

This PR adds a set of malloc related API, and another set of malloc API that will count allocation size into MMTk heap under the feature malloc_counted_size. This PR adds things described in #572 (comment).

The changes in this PR:

  • tidy up the malloc library import
  • provide a set of standard malloc functions (malloc, calloc, realloc and free)
  • provide a set of malloc functions that will count the malloc size into MMTk heap under the feature malloc_counted_size
  • provide gc_poll() that can be used to check GC when a binding uses counted malloc functions. Small refactoring to allow poll() without a space reference.

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Jun 23, 2022
@qinsoon qinsoon marked this pull request as ready for review June 23, 2022 00:13
@qinsoon qinsoon requested a review from wks June 23, 2022 05:00
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.

Mostly OK. I only have one question and one suggestion.

) -> bool {
let nursery_full = self.nursery.reserved_pages()
>= (conversions::bytes_to_pages_up(*self.common.base.options.max_nursery));
if nursery_full {
return true;
}

if space_full && space.common().descriptor != self.nursery.common().descriptor {
if space_full
&& space.is_some()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This chain of && means "if space is None, then the next GC will not be full-heap". Is this intentional? This means memory_manager::gc_poll will never trigger full-heap GC directly.

If you mean, "if space_full is true and space is None, then trigger full-heap immediately", it should be space_full && (!space.is_some() || space.unwrap().common().descriptor != self.nursery.common().descriptor).

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. That was a mistake. I will change the code to this:

        // Is the GC triggered by nursery?
        // - if space is none, it is not. Return false immediately.
        // - if space is some, we further check its descriptor.
        let is_triggered_by_nursery = space.map_or(false, |s| s.common().descriptor == self.nursery.common().descriptor);
        // If space is full and the GC is not triggered by nursery, next GC will be full heap GC.
        if space_full && !is_triggered_by_nursery {
            self.next_gc_full_heap.store(true, Ordering::SeqCst);
        }

Self {
content: AtomicRefCell::new(None),
once: Once::new(),
serial_lock: Some(Mutex::default()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a fixture is serial, then neither AtomicRefCell nor Once are necessary. It is easier to create a dedicated SerialFixture type, and wrap both Option<Box<T>> and an initialized flag into a big Mutex.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added a SerialFixture.

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 merged commit a424ef2 into mmtk:master Jun 25, 2022
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