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.

Sign up for GitHub

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

Jump to bottom

Uninitialized Fields #57

Closed
Tracked by #541
qinsoon opened this issue Mar 26, 2020 · 4 comments
Closed
Tracked by #541

Uninitialized Fields #57

qinsoon opened this issue Mar 26, 2020 · 4 comments
Assignees
@qinsoon
Labels
A-general Area: all code base (issues with this label may be divided into more concrete issues) C-refactoring Category: Refactoring S-stale:end: Status: Stale (will be closed soon)

Comments

@qinsoon
Copy link
Member

qinsoon commented Mar 26, 2020

In GitLab by @qinsoon on Jan 26, 2020, 11:44

A lot of structs in Rust MMTk are initialized with some uninitialized fields. We are using empty values (such as 0, OpaquePointer::EMPTY), mem::uninitialized(), or Option::None for the uninitialized fields.

The reasons why those uninitialized fields exist are:

  1. Rust MMTk used to create some structs as static variables, some fields are uninitialized. Rust MMTk relies on an extra init() step to properly set values for those fields.
  2. Initialize circular references.

I suggest we do these:

  1. As we are removing global states, we should remove the extra init() so that all the fields are initialized properly in new(). We should do this wherever possible.
  2. If we cannot remove init(), we should use a wrapper to properly indicate that this field might be uninitialized such as MaybeUninit<T>.
  3. Remove circular references if we can.

This issue is raised in code reviews for both https://gitlab.anu.edu.au/mmtk/mmtk/merge_requests/20 and https://gitlab.anu.edu.au/mmtk/mmtk/merge_requests/22.
@qinsoon qinsoon self-assigned this Mar 26, 2020
@qinsoon
Copy link
Member Author

qinsoon commented Mar 26, 2020

In GitLab by @qinsoon on Jan 26, 2020, 11:45

mentioned in merge request !22

@qinsoon
Copy link
Member Author

qinsoon commented Mar 26, 2020

In GitLab by @qinsoon on Jan 26, 2020, 19:10

mentioned in merge request !23

@qinsoon qinsoon added A-general Area: all code base (issues with this label may be divided into more concrete issues) C-refactoring Category: Refactoring labels Jun 5, 2020
@qinsoon qinsoon added this to the v0.2 milestone Nov 4, 2020
@qinsoon qinsoon removed this from the v0.2 milestone Dec 18, 2020
@qinsoon qinsoon mentioned this issue Mar 29, 2021
Merged
wenyuzhao referenced this issue in wenyuzhao/mmtk-core Sep 14, 2021
@qinsoon
Update to mmtk 0.3.0 (#57)
c3dc9d0
@qinsoon qinsoon mentioned this issue Jan 13, 2022
Merged
@qinsoon qinsoon mentioned this issue Feb 7, 2022
Closed
5 tasks
@qinsoon qinsoon mentioned this issue Jul 22, 2022
Merged
qinsoon added a commit that referenced this issue Aug 3, 2022
@qinsoon
Remove Plan.gc_init() and Space.init() (#629)
af2bfe7 
This pull request removes `Plan.gc_init()`, and `Space.init()`. I think this can close the issue: #57, I am not aware of any occurrences of two-step initialisation in MMTk after this PR.

Note that We eagerly set SFT for each space in their `Space.init()` if they are contiguous and SFT needs a non-moving reference to the `Space`. When we create a `Space`, the reference points to an address on the stack, and will be moved. So we cannot set SFT at creation. I added a method `Space.initialize_sft()` which is called after the `Space` has a fixed address. `initialize_sft()` is only used to set SFT, not any other post-creation initialisation.
@qinsoon
Copy link
Member Author

qinsoon commented Aug 8, 2022

We have gradually removed the pattern. I am not aware of any unnecessary two-step initialisation in our code base. I feel we can close this issue now. If you find anything that is related with this issue and hasn't been addressed, please reply to this issue. Otherwise, I will close this issue soon.

@qinsoon qinsoon added the S-stale:end: Status: Stale (will be closed soon) label Aug 8, 2022
@k-sareen
Copy link
Collaborator

Closed as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@qinsoon qinsoon

Labels
A-general Area: all code base (issues with this label may be divided into more concrete issues) C-refactoring Category: Refactoring S-stale:end: Status: Stale (will be closed soon)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@qinsoon @k-sareen