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

Adding macros to push pinning roots #43

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

udesou
Copy link

@udesou udesou commented Mar 19, 2024

Supporting pushing roots to the shadow stack that are not transitively pinned (only the root object is pinned), and therefore should enable moving more objects.

For backwards compatibility the original functions will be used to represent transitively pinned roots, whereas the *_RELAXED version will represent roots in which only the root object needs to be pinned. Therefore, to support moving move objects the goal is to replace as much as possible of the original functions with their _RELAXED counterpart.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

I suggest not using 'relaxed' in the name. When I saw 'relaxed', I first thought about memory order. And it always takes an extra step to associate 'relaxed' with 'not transitively pin'. You can use a more direct naming, e.g. push_tpin vs push_no_tpin.

#define JL_GC_ENCODE_PUSH(n) ((((size_t)(n))<<3)|1)

// these only pin the root object itself
#define JL_GC_ENCODE_PUSHARGS_NO_TPIN(n) (((size_t)(n))<<3|4)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if Julia documents this anywhere. It is a good idea to explain what those bits are used for.

Copy link
Author

Choose a reason for hiding this comment

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

There seems to be three different functions:

#define JL_GC_ENCODE_PUSHARGS(n)   (((size_t)(n))<<2)
#define JL_GC_ENCODE_PUSH(n)       ((((size_t)(n))<<2)|1)
#define JL_GC_ENCODE_PUSHFRAME(n)  ((((size_t)(n))<<2)|2)

I think they currently use 2 bits (<<2). It seems to indicate what stuff you'd push onto the shadow stack, but I might ask around so we can document this properly.

@qinsoon
Copy link
Member

qinsoon commented Apr 24, 2024

I am wondering if we should merge this. We need the changes in this PR for the static analyser as well. @udesou

@udesou
Copy link
Author

udesou commented Apr 24, 2024

I am wondering if we should merge this. We need the changes in this PR for the static analyser as well. @udesou

Sounds good to me. I've added a comment to explain how the bits work, in particular for transitively pinning roots. If you have no issues with the PR, then we can merge it.

@qinsoon qinsoon merged commit 7d89137 into mmtk:v1.9.2+RAI Apr 29, 2024
1 check passed
qinsoon pushed a commit to mmtk/mmtk-julia that referenced this pull request Apr 29, 2024
Supporting processing roots in the shadow stack that are not
transitively pinned (only the root object is pinned), and therefore
should enable moving more objects.

NB: Should be merged with mmtk/julia#43.
qinsoon added a commit to qinsoon/julia that referenced this pull request Nov 6, 2024
qinsoon added a commit that referenced this pull request Nov 7, 2024
Port #43 to dev. Add _NO_TPIN macros for pushing GC frames.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants