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

Store pinned static fields in the Pinned Object Heap. #89895

Merged
merged 8 commits into from
Sep 22, 2023

Conversation

teo-tsirpanis
Copy link
Contributor

This PR allocates fields marked with [FixedAddressValueType] to the Pinned Object Heap. We introduce a new overload of AllocateObject that accepts a GC_ALLOC_FLAGS, and when allocating the static fields we pass GC_ALLOC_PINNED_OBJECT_HEAP if the field has that attribute.

Because we don't use a pinned handle to pin the fields anymore, the object is not permanently held in memory, which enables unloadability for assemblies with pinned static fields. All we have to do is remove the check that blocked these assemblies, and unmark the relevant unit test as incompatible with unloadability. I also removed a now-unused tracking mechanism for thread-static pinned fields.

Fixes #66043

Now that we don't pin `FixedAddressValueType` fields, this mechanism is unused.
We remove the check that blocked it, enable a test and update a design document.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 2, 2023
@ghost
Copy link

ghost commented Aug 2, 2023

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR allocates fields marked with [FixedAddressValueType] to the Pinned Object Heap. We introduce a new overload of AllocateObject that accepts a GC_ALLOC_FLAGS, and when allocating the static fields we pass GC_ALLOC_PINNED_OBJECT_HEAP if the field has that attribute.

Because we don't use a pinned handle to pin the fields anymore, the object is not permanently held in memory, which enables unloadability for assemblies with pinned static fields. All we have to do is remove the check that blocked these assemblies, and unmark the relevant unit test as incompatible with unloadability. I also removed a now-unused tracking mechanism for thread-static pinned fields.

Fixes #66043

Author: teo-tsirpanis
Assignees: -
Labels:

area-AssemblyLoader-coreclr

Milestone: -

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

src/coreclr/vm/gchelpers.h Show resolved Hide resolved
@janvorli
Copy link
Member

janvorli commented Aug 3, 2023

@teo-tsirpanis you'll also need to remove the library negative test for the FixedAddressValueType attribute and unloadability - it is causing the CI failures.

src/coreclr/vm/gchelpers.h Outdated Show resolved Hide resolved
@MichalPetryka
Copy link
Contributor

Because we don't use a pinned handle to pin the fields anymore, the object is not permanently held in memory, which enables unloadability for assemblies with pinned static fields.

Worth noting is that this will unload the assembly when the user uses Unsafe.AsPointer on the field since nothing with keep it alive and then such access could corrupt the process.

@mangod9
Copy link
Member

mangod9 commented Sep 18, 2023

@teo-tsirpanis, could you please rebase the PR and then we could run the CI again to get it merged? Thanks

@teo-tsirpanis
Copy link
Contributor Author

@mangod9 done. CI is green.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit f61d3c7 into dotnet:main Sep 22, 2023
157 checks passed
@teo-tsirpanis teo-tsirpanis deleted the fixedaddressvaluetype-poh branch September 22, 2023 10:21
@teo-tsirpanis teo-tsirpanis mentioned this pull request Sep 23, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store static fields with [FixedAddressValueType] in the Pinned Object Heap.
6 participants