Optimize calls to BlockLiteral.SetupBlock to inject the block signature. #3391

rolfbjarne · 2018-02-05T14:23:34Z

Optimize calls to BlockLiteral.SetupBlock[Unsafe] to calculate the block
signature at build time, and inject it into the call site.

This makes block invocations 10-15x faster (I've added tests that asserts at
least an 8x increase, along with numerous other tests for this optimization).

It's also required in order to be able to remove the dynamic registrar code in
the future (since calculating the block signature at runtime requires the
dynamic registrar).

…the code line causing the error/warning. Add support for reporting errors/warnings that point to the code line causing the error/warning by adding ErrorHelper overloads that take the exact instruction to report (previously we defaulted to the first line/instruction in a method).

…ages.

…k signature. Optimize calls to BlockLiteral.SetupBlock[Unsafe] to calculate the block signature at build time, and inject it into the call site. This makes block invocations 10-15x faster (I've added tests that asserts at least an 8x increase). It's also required in order to be able to remove the dynamic registrar code in the future (since calculating the block signature at runtime requires the dynamic registrar).

…n both XI's and XM's link all test.

…al.SetupBlock optimization.

…tion.

VincentDondain · 2018-02-05T16:25:28Z

docs/website/mmp-errors.md

@@ -252,6 +259,15 @@ See https://msdn.microsoft.com/en-us/library/x0w2664k.aspx for more information

 ### <a name="MM4134">MM4134: Your application is using the '{0}' framework, which isn't included in the MacOS SDK you're using to build your app (this framework was introduced in OSX {2}, while you're building with the MacOS {1} SDK.) This configuration is not supported with the static registrar (pass --registrar:dynamic as an additional mmp argument in your project's Mac Build option to select). Alternatively select a newer SDK in your app's Mac Build options.

+### <a name="MT4173"/>MT4173: The registrar can't compute the block signature for the delegate of type {delegate-type} in the method {method} because *.


We should figure out a way to not having to write duplicate documentation here 😒

@VincentDondain I've now fixed the docs according to your comments.

VincentDondain · 2018-02-05T16:33:39Z

docs/website/optimizations.md

+time, and modify the IL to call a SetupBlock variation that takes the
+signature as an argument instead, avoiding the need for calculating the
+signature at runtime.
+


Can we break this ^ down even more, the last sentence is hard to digest.

Maybe: "This might be a fairly expensive operation. This optimization will calculate the block signature at build time, and modify the IL to call a SetupBlock variation that takes the signature as an argument instead. Doing this avoid the need for calculating the signature at runtime."

Note: let's use ` for SetupBlock -> SetupBlock

VincentDondain · 2018-02-05T16:34:55Z

docs/website/optimizations.md

+signature as an argument instead, avoiding the need for calculating the
+signature at runtime.
+
+Benchmarks show that this speeds up calling a block 10-15x.


Missing "by"?

"Benchmarks show that this speeds up calling a block by 10-15x."

Or maybe "Benchmarks show that this speeds up calling a block by a factor of 10 to 15."

VincentDondain · 2018-02-05T16:36:14Z

docs/website/optimizations.md

+
+It is is enabled by default when using the static registrar (which is enabled
+by default when building for device (iOS) or building for release (macOS)).
+


maybe go for "(which is itself enabled by default ..." because it's a double enabled by default.

Also I'd use something else than parenthesis for (iOS) and (macOS), dunno if nested parenthesis is a thing. Maybe [iOS], [macOS]?

VincentDondain

Otherwise code looks good to me 👍

dalexsoto

I see you added [BindingImpl (BindingImplOptions.Optimizable)] to some manual bindings containing manual blocks code, when shouldn't this be added / used??

monojenkins · 2018-02-05T17:08:10Z

Build failure

rolfbjarne · 2018-02-05T17:41:10Z

I see you added [BindingImpl (BindingImplOptions.Optimizable)] to some manual bindings containing manual blocks code, when shouldn't this be added / used??

When there's nothing in the method that will be optimized 😄 Theoretically we could try to optimize everything, but it's time-consuming to process every method, so the attribute is really just a way to not waste time trying to optimize methods that can't be optimized (it also limits the potential for broken optimizations).

spouliot · 2018-02-05T15:32:23Z

docs/website/mmp-errors.md

+The linker reports this warning when it can't optimize a call to BlockLiteral.SetupBlock or Block.SetupBlockUnsafe.
+
+The message will point to the method that calls BlockLiteral.SetupBlock[Unsafe], and
+it may also give clues as to why the call couldn't be optimized.


what would the developer action be ?

The developer can probably not do much more than report a bug and ignore the warning (so I've added that to the help just below).

spouliot · 2018-02-05T15:33:09Z

docs/website/mmp-errors.md

+the registrar couldn't compute it.
+
+This means that the block signature has to be computed at runtime, which is
+somewhat slower.


same. Should we ask them to file an issue so we can investigate why ?

Done: I've explained a bit below when this can happen, and what to do (for the developer).

spouliot · 2018-02-05T15:33:19Z

docs/website/mtouch-errors.md

+The linker reports this warning when it can't optimize a call to BlockLiteral.SetupBlock or Block.SetupBlockUnsafe.
+
+The message will point to the method that calls BlockLiteral.SetupBlock[Unsafe], and
+it may also give clues as to why the call couldn't be optimized.


spouliot · 2018-02-05T15:33:29Z

docs/website/mtouch-errors.md

+the registrar couldn't compute it.
+
+This means that the block signature has to be computed at runtime, which is
+somewhat slower.


spouliot · 2018-02-05T19:03:42Z

src/AddressBook/ABAddressBook.cs

@@ -207,6 +207,7 @@ public static ABAuthorizationStatus GetAuthorizationStatus ()
 		extern unsafe static void ABAddressBookRequestAccessWithCompletion (IntPtr addressbook, void * completion);

 		[iOS (6,0)]
+		[BindingImpl (BindingImplOptions.Optimizable)]


I wonder if we can add an intro tests to detect them, e.g. based on any locals with a BlockLiteral type ?

I already added such a test, but as an mtouch/mmp test (because intro would run on device as well, and that seemed wasteful for something that's clearly not needed): https://github.com/xamarin/xamarin-macios/pull/3391/files#diff-f773c4aa1cac370f711a23af6d86eb2aR29

that's weird... GitHub shows me New changes since you last viewed that are way older than what I reviewed ?!?
anyhow I see them now and it's even better the way you implemented them!

chamons

👍 (assuming other issues noted above are obviously fixed).

Outside my area of expertise but ❤️ the tests that give me a greater clue.

chamons · 2018-02-05T20:07:10Z

tests/common/BundlerTest.cs

+	{
+		[Test]
+#if __MACOS__
+		[TestCase (Profile.macOSMobile)]


Do we need/want Full here?

I don't think so, because this is not testing any API/code inside the platform assembly, only mmp (the linker), which should not depend on the profile (note that for XI I'm not running the test for tvOS/watchOS, only iOS).

That's fair.

chamons · 2018-02-05T20:07:30Z

tests/common/BundlerTest.cs

+
+		[Test]
+#if __MACOS__
+		[TestCase (Profile.macOSMobile)]


…se class aren't executed twice.

The optimization tests only apply when the test assembly is linked, and that only happens in linkall, so exclude those tests in linksdk.

Fixes these test failures: 1) Failed : Xamarin.MMP.Tests.MMPTests.MM0132("inline-runtime-arch") The warning 'MM0132: Unknown optimization: 'inline-runtime-arch'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size.' was not found in the output: Message #1 did not match: actual: 'Unknown optimization: 'inline-runtime-arch'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size, blockliteral-setupblock.' expected: 'Unknown optimization: 'inline-runtime-arch'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size.' Message #2 did not match: actual: 'Unknown optimization: 'inline-runtime-arch'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size, blockliteral-setupblock.' expected: 'Unknown optimization: 'inline-runtime-arch'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size.' 2) Failed : Xamarin.MMP.Tests.MMPTests.MM0132("foo") The warning 'MM0132: Unknown optimization: 'foo'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size.' was not found in the output: Message #1 did not match: actual: 'Unknown optimization: 'foo'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size, blockliteral-setupblock.' expected: 'Unknown optimization: 'foo'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size.' Message #2 did not match: actual: 'Unknown optimization: 'foo'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size, blockliteral-setupblock.' expected: 'Unknown optimization: 'foo'. Valid optimizations are: remove-uithread-checks, dead-code-elimination, inline-isdirectbinding, inline-intptr-size.'

Fixes this test failure: 1) SetupBlock_CustomDelegate (Linker.Shared.BaseOptimizeGeneratedCodeTest.SetupBlock_CustomDelegate) Counter Expected: 1 But was: 2

…better) behavior. Fixes this test failure: 1) Failed : Xamarin.Registrar.GenericType_WithInvalidParameterTypes The error 'MT4136: The registrar cannot marshal the parameter type 'System.Collections.Generic.List`1' of the parameter 'arg' in the method 'Open`1.Bar(System.Collections.Generic.List`1)'' was not found in the output: Message #1 did not match: actual: 'The registrar cannot marshal the parameter type 'System.Collections.Generic.List`1<Foundation.NSObject>' of the parameter 'arg' in the method 'Open`1.Bar(System.Collections.Generic.List`1)'' expected: 'The registrar cannot marshal the parameter type 'System.Collections.Generic.List`1' of the parameter 'arg' in the method 'Open`1.Bar(System.Collections.Generic.List`1)''

monojenkins · 2018-02-06T01:13:43Z

Build failure

dalexsoto

Thanks Rolf! ❤️

rolfbjarne · 2018-02-06T06:07:36Z

Test failures are unrelated:

MSBuild/iOS: https://github.com/xamarin/maccore/issues/592
mscorlib/Mac/Full: https://github.com/xamarin/maccore/issues/628
mscorlib/iOS 32-bit: https://github.com/xamarin/maccore/issues/629

rolfbjarne added 7 commits February 5, 2018 15:14

[tests] Add support for asserting filename/linenumber in warning mess…

d0f16c6

…ages.

[tests] Create a BaseOptimizeGeneratedCodeTest test that's included i…

056cb5c

…n both XI's and XM's link all test.

[tests] Add link all test (for both XI and XM) to test the BlockLiter…

23ad930

…al.SetupBlock optimization.

[tests] Add mtouch/mmp tests for the BlockLiteral.SetupBlock optimiza…

5fb1855

…tion.

Make all methods that manually create BlockLiterals optimizable.

c260484

rolfbjarne requested review from chamons, dalexsoto, mandel-macaque, spouliot and VincentDondain as code owners February 5, 2018 14:23

VincentDondain requested changes Feb 5, 2018

View reviewed changes

VincentDondain reviewed Feb 5, 2018

View reviewed changes

dalexsoto reviewed Feb 5, 2018

View reviewed changes

spouliot suggested changes Feb 5, 2018

View reviewed changes

chamons approved these changes Feb 5, 2018

View reviewed changes

rolfbjarne added 8 commits February 5, 2018 22:35

[tests][linker] Make the base test class abstract, so tests in the ba…

026570b

…se class aren't executed twice.

[tests][linker] Don't execute linkall-only tests in linksdk.

cc74ef9

The optimization tests only apply when the test assembly is linked, and that only happens in linkall, so exclude those tests in linksdk.

[tests][linker] Fix typo.

34113a0

Fixes this test failure: 1) SetupBlock_CustomDelegate (Linker.Shared.BaseOptimizeGeneratedCodeTest.SetupBlock_CustomDelegate) Counter Expected: 1 But was: 2

[docs] mmp shows MM errors/warnings.

44601e7

[docs] Improve according to reviews.

b728169

[tests] Fix merge failure causing test duplication.

f6c38bd

spouliot approved these changes Feb 5, 2018

View reviewed changes

VincentDondain approved these changes Feb 5, 2018

View reviewed changes

dalexsoto approved these changes Feb 6, 2018

View reviewed changes

rolfbjarne merged commit 97230c2 into xamarin:master Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize calls to BlockLiteral.SetupBlock to inject the block signature. #3391

Optimize calls to BlockLiteral.SetupBlock to inject the block signature. #3391

rolfbjarne commented Feb 5, 2018

VincentDondain Feb 5, 2018

rolfbjarne Feb 5, 2018

rolfbjarne Feb 5, 2018

VincentDondain Feb 5, 2018 •

edited

Loading

VincentDondain Feb 5, 2018

VincentDondain Feb 5, 2018

VincentDondain Feb 5, 2018

VincentDondain Feb 5, 2018

VincentDondain left a comment

dalexsoto left a comment •

edited

Loading

monojenkins commented Feb 5, 2018

rolfbjarne commented Feb 5, 2018 •

edited

Loading

spouliot Feb 5, 2018

rolfbjarne Feb 5, 2018

spouliot Feb 5, 2018

rolfbjarne Feb 5, 2018

spouliot Feb 5, 2018

spouliot Feb 5, 2018

spouliot Feb 5, 2018

rolfbjarne Feb 5, 2018

spouliot Feb 5, 2018

chamons left a comment •

edited

Loading

chamons Feb 5, 2018

rolfbjarne Feb 5, 2018

chamons Feb 5, 2018

chamons Feb 5, 2018

rolfbjarne Feb 5, 2018

monojenkins commented Feb 6, 2018

dalexsoto left a comment

rolfbjarne commented Feb 6, 2018

		@@ -252,6 +259,15 @@ See https://msdn.microsoft.com/en-us/library/x0w2664k.aspx for more information

		### <a name="MM4134">MM4134: Your application is using the '{0}' framework, which isn't included in the MacOS SDK you're using to build your app (this framework was introduced in OSX {2}, while you're building with the MacOS {1} SDK.) This configuration is not supported with the static registrar (pass --registrar:dynamic as an additional mmp argument in your project's Mac Build option to select). Alternatively select a newer SDK in your app's Mac Build options.

		### <a name="MT4173"/>MT4173: The registrar can't compute the block signature for the delegate of type {delegate-type} in the method {method} because *.


		It is is enabled by default when using the static registrar (which is enabled
		by default when building for device (iOS) or building for release (macOS)).

Optimize calls to BlockLiteral.SetupBlock to inject the block signature. #3391

Optimize calls to BlockLiteral.SetupBlock to inject the block signature. #3391

Conversation

rolfbjarne commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentDondain Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentDondain left a comment

Choose a reason for hiding this comment

dalexsoto left a comment • edited Loading

Choose a reason for hiding this comment

monojenkins commented Feb 5, 2018

rolfbjarne commented Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chamons left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Feb 6, 2018

dalexsoto left a comment

Choose a reason for hiding this comment

rolfbjarne commented Feb 6, 2018

VincentDondain Feb 5, 2018 •

edited

Loading

dalexsoto left a comment •

edited

Loading

rolfbjarne commented Feb 5, 2018 •

edited

Loading

chamons left a comment •

edited

Loading