[linker] Add an well known candidate inliner substep along with tests #1513
Conversation
TL&DR: This is *how* it should be done and tested, it's not complete (single, simple case) nor the most interesting case ;-) The trick is to make sure each case is covered by tests so a mono _bump_ won't give us a BCL that does not conform to what the linker expect. What's the impact ? 1. There is the expected reduction of metadata in mscorlib. Since both methods don't call other API there's no indirect effect (removal). --- before 2017-01-15 11:12:44.000000000 -0500 +++ after 2017-01-15 11:12:56.000000000 -0500 @@ -13166,9 +13166,6 @@ System.Void System.Security.SecurityException::.ctor(System.Runtime.Serialization.SerializationInfo,System.Runtime.Serialization.StreamingContext) System.Void System.Security.SecurityException::.ctor(System.String) System.Void System.Security.SecurityException::GetObjectData(System.Runtime.Serialization.SerializationInfo,System.Runtime.Serialization.StreamingContext) -System.Boolean System.Security.SecurityManager::CheckElevatedPermissions() -System.Security.SecurityManager -System.Void System.Security.SecurityManager::EnsureElevatedPermissions() System.Security.SecurityRulesAttribute System.Security.SecurityRuleSet System.Security.SecurityRulesAttribute::m_ruleSet System.Void System.Security.SecurityRulesAttribute::.ctor(System.Security.SecurityRuleSet) 2. There is no visible size change (even with #1) in mscorlib.dll due to padding (compiler /filealign) mscorlib.dll 793,600 793,600 0 0.00 % 3. there's a *very* small reduction of mscorlib.*.aotdata size mscorlib.armv7.aotdata 717,264 717,216 -48 -0.01 % mscorlib.arm64.aotdata 712,840 712,704 -136 -0.02 % AOT data *.aotdata 6,460,064 6,459,880 -184 0.00 % 4. there's no change in executable size - normal as the AOT compiler has _likely_ already doing the same optimization (before this commit) Executable 29,270,272 29,270,272 0 0.00 % Full comparison: https://gist.github.com/spouliot/0464c8fa3a92b6486dfd90595d9eb718
|
Build failure |
I did not notice the xml files were duplicated across platforms.
|
Build success |
|
|
||
| /// <summary> | ||
| /// We look for candidates, without parameters, that only do `return true;`. | ||
| /// Such methods can be inlined by replacing the `call` with a `ldc.i4.1` instruction |
There was a problem hiding this comment.
Is this true for instance calls as well, or will replacing the call with a ldc.* leave the instance on the IL stack?
You might also have to consider tail calls (not a problem for the methods in this PR, but it might be a problem for a more general scenario).
There was a problem hiding this comment.
Good point, the comment should have been on the NoParameterReturnOnlyConstant method (one time) on not on the here, which would be duplicated for each assembly and might differ in future cases.
|
|
||
| /// <summary> | ||
| /// We look for candidates, without parameters and return value, that does nothing (only `ret`). | ||
| /// Such methods can be inlined by replacing the `call` with a nop` instruction. |
There was a problem hiding this comment.
Same comment wrt instance calls & tail calls.
There was a problem hiding this comment.
same (will move comment)
| var ins = b.Instructions [0]; | ||
| if (code == ins.OpCode.Code) { | ||
| var s = m.ToString (); | ||
| list.Remove (s); |
There was a problem hiding this comment.
This will fail if a new candidate not in the list shows up (good), but it will fail unhelpfully (you'll get a KeyNotFoundException). Maybe accumulate all the failures (so that you don't have to fix them one-by-one), and then show them at once in a single assert at the end, with a helpful assert message?
There was a problem hiding this comment.
HashSet.Remove returns bool, i.e. it does not throw. So ListMethods will show the list (not just the first instance) of the things that needs fixing.
There was a problem hiding this comment.
Ah, you're right about HashSet. However ListMethods will show if a method we already inline changes (if it's not inlineable anymore), but it will not show if a method becomes inlineable (either because it's new, or its implementation changed) - but I guess that wasn't your intention.
There was a problem hiding this comment.
No, I added the DisplayCandidates for this https://github.com/xamarin/xamarin-macios/pull/1513/files#diff-35eb92cb55aa583082344a291c8b5c86R25 - nut so far it seems only a few of them will have an real impact (many are naturally removed from most apps).
…the example given works for a static call
|
Build success |
TL&DR: This is how it should be done and tested, it's not complete
(single, simple case) nor the most interesting case ;-)
The trick is to make sure each case is covered by tests so a mono
bump won't give us a BCL that does not conform to what the linker
expect.
What's the impact ?
methods don't call other API there's no indirect effect (removal).
There is no visible size change (even with
make installdoes not work unlessmake allis already complete #1) in mscorlib.dll due topadding (compiler /filealign)
mscorlib.dll 793,600 793,600 0 0.00 %
there's a very small reduction of mscorlib.*.aotdata size
mscorlib.armv7.aotdata 717,264 717,216 -48 -0.01 %
mscorlib.arm64.aotdata 712,840 712,704 -136 -0.02 %
AOT data *.aotdata 6,460,064 6,459,880 -184 0.00 %
there's no change in executable size - normal as the AOT compiler has
likely already doing the same optimization (before this commit)
Executable 29,270,272 29,270,272 0 0.00 %
Full comparison: https://gist.github.com/spouliot/0464c8fa3a92b6486dfd90595d9eb718