TimLariviere / Fabulous-new

Fabulous v2 - Work in progress
https://timothelariviere.com/Fabulous-new/
Other
41 stars 3 forks source link

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

Closed TimLariviere closed 2 years ago

TimLariviere commented 2 years ago

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:

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 commented 2 years ago

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

edgarfgp commented 2 years ago

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 #10 is still blocked . ?
  4. Once this PR is merged . Would you accept PR to add the missing controls following the new structure ?
TimLariviere commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

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

twop commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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