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

Conservative stack scanning #157

Merged
merged 34 commits into from
Aug 19, 2024
Merged

Conservative stack scanning #157

merged 34 commits into from
Aug 19, 2024

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Jun 19, 2024

This uses conservative stack scanning to find object references from the execution stack, and pins those values. The changes in the PR include:

  • Conservatively find object references from stacks and registers, and pin those references. The references are unpinned after the GC if they are still alive.
  • Calling jl_save_context_for_conservative_scanning for the first thread that is blocked for the GC.
  • Allow getting object size for large objects.
  • Refactoring and cleaning up features. We should choose either the moving or the non_moving feature.

@qinsoon qinsoon force-pushed the conserv-stack branch 2 times, most recently from c280432 to 4c6af3c Compare July 19, 2024 02:49
@qinsoon
Copy link
Member Author

qinsoon commented Aug 7, 2024

This PR passed tests with mmtk/julia#63. However, if we turn on stress copying in Immix, it segfaults in the building process. I am still looking into this.

@qinsoon
Copy link
Member Author

qinsoon commented Aug 15, 2024

This PR passed tests with mmtk/julia#63. However, if we turn on stress copying in Immix, it segfaults in the building process. I am still looking into this.

The problem is now solved. We further pin julia values in julia_to_scm_.

@qinsoon qinsoon marked this pull request as ready for review August 15, 2024 01:57
@qinsoon
Copy link
Member Author

qinsoon commented Aug 15, 2024

This PR may show slowdown from previous moving plans:

  1. the conservative stack scanning is unoptimized -- we conservatively scan the entire stack range and the entire machine context range (which includes registers), rather than only scan the active range.
  2. The object references found on the stack are saved to a global var with locking. This could cause contention.
  3. Some stacks are scanned multiple times. ptls->root_task, ptls->current_task, ptls->next_task, ptls->previous_task may point to the same task, the task's stack will be scanned multiple times.

I plan to focus on correctness and allowing more objects to be moved first, and do optimization in future PRs. Now we still want to remove the TPin for the global roots table. However, if requested, I can work on the performance right now within this PR.

@qinsoon qinsoon requested a review from udesou August 15, 2024 02:03
mmtk/Cargo.toml Outdated
is_mmtk_object = ["mmtk/is_mmtk_object"]
conservative = ["is_mmtk_object"]
moving = ["conservative", "object_pinning"]
Copy link
Contributor

@udesou udesou Aug 15, 2024

Choose a reason for hiding this comment

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

Should moving be in the default? Or non_moving?
Ideally, people can build withcargo build --features immix/stickyimmix and then get immix/sticky working with or without moving, whatever the default is. But as it is, I think we get moving but without conservative stack scanning which tpins everything I assume? That being said, maybe we should have non-moving as default?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can put it to default. But the default features will be included in all the builds, unless we pass --no-default-features. So when we build the non moving build, we will need to do cargo build --no-default-features --features non_moving,mmtk/vm_space,julia_copy_stack.

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 can revert his change to what we had before if you think that is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason that I changed this is that I found the following line confusing: object_pinning is turned on even for a non moving build.
#[cfg(all(feature = "object_pinning", not(feature = "non_moving")))]

When the changes in this PR, we will need conservative as a default feature to support moving. We will need to do the same thing for conservative.

I will see if there is a better way.

Copy link
Contributor

@udesou udesou Aug 15, 2024

Choose a reason for hiding this comment

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

If it's too difficult, it's also fine to leave as it is. The other thing to have in mind is that we should probably update the README file to reflect the changes - reminding people to set MMTK_CONSERVATIVE=1 for example, and perhaps add that you should have moving or non_moving as a feature when building the binding.

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 mostly revert back to what we had before. But I removed some features from conditional compilation in the binding crate, like is_mmtk_object, object_pinning. So we always build with those features, as they are default features -- we have to build with default features.

On top of default features, people may build with non_moving. With non_moving, some methods will do early return and skip the actual work. Like pinning, or conservative scanning.

mmtk/src/scanning.rs Outdated Show resolved Hide resolved
@udesou udesou self-requested a review August 19, 2024 00:14
@qinsoon qinsoon merged commit 173b338 into mmtk:v1.9.2+RAI Aug 19, 2024
17 checks passed
qinsoon added a commit that referenced this pull request Dec 18, 2024
This PR ports #157 and
#184 to dev.

It also adds an optimization that if a task is not started, the
conservative stack scanning can be skipped for the task.

Merge with mmtk/julia#80.

---------

Co-authored-by: Yi Lin <qinsoon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants