Skip to content

Differentiate OBJ_PIN and PTR_PIN. Add more pinning. Trace all global roots.#84

Merged
qinsoon merged 5 commits intommtk:devfrom
qinsoon:final-moving
Feb 5, 2025
Merged

Differentiate OBJ_PIN and PTR_PIN. Add more pinning. Trace all global roots.#84
qinsoon merged 5 commits intommtk:devfrom
qinsoon:final-moving

Conversation

@qinsoon
Copy link
Copy Markdown
Member

@qinsoon qinsoon commented Jan 8, 2025

This PR

  • differentiates OBJ_PIN from PTR_PIN. In rare cases, we have to deal with internal pointers, and have to use PTR_PIN.
  • pins more objects that cannot be moved.
  • traces all the JL_GLOBALLY_ROOTED symbols.

This PR still transitively pins jl_global_roots_list. Without transitive pinning, we observed assertions failures in the precompilation step during Julia build, saying we reach objects without the valid object bit. This issue will be debugged and fixed after this PR.

@qinsoon qinsoon changed the title Remove transitive pinning of global roots Differentiate OBJ_PIN and PTR_PIN. Add more pinning. Trace all global roots. Jan 29, 2025
@qinsoon qinsoon marked this pull request as ready for review January 29, 2025 03:01
@qinsoon qinsoon requested a review from udesou January 29, 2025 03:05
Copy link
Copy Markdown

@udesou udesou left a comment

Choose a reason for hiding this comment

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

A general note is that you should probably add comments to all the points you're pinning objects (maybe not for the *HASH_PIN ones).

// Typenames should be pinned since they are used as metadata, and are
// read during scan_object
PTR_PIN(tn);
OBJ_PIN(tn);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should typenames be allocated in a non-moving space? Maybe add a FIXME comment saying that instead of pinning them, we might do that instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have a commit to add an allocation function jl_gc_alloc_non_moving_ and use it for cases like this. Do you want me to include the change in this PR? I planned to have it as a separate PR after this one, but I can include it here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This commit: qinsoon@70a4e14

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Happy for it to be a separate PR. 👍


#define TRACE_GLOBALLY_ROOTED(r) add_node_to_roots_buffer(closure, buf, buf_len, r)

// This is a list of global variables that are marked with JL_GLOBALLY_ROOTED. We need to make sure that they
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For now this is fine, but I wonder whether we should try to capture this automatically somehow. It's a bit of a pain that you had to add all these variables manually.

Copy link
Copy Markdown

@udesou udesou 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 f1b7ccc into mmtk:dev Feb 5, 2025
4 checks passed
qinsoon added a commit to qinsoon/julia that referenced this pull request Feb 6, 2025
… roots. (mmtk#84)

This PR
* differentiates `OBJ_PIN` from `PTR_PIN`. In rare cases, we have to deal with internal pointers, and have to use `PTR_PIN`.
* pins more objects that cannot be moved.
* traces all the `JL_GLOBALLY_ROOTED` symbols.

This PR still transitively pins `jl_global_roots_list`. Without transitive pinning, we observed assertions failures in the precompilation step during Julia build, saying we reach objects without the valid object bit. This issue will be debugged and fixed after this PR.
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