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

[mono] Add DISABLE_NONBLITTABLE for non-blittable marshaling #46444

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

CoffeeFlux
Copy link
Contributor

This flag is not set by default on any platforms, but long-term it should be useful on wasm with the pinvoke generator work being done by the interop team.

This flag reduces around 13k in dotnet.wasm as of today (12/29/2020). However, that is a fairly low-end measurement because this PR only focused on the pinvoke marshaling rather than the full functionality. The structure of our marshaling callbacks means that even with icall linking, a lot of marshaling code will be kept around and unused, including all the string marshaling.

To solve this, we can either give up on the ilgen functionality being in a separate library so it gets linked our normally or we can just have this flag cover marshaling functionality as a whole. I've split that part off into a followup PR since it's more likely to be contested, but I implemented the latter option (and also disable things like certain JIT icalls for the same reason). We also will want analyzers set up before this is useful, so hopefully this solution is fine in combination with an extra analyzer for the marshaling methods in question? We also need to finish converting the libraries over to function pointers where appropriate (see 839b134 for an example).

@ghost
Copy link

ghost commented Dec 29, 2020

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

This flag is not set by default on any platforms, but long-term it should be useful on wasm with the pinvoke generator work being done by the interop team.

This flag reduces around 13k in dotnet.wasm as of today (12/29/2020). However, that is a fairly low-end measurement because this PR only focused on the pinvoke marshaling rather than the full functionality. The structure of our marshaling callbacks means that even with icall linking, a lot of marshaling code will be kept around and unused, including all the string marshaling.

To solve this, we can either give up on the ilgen functionality being in a separate library so it gets linked our normally or we can just have this flag cover marshaling functionality as a whole. I've split that part off into a followup PR since it's more likely to be contested, but I implemented the latter option (and also disable things like certain JIT icalls for the same reason). We also will want analyzers set up before this is useful, so hopefully this solution is fine in combination with an extra analyzer for the marshaling methods in question? We also need to finish converting the libraries over to function pointers where appropriate (see 839b134 for an example).

Author: CoffeeFlux
Assignees: -
Labels:

area-AssemblyLoader-mono

Milestone: -

@ghost
Copy link

ghost commented Dec 29, 2020

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

This flag is not set by default on any platforms, but long-term it should be useful on wasm with the pinvoke generator work being done by the interop team.

This flag reduces around 13k in dotnet.wasm as of today (12/29/2020). However, that is a fairly low-end measurement because this PR only focused on the pinvoke marshaling rather than the full functionality. The structure of our marshaling callbacks means that even with icall linking, a lot of marshaling code will be kept around and unused, including all the string marshaling.

To solve this, we can either give up on the ilgen functionality being in a separate library so it gets linked our normally or we can just have this flag cover marshaling functionality as a whole. I've split that part off into a followup PR since it's more likely to be contested, but I implemented the latter option (and also disable things like certain JIT icalls for the same reason). We also will want analyzers set up before this is useful, so hopefully this solution is fine in combination with an extra analyzer for the marshaling methods in question? We also need to finish converting the libraries over to function pointers where appropriate (see 839b134 for an example).

Author: CoffeeFlux
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@lambdageek
Copy link
Member

nit: the #define is called DISABLE_NONBLITTABLE, but the commit message and PR description says DISABLE_BLITTABLE

src/mono/mono/metadata/marshal-ilgen.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/marshal-ilgen.c Outdated Show resolved Hide resolved
@marek-safar
Copy link
Contributor

We also will want analyzers set up before this is useful,

Do we have an issue tracking/requesting this feature?

@CoffeeFlux
Copy link
Contributor Author

I think some of them are a bullet point on the pinvoke generator issue, but I need to go through and open separate issues about the ones we need.

@CoffeeFlux CoffeeFlux changed the title [mono] Add DISABLE_BLITTABLE for non-blittable marshaling [mono] Add DISABLE_NONBLITTABLE for non-blittable marshaling Jan 8, 2021
This flag is not set by default on any platforms, but long-term it should be useful on wasm with the pinvoke generator work being done by the interop team.

This flag reduces around 13k in dotnet.wasm as of today (12/29/2020). However, that is a fairly low-end measurement because this PR only focused on the pinvoke marshaling rather than the full functionality. The structure of our marshaling callbacks means that even with icall linking, a lot of marshaling code will be kept around and unused, including all the string marshaling. 

To solve this, we can either give up on the ilgen functionality being in a separate library so it gets linked our normally or we can just have this flag cover marshaling functionality as a whole. I've split that part off into a followup PR since it's more likely to be contested, but I implemented the latter option (and also disable things like certain JIT icalls for the same reason). We also will want analyzers set up before this is useful, so hopefully this solution is fine in combination with an extra analyzer for the marshaling methods in question?
@CoffeeFlux CoffeeFlux merged commit 55211d2 into dotnet:master Jan 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants