-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add new optimization steps to make Mark step more effective #848
Conversation
f884c05
to
ec39b3c
Compare
{ | ||
protected override void Process () | ||
{ | ||
var files = Context.Substitutions; |
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.
Why did you decide to go with a separate list of files instead of extending the ResolveFromXmlStep to be able to parse the body
and value
xml attributes?
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 can see pros and cons to both approaches. Just curious what your thinking was.
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 is actually not that much in common and adding if everywhere would not make it readable
string value = GetAttribute (iterator.Current, "value"); | ||
if (value != "") { | ||
if (!TryConvertValue (value, method.ReturnType, out object res)) { | ||
Context.LogMessage (MessageImportance.Normal, $"Invalid value for '{signature}' stub"); |
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'd argue for throwing instead of logging a message. If we log a message & ignore the item then the user doesn't get the behavior they are expecting and then they have to waste time figuring out why whatever they intended to have stubbed wasn't stubbed. Best case scenario they notice the log message and fix it, worst case they waste a lot more time fumbling around. In either case you are wasting more of the users time than just throwing and telling them what mistake they made.
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 you are not a fan of that, then my other recommendations would be
-
Use
MessageImportance.High
instead. -
Put the LogMessge call into a protected virtual method so that we can choose to change the behavior if we want.
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.
Fixed by using MessageImportance.High
Annotations.SetAction (method, MethodAction.ConvertToStub); | ||
return; | ||
default: | ||
Context.LogMessage (MessageImportance.Normal, $"Unknown body modification '{action}' for '{signature}'"); |
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.
Whatever is decided above, same here.
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.
Ditto as above
il.Emit (OpCodes.Ldc_I4_0); | ||
il.Emit (OpCodes.Ret); | ||
return body; | ||
throw new NotImplementedException (method.ReturnType.MetadataType.ToString ()); |
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 think it would be nice to display the name of the method that was beings stubbed in the exception message.
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.
Changed
public static void Main() | ||
{ | ||
new RemoveBody (); | ||
new NestedType (5); |
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.
Indent looks messed up in a couple places in this file.
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.
Fixed
} | ||
} | ||
|
||
[Kept] |
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.
Indent
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.
Fixed
[ExpectBodyModified] | ||
static void TestMethod_1 () | ||
{ | ||
if (TestProperty_int () == 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.
Once we got into more complex body editing and leveraging [ExpectBodyModified]
I noticed it wasn't that hard to have bugs that resulted in valid IL but the modified IL was wrong. If you were really diligent with your test setup and calling methods and should be removed or kept you could prevent most bugs from slipping by. However, for added safety we added a series of attributes to the test framework to invoke the linked code.
We a few attributes that will load the input and output assemblies in an AppDomain and invoke methods and check their behavior.
[CheckInvokeMatchesOriginal]
[CheckInvoke(<expected result>)]
[CheckInvokeThrows]
And we have one other attribute that you can put on the test case type itself that will tell the test framework to run the input and output test.exe
and compare their output.
[CheckOutput]
If you would like me to bring these upstream let me know.
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 think peverify is able to catch such issue so no for now
24bfd40
to
70cc9ca
Compare
static XPathDocument GetSubstitutions (Stream substitutions) | ||
{ | ||
using (StreamReader sr = new StreamReader (substitutions)) { | ||
return new XPathDocument (new StringReader (sr.ReadToEnd ())); |
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.
Avoid loading the whole stream into a string, then making another reader on top. This should help with performance of the operation and is simpler
return new XPathDocument (new StringReader (sr.ReadToEnd ())); | |
return new XPathDocument (sr); |
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
Two new steps have been introduced BodySubstituterStep This step removes any conditional blocks when the condition or conditions are evaluated as constants. This step does not do any inlining. The conditional logic is kept but based on the known values only one branch is always taken. A simple example which can be detected by linker. ```c# class C { static void Main () { if (Result) Console.WriteLine (); // <- this call will be marked and removed } static bool Result () => false; } ``` RemoveUnreachableBlocksStep A new command-line option called `--substitutions` allow external customization of any methoda for assemblies which are linked. The syntax is same as for existing linker descriptor XML files but it add way to control body modifications. An example of XML file ```xml <linker> <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null"> <type fullname="Mono.Linker.Tests.Cases.Substitutions.StubBodyWithValue"> <method signature="System.String TestMethod_1()" body="stub" value="abcd"> </method> </type> </assembly> </linker> ``` The `value` attribute is optional and only required when the method stub should not fallback to default behaviour. Additional to `stub` value also `remove` value is supported to forcibly remove body of the method when the method is marked. This is useful when the conditional logic cannot be evaluated by linker and the method will be marked but never actually reached. Applicability Both steps can be combined to achieve the effect of externally customizing which conditional branches can be removed during the linking. I can illustrate that on IntPtr.Size property. With following substitution any code that has compiled in conditional logic for 64 bits handling by checking IntPtr.Size will be removed during linking. ```xml <linker> <assembly fullname="mscorlib"> <type fullname="System.IntPtr"> <method signature="System.Int32 get_Size()" body="stub" value="4"> </method> </type> </assembly> </linker> ``` Implements dotnet#752
Two new steps have been introduced
BodySubstituterStep
This step removes any conditional blocks when the condition or conditions
are evaluated as constants. This step does not do any inlining. The conditional
logic is kept but based on the known values only one branch is always taken.
A simple example which can be detected by linker.
RemoveUnreachableBlocksStep
A new command-line option called
--substitutions
allow external customization of anymethoda for assemblies which are linked. The syntax is same as for existing linker descriptor
XML files but it add way to control body modifications.
An example of XML file
The
value
attribute is optional and only required when the method stub should notfallback to default behaviour.
Additional to
stub
value alsoremove
value is supported to forcibly remove bodyof the method when the method is marked. This is useful when the conditional logic
cannot be evaluated by linker and the method will be marked but never actually reached.
Applicability
Both steps can be combined to achieve the effect of externally customizing which
conditional branches can be removed during the linking.
I can illustrate that on IntPtr.Size property. With following substitution any code
that has compiled in conditional logic for 64 bits handling by checking IntPtr.Size will
be removed during linking.