Skip to content
This repository has been archived by the owner on Apr 2, 2022. It is now read-only.

Rethink View DSL structure to enable scaling and improve tree-shaking #52

Merged
merged 3 commits into from
Jan 21, 2022

Conversation

TimLariviere
Copy link
Owner

@TimLariviere TimLariviere commented Jan 17, 2022

Supersedes #42 and #47

Like @edgarfgp suggested in #12, Xamarin.Forms is a "done" API, meaning it won't add breaking changes.
All the focus from Microsoft is now on MAUI.

This means putting effort on CodeGen is not necessarily worth it right now, and we are better off writing all wrappers by hand (it is very time consuming, but much easier than porting CodeGen from v1).

Also, I will continue on my efforts started in #47 about tree shaking, ideally with no allocation more.

Benefits of this PR:

  • Improved readability for the supported wrapped controls (1 file per control)
  • Removed weird naming conflict because of interface-typing in extension methods (padding/paddingLayout => all are now padding)

Typical structure for a wrapped control

namespace Fabulous.XamarinForms

 open System.Runtime.CompilerServices
 open Fabulous
 open Xamarin.Forms

 type IButton =
     inherit IView

 module Button =
     let WidgetKey = Widgets.register<Button> ()

     let Text =
         Attributes.defineBindable<string> Button.TextProperty

     let Clicked =
         Attributes.defineEventNoArg "Button_Clicked" (fun target -> (target :?> Button).Clicked)

     let TextColor =
         Attributes.defineAppThemeBindable<Color> Button.TextColorProperty

     let FontSize =
         Attributes.defineBindable<double> Button.FontSizeProperty

 [<AutoOpen>]
 module ButtonBuilders =
     type Fabulous.XamarinForms.View with
         static member inline Button<'msg>(text: string, onClicked: 'msg) =
             WidgetBuilder<'msg, IButton>(
                 Button.WidgetKey,
                 Button.Text.WithValue(text),
                 Button.Clicked.WithValue(onClicked)
             )

 [<Extension>]
 type ButtonModifiers =
     [<Extension>]
     static member inline textColor(this: WidgetBuilder<'msg, #IButton>, light: Color, ?dark: Color) =
         this.AddScalar(
             Button.TextColor.WithValue(
                 { Light = light
                   Dark =
                       match dark with
                       | None -> ValueNone
                       | Some v -> ValueSome v }
             )
         )

     [<Extension>]
     static member inline font(this: WidgetBuilder<'msg, #IButton>, value: double) =
         this.AddScalar(Button.FontSize.WithValue(value))

     [<Extension>]
     static member inline font(this: WidgetBuilder<'msg, #IButton>, value: NamedSize) =
         this.AddScalar(Button.FontSize.WithValue(Device.GetNamedSize(value, typeof<Button>)))

@twop
Copy link
Collaborator

twop commented Jan 17, 2022

I agree, I think writing by hand makes the most sense given the circumstances.

@edgarfgp
Copy link
Contributor

edgarfgp commented Jan 18, 2022

This looks fantastic. . Adds some missing control so we I continue migrating my company app to v2 . @TimLariviere Couple questions:

  1. Can we have all the Color properties bee theme aware ie for Layouts, Controls , Shapes etc ?
  2. Same but for Images from my experience sometime you need to provide a different image for dark mode ?
  3. With the recents changes is Implement Style widgets #10 is still blocked . ?
  4. Once this PR is merged . Would you accept PR to add the missing controls following the new structure ?

@TimLariviere
Copy link
Owner Author

TimLariviere commented Jan 18, 2022

Can we have all the Color properties bee theme aware ie for Layouts, Controls , Shapes etc ?

Yes, the plan is to think of all the properties (Colors and others) that make sense to be theme aware and change them to AppThemeBindable.

Same but for Images from my experience sometime you need to provide a different image for dark mode ?

See my answer above.
For this, maybe we should keep Image to be not theme aware and have a ThemeAwareImage that maps to AppThemeBindable properties.

With the recents changes is #10 is still blocked . ?

Yes, still blocked.
Xamarin.Forms.Style being sealed prevents us from doing anything, unfortunately.

Once this PR is merged . Would you accept PR to add the missing controls following the new structure ?

Yes, absolutely!
Fabulous v2 is advanced enough to start adding missing controls.
I think I'll open an issue with the list of all missing controls. There we will be able to discuss about 1 and 2 as well

@TimLariviere
Copy link
Owner Author

TimLariviere commented Jan 18, 2022

Interestingly the linker is capable of tree-shaking whole files, even though we have side effects in WidgetDef creation. But unused AttributeDefinitions in the same file than used ones are not removed.

So this change of structure enabled removing files whose controls are not used, even if basically nothing changed.

@TimLariviere
Copy link
Owner Author

TimLariviere commented Jan 18, 2022

Also confirmed that having a pure unused let value in a module will get removed if unused.
I'll try to make AttributeDefinition not rely on AttributeDefinitionStore to make it pure

@twop
Copy link
Collaborator

twop commented Jan 18, 2022

Also confirmed that having a pure unused let value in a module will get removed if unused.
I'll try to make AttributeDefinition not rely on AttributeDefinitionStore to make it pure

@TimLariviere I was thinking about this problem, I think it will be possible to store a direct reference to attribute definition in ScalarAttribute (the same applies for Widget as well).

But there are a couple of thoughts/considerations.

  1. How to sort Scalars, it Is possible that we can use GetHashCode() for definitions, but we should probably make it statically computed, otherwise it is a cache miss + dynamic dispatch for sorting.
  2. We (I?) want to introduce the notion of flags, probably both for Attributes and Widgets.

So if you have a good feeling how it might all work together then I suggest to "do it", otherwise I may propose to swap tasks: I will do "pure" attribute definitions (I thought about it + it connects to flags optimization I'm tinkering about) you will do "key" property? What are you thoughts?

@TimLariviere
Copy link
Owner Author

I want to take a stab at pure AttributeDefinition to see if we can get fully linked assemblies, and still having a working app.

I think it will be possible to store a direct reference to attribute definition in ScalarAttribute (the same applies for Widget as well).

Yes, it's what I was thinking too.
For the flag optimization, I won't do it in that PR. So you'll be able to work on it after.

A couple of questions:

  • Am I right thinking a direct reference will take 64 bits instead of the 32 bits int?
  • Is that a potential issue for cache lines?

I think with pure attribute definitions, we can strongly type those in WidgetAttribute and WidgetCollectionAttribute.

@twop
Copy link
Collaborator

twop commented Jan 19, 2022

Perfect!

  1. Am I right thinking a direct reference will take 64 bits instead of the 32 bits int?
  2. Is that a potential issue for cache lines?
  1. As far as I know it is 8 bytes (64 bits) for all relevant platforms
  2. Unlikely. It is already 12 bytes. So replacing key with a pointer will make it 16 bytes (which is still ok), but adding more data might present perf degradation. My hope is that it won't be noticeable. As I mentioned before even before flagging we probably want a stable numeric "key" to sort against (hash can be that).

@twop
Copy link
Collaborator

twop commented Jan 19, 2022

One of the easier ways to check cache line misses is to introduce 2 floats (8 + 8 bytes) to Scalars (initialized with 0.0). And run benchmarks. I think you will notice the difference both in terms of perf and memory consumptions but it is unlikely going to be huge. I think anything below 10-20% for memory and time is acceptable. We will find other ways to save on memory.

The same experiment is applicable to Widget, though the theory is that we have fewer widgets than scalars, thus we probably have bigger affordance there

@TimLariviere
Copy link
Owner Author

Merging this since it's working and already improves tree-shaking.
Will work on pure attribute definitions in another PR

@TimLariviere TimLariviere merged commit 19c35f6 into main Jan 21, 2022
@TimLariviere TimLariviere deleted the view-dsl branch January 21, 2022 08:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants