-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Partially allow marshalling ILSTUB compilation for ndirect methods using crossgen2 if SPC.dll is in the same bubble #54847
Conversation
…ing crossgen2 if SPC.dll is in the same bubble AnsiStringMarshaller is disallowed because it incorrectly handles pointers returned from native code.
if (((EcmaMethod)method).Module.Equals(method.Context.SystemModule)) | ||
return true; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (((EcmaMethod)method).Module.Equals(method.Context.SystemModule)) | |
return true; |
This condition is redundant
for (int i = 0; i < marshallers.Length; i++) | ||
{ | ||
if (marshallers[i].GetType() == typeof(NotSupportedMarshaller) | ||
// TODO: AnsiStringMarshaller can be allowed when it's logic is fixed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to fix this.
@@ -361,6 +359,9 @@ public sealed override bool GeneratesPInvoke(MethodDesc method) | |||
if (((EcmaMethod)method).Module.Equals(method.Context.SystemModule)) | |||
return true; | |||
|
|||
if (_versionBubbleModuleSet.Contains(method.Context.SystemModule)) | |||
return !Marshaller.IsMarshallingNotSupported(method); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we have an existing mechanism that rejects unsupported IL stubs already. Is this one redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So enabling stub generation for broader scenarios uncovered other issues. So plan is to merge the fix for BStr issue, and remove other Marshallers not implemented for R2R as a separate change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mangod9 Ok, I'll update my PR when other marshallers are removed. This marshallers removal will be included in 6.0, right? Do you have milestone for correct marshallers inplementation for crossgen2? Will it be 6.0 too or 7.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah hoping removal will be done in the next week. Correct implementation for cg2 most likely will be in 7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbalykov the UTF8 and Unicode marshallers have been fixed. We will evaluate how to properly generate stubs for these cases in 7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll update this PR then
@gbalykov are you still planning to update this PR, else we could close it for now. |
@mangod9 I didn't have time to update this, you can close it for now, I'll get back to this a bit later |
ok, closing for now. |
AnsiStringMarshaller is disallowed because it incorrectly handles pointers returned from native code (related issue: #54820).
This change reduces number of ilstubs compiled during startup on ~40-50 on tizen xamarin apps. Startup time is improved on ~1.5%.
cc @alpencolt @jkotas