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] Refactored code into marshal-ilgen and marshal-shared. #68656

Merged
merged 8 commits into from
May 1, 2022

Conversation

naricc
Copy link
Contributor

@naricc naricc commented Apr 28, 2022

This PR refactors the marshaling code into ilgen and shared parts. This is one step in a change that will let move the legacy marshaling code into to a component; marshal-shared has the parts that will be shared by both legacy and ilgen. The next step is to factor out the legacy marshaling code, and then move it into a component.

Once componentization is complete, the runtime size will be reduced by approximately 100 KB (on x64) when the legacy component is not needed.

contributes to: #61685

@ghost ghost assigned naricc Apr 28, 2022
@naricc naricc force-pushed the naricc/runtime-marshal-shared-refactor branch from 10e305f to ec6c0b2 Compare April 28, 2022 21:09
@naricc naricc force-pushed the naricc/runtime-marshal-shared-refactor branch from ec6c0b2 to 5715438 Compare April 28, 2022 21:10
@naricc naricc changed the title [DRAFT][MONO] Refactored code into marshal-ilgen and marshal-shared. [MONO] Refactored code into marshal-ilgen and marshal-shared. Apr 28, 2022
@naricc naricc marked this pull request as ready for review April 28, 2022 21:14
@naricc
Copy link
Contributor Author

naricc commented Apr 28, 2022

@lambdageek I believe this is ready for review.

@vargaz
Copy link
Contributor

vargaz commented Apr 28, 2022

In general, marshal-shared.c still contains a lot of code, are you sure those are all going to be needed with the new style of marshaling ?

@naricc
Copy link
Contributor Author

naricc commented Apr 29, 2022

@vargaz

In general, marshal-shared.c still contains a lot of code, are you sure those are all going to be needed with the new style of marshaling ?

This seems to be the most I can do statically. All of these functions are referenced by something that needs to stay in the runtime. But it is possible that some of them cannot actually be called at runtime in the new mode, or that the functions could be broken into pieces and parts of them put into the legacy component.

Basically what I found that could be moved into legacy is these functions:

cb.emit_marshal_boolean = emit_marshal_boolean_ilgen;
cb.emit_marshal_char = emit_marshal_char_ilgen;
cb.emit_marshal_custom = emit_marshal_custom_ilgen;
cb.emit_marshal_asany = emit_marshal_asany_ilgen;
cb.emit_marshal_vtype = emit_marshal_vtype_ilgen;
cb.emit_marshal_string = emit_marshal_string_ilgen;
cb.emit_marshal_safehandle = emit_marshal_safehandle_ilgen;
cb.emit_marshal_handleref = emit_marshal_handleref_ilgen;
cb.emit_marshal_object = emit_marshal_object_ilgen;
cb.emit_marshal_variant = emit_marshal_variant_ilgen;
, plus emit_marshal_ptr_ilgen, emit_marshal_array_ilgen, and static methods only reachable from those.

This represents a clear size savings and is easy to implement. We plan to look at ways to squeeze even more out later, but its somewhat more complicated and I don't know how much it will save.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

Looking good, just need to address Zoltan's suggestions

src/mono/mono/metadata/marshal-shared.c Show resolved Hide resolved
src/mono/mono/metadata/marshal-shared.h Outdated Show resolved Hide resolved
@naricc
Copy link
Contributor Author

naricc commented Apr 29, 2022

@lambdageek @vargaz I've addressed all the suggestions/issues.

Comment on lines -677 to -678
static MonoJitICallId
conv_to_icall (MonoMarshalConv conv, int *ind_store_type)
Copy link
Member

Choose a reason for hiding this comment

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

i'm surprised this needs to be shared.

in the lightweight marshaler this should never get called (again because conv should never be anything other than NONE)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, taking that into account I can probably remove it. But can I actually make that change in this PR? It seems like I need to seperate out the lightweight and ilgen first.

Copy link
Member

Choose a reason for hiding this comment

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

I think this will probably have to wait until we have the lightweight marshaling code separated out. right now I think conv_to_icall is getting called from both marshal-ilgen and marshal-shared, so it would be difficult to just have a single copy. after the split, it will hopefully be easier to refactor the code so that -ilgen and -lightweight each get their own copy and the -ligthweight one is trivial.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

i'm a little surprised that any code that deals with marshaling conversions or anything like that needs to be shared. it seems like that stuff is only for the legacy marshaler not the lightweight marshaler.

@lambdageek
Copy link
Member

I think once we have the lightweight marshaler separated out, we can delete a lot of code by un-sharing some of this stuff and implementing the lightweight marshaling versions as g_assert_not_reached() or no-ops or single simple cases.

@naricc naricc merged commit 9299bc4 into dotnet:main May 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 31, 2022
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.

3 participants