Skip to content

Remove helperframe marshal - #96860

Merged
jkotas merged 7 commits into
dotnet:mainfrom
AaronRobinsonMSFT:remove_helperframe_marshal
Jan 12, 2024
Merged

Remove helperframe marshal#96860
jkotas merged 7 commits into
dotnet:mainfrom
AaronRobinsonMSFT:remove_helperframe_marshal

Conversation

@AaronRobinsonMSFT

@AaronRobinsonMSFTAaronRobinsonMSFT commented Jan 11, 2024

Copy link
Copy Markdown
Member

Convert Marshal API FCalls with HelperMethodFrames into QCalls.

@AaronRobinsonMSFT
Remove HelperMethodFrames from Marshal APIs
7eb5710
Convert Marshal.GetHRForException and Marshal.GetExceptionForHRInternal to QCalls.
@AaronRobinsonMSFT
Convert OffsetOfHelper to QCall
36bc210
@AaronRobinsonMSFTAaronRobinsonMSFT added this to the 9.0.0 milestone Jan 11, 2024
davidwrighton
davidwrighton requested changes Jan 11, 2024
View reviewed changes

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OffsetOfInternal shouldn't take a field object, it should just be implemented as taking a FieldDesc* as input. Then it won't need anything to do with wonking around with the GC state at all, and can operate entirely in preemptive mode

@ghostghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 11, 2024
@ghostghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 11, 2024
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Jan 11, 2024

Copy link
Copy Markdown
MemberAuthor

OffsetOfInternal shouldn't take a field object, it should just be implemented as taking a FieldDesc* as input. Then it won't need anything to do with wonking around with the GC state at all, and can operate entirely in preemptive mode

@davidwrighton Done.

jkotas
jkotas reviewed Jan 11, 2024
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs Outdated
jkotas
jkotas reviewed Jan 11, 2024
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs Outdated
jkotas
jkotas reviewed Jan 11, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/marshalnative.cpp Outdated
davidwrighton
davidwrighton approved these changes Jan 11, 2024
View reviewed changes

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just one more nit... around unnecessary GCX_PREEMP();

Comment threadsrc/coreclr/vm/marshalnative.cpp Outdated
jkotas
jkotas reviewed Jan 12, 2024
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs Outdated
jkotas
jkotas reviewed Jan 12, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/marshalnative.cpp Outdated
jkotas
jkotas approved these changes Jan 12, 2024
View reviewed changes

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!

This was referenced Jan 12, 2024
Closed
Closed
@jkotas
jkotas merged commit 22ba7d6 into dotnet:mainJan 12, 2024
@AaronRobinsonMSFT
AaronRobinsonMSFT deleted the remove_helperframe_marshal branch January 12, 2024 04:52
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@AaronRobinsonMSFT@jkotas@tmds
Remove helperframe marshal (dotnet#96860)
0a0ae16
* Remove HelperMethodFrames from Marshal APIs
Convert Marshal.GetHRForException and Marshal.GetExceptionForHRInternal to QCalls.
* Convert OffsetOfHelper to QCall
* Feedback
* Remove GC transition.
* Update src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs
* Update src/coreclr/vm/marshalnative.cpp
---------
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 11, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes
@davidwrightondavidwrightondavidwrighton approved these changes
@elinor-fungelinor-fungelinor-fung approved these changes

Assignees

@AaronRobinsonMSFTAaronRobinsonMSFT

Labels

area-VM-coreclr

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@AaronRobinsonMSFT@jkotas@davidwrighton@elinor-fung