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

[XamlC] compile ThicknessTypeConverter #603

Merged
merged 1 commit into from Dec 12, 2016

Conversation

Projects
None yet
4 participants
@StephaneDelcroix
Member

StephaneDelcroix commented Dec 4, 2016

Description of Change

[XamlC] compile ThicknessTypeConverter

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@activa

This comment has been minimized.

Show comment
Hide comment
@activa

activa Dec 5, 2016

Contributor

May I ask why you are consistently ignoring coding style requirements for Xamarin.Forms? I've commented about this on 2 of your other commits but you ignored that entirely and merged the code anyway.

Don't you think it's important to have a consistent coding style across all code?

Contributor

activa commented Dec 5, 2016

May I ask why you are consistently ignoring coding style requirements for Xamarin.Forms? I've commented about this on 2 of your other commits but you ignored that entirely and merged the code anyway.

Don't you think it's important to have a consistent coding style across all code?

@StephaneDelcroix StephaneDelcroix requested a review from hartez Dec 9, 2016

@StephaneDelcroix

@hartez I've left some comments to help you review this code. I'll rebase after approved review

@@ -3,6 +3,7 @@
namespace Xamarin.Forms
{
[Xaml.ProvideCompiled("Xamarin.Forms.Core.XamlC.ThicknessTypeConverter")]

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member

it references the class by name, so the Core doesn't hold a reference on the BuildTask assembly

@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member

it references the class by name, so the Core doesn't hold a reference on the BuildTask assembly

return GenerateIL(module, l, t, r, b);
break;
}
}

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member

this is very similar to the runtime ThicknessTypeConverter so far

@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member

this is very similar to the runtime ThicknessTypeConverter so far

yield return Instruction.Create(OpCodes.Ldc_R8, d);
var thicknessCtor = module.Import(typeof(Thickness)).Resolve().Methods.FirstOrDefault(md => md.IsConstructor && md.Parameters.Count == args.Length);
var thicknessCtorRef = module.Import(thicknessCtor);
yield return Instruction.Create(OpCodes.Newobj, thicknessCtorRef);

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member

this produces the following IL

IL_00b9:  ldc.r8 0.
IL_00c2:  ldc.r8 20.
IL_00cb:  ldc.r8 0.
IL_00d4:  ldc.r8 20.
IL_00dd:  newobj instance void valuetype [Xamarin.Forms.Core]Xamarin.Forms.Thickness::'.ctor'(float64, float64, float64, float64)

equivalent to this c# code

new Thickness(0d,20d,0d,20);

a sharp mind would notice that using ldc.r8 for loading 0, or 20, takes 9B, and that's way too much. like if those bytes were for free ! when this IL is equally as good

ldc.i4.0
conv.r8

and takes only 2Bytes.

This is a nice observation, but that would make this code very hard to understand. That's why we do this kind of optimization as a post-generation step, here for i8 https://github.com/xamarin/Xamarin.Forms/pull/611/files#diff-6b2666065ac83ca89999db8a704b717eR247, and I should write a conversion for r8 as well, and submit it upstream.

[UPDATE] see http://stackoverflow.com/questions/41058165/why-does-the-compiler-optimize-ldc-i8-and-not-ldc-r8

[UPDATE] not worth the trouble

@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member

this produces the following IL

IL_00b9:  ldc.r8 0.
IL_00c2:  ldc.r8 20.
IL_00cb:  ldc.r8 0.
IL_00d4:  ldc.r8 20.
IL_00dd:  newobj instance void valuetype [Xamarin.Forms.Core]Xamarin.Forms.Thickness::'.ctor'(float64, float64, float64, float64)

equivalent to this c# code

new Thickness(0d,20d,0d,20);

a sharp mind would notice that using ldc.r8 for loading 0, or 20, takes 9B, and that's way too much. like if those bytes were for free ! when this IL is equally as good

ldc.i4.0
conv.r8

and takes only 2Bytes.

This is a nice observation, but that would make this code very hard to understand. That's why we do this kind of optimization as a post-generation step, here for i8 https://github.com/xamarin/Xamarin.Forms/pull/611/files#diff-6b2666065ac83ca89999db8a704b717eR247, and I should write a conversion for r8 as well, and submit it upstream.

[UPDATE] see http://stackoverflow.com/questions/41058165/why-does-the-compiler-optimize-ldc-i8-and-not-ldc-r8

[UPDATE] not worth the trouble

@StephaneDelcroix StephaneDelcroix merged commit f0ea0fa into master Dec 12, 2016

2 of 6 checks passed

Android-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1
Details
iOS10-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10
Details
iOS8-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8
Details
iOS9-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3681, ignored: 10
Details

@StephaneDelcroix StephaneDelcroix deleted the compiledThickenssConverter branch Dec 12, 2016

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment