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

Clarify contract/semantics of "Release" and other work buckets #717

Open
k-sareen opened this issue Dec 21, 2022 · 5 comments
Open

Clarify contract/semantics of "Release" and other work buckets #717

k-sareen opened this issue Dec 21, 2022 · 5 comments
Labels
C-bug Category: Bug P-normal Priority: Normal.

Comments

@k-sareen
Copy link
Collaborator

Currently the semantics/contract for certain work buckets, such as "Release" are unclear (see discussion in #714). We should clarify the semantics of all the work buckets in documentation. Semantics can include unsafety, can other work packets execute concurrently along side, etc.

@qinsoon
Copy link
Member

qinsoon commented Dec 21, 2022

To be specific, Prepare and Release will get a mutable reference to Plan with unsafe code. To avoid race condition, we need to make sure one of these conditions is true:

  1. there is no no other work packet in those work bucket that is executing in parallel with Prepare or Release,
  2. other work packets that run in parallel with Prepare/Release should not access Plan,
  3. other work packets that run in parallel with Prepare/Release do not have race conditions with Plan.prepare/release.

@k-sareen
Copy link
Collaborator Author

Seems like semantics of the Release work bucket are reclamation which aligns with how it is currently, however we may further need to discuss if work packets should be allowed to execute alongside the Release work packet as the Release work packet unsafely accesses the Plan.

@qinsoon
Copy link
Member

qinsoon commented Jan 11, 2023

@wks also raised an issue in #724 (comment). We may have more and more work packets that run in parallel with Plan::release. It will be more difficult to argue the correctness of the unsafe mutable &mut Plan in the release phase. Maybe we should just remove the unsafe mutable reference for Plan::prepare/release, and use a safer implementation.

@k-sareen
Copy link
Collaborator Author

k-sareen commented Jan 11, 2023

I think it may be convenient and safe to do Plan::release in the EndOfGc work packet. At which point we should rename the Plan::release function to something else.

@qinsoon
Copy link
Member

qinsoon commented Jan 12, 2023

I think it may be convenient and safe to do Plan::release in the EndOfGc work packet. At which point we should rename the Plan::release function to something else.

There is some code in Plan::release which should really be in EndOfGC. We can move them, like #714. But moving the entire Plan::release to EndofGC does not seem to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Bug P-normal Priority: Normal.
Projects
None yet
Development

No branches or pull requests

3 participants