Ref uniform edge case

[alexswan10k]

Just a small one, couldn't help myself. The uniform parameters thing, I didn't believe you, so suffer and learn the hard way what you were talking about haha. As for the problem, it is quite clear to me now - when you share generic parameters for the params AND the return type, it gets super confused because it infers if it is a ref or not from the generic.

I have added a trivial example of a ( T -> T) passthrough function, which broke everything before. I have tweaked the shouldBePassByRefForParam so this now correctly dereferences etc. Also to clarify, we also do not want to be cloning when borrowing because it returns a value type, so I have tweaked this also.

It sucks that everything has to be uniform, but I cannot see any way around it either. Will have a good think.

[ncave]

It sucks that everything has to be uniform

I think it's fine, for large value types it's actually an optimization to pass them by (immutable) ref.
I was wondering few days ago if we should be also be using cows to minimize the cloning.

[alexswan10k]

That is indeed interesting. I feel a bit out of my depth with this one, going to read up a bit. A prelim scan though pointed me to the fact that Rc has an as_mut, which basically does the same thing. Since we are almost definitely going to need interior mutability to move mutable refs around, I am curious if this is a workable substitute now to a Rc<Cell<T>>

Saying that though, aren't you just going to end up with lots of bugs? The expected behaviour is that there is 1 mutable shared reference and all access to it will give the same thing. It sounds to me like with a COW you could end up with separate desynchronised mutable copies of the same var, which will no doubt diverge from dotnet behaviour, no?

[ncave]

@alexswan10k Perhaps, I was thinking about it in the context where we pass everything by value (includinc Rc). I didn't go as far as actually trying it.

[alexswan10k]

Heh fair play. One to keep in mind anyway.

By the way I was thinking more about async functions and I am reasonably confident you could carry on using refs in. The thing is the function could internally just clone everything it needed to keep in scope just before the first continuation, basically the same as what we do for closures. That way we maintain consistency of refs in, value/owned out.

[ncave]

@alexswan10k Ok, I think I fixed my "closure" issue with generics.

[alexswan10k]

I wanted to just throw down some thoughts while they are fresh in my head, no action required.

Firstly, pretty confident we can get pass by both ref/value based on context working again once function registrations with reftracking is complete, so it is definitely worth keeping that logic around. The way we can get around the uniform generics is by treating all generics as by ref, and aligning mismatched function parameters explicitly where a non-generic function feeds a generic one. We have to pull them apart anyway with the closure processing, so it makes sense to check against the definitions then, and unwrap/wrap, deref/borrow accordingly. That being said, function registrations is one we agreed to delay as it is a complex optimization that can wait. Just one to keep in mind though.

Here is an example:

            pub fn map<a_: Clone, b_: Clone>(f: &impl Fn(&a_) -> b_, x: &a_)
             -> b_ {
                f(x)
            }
#[test]
            pub fn fn_as_param_should_also_accept_static_functions() {
                let a: i32 = 3;
                {
                    let actual_4: i32 =
                        crate::Fable::Tests::Closure::map(&(move |x_1: i32|
                                                                crate::Fable::Tests::Closure::staticFnAdd1(x_1)),
                                                          a);
                    assert_eq!(actual_4, 4)
                }
            }

Notice how x_1 is an int (value) but the underlying map abstracts it to a &T, so this now breaks. Since we know this is a ref from the function registration, and the target closure is mismatched (it is declaring x as an int and not an &int) we could (at the boundary) plug the gap by adding in any referencs/dereferences etc. With a raw closure you could just adjust the closure parameters, but with a declared function you obviously can't do this as it is fixed. You would then have to create a bridging closure to convert a int to an &int.

On a side note I just read this https://doc.rust-lang.org/nomicon/aliasing.html and perhaps this whole optimisation is not even required as the rust compiler already does something equivalent.

Secondly, been thinking a lot about how the hell we do parallelized code. It really is quite clear that there is no way to elegantly abstract over both Rc and Arc, but it does look like the community has some examples to draw from such as this. Interestingly they just drop two libraries for immutable, one for the single threaded context, one for multi threaded. That got me thinking. Since we are generating code as it is, i wonder if we could simply duplicate the output of every type and every function for both the Rc case and the Arc case, and even add conversions in between. Obviously this is something you would want to be able to turn off at the compiler level if you are only working in a single threaded context, but it might give a way to be able to easily transition to a multi-threaded environment without too many compromises.

I imagine this would be quite useful for a Rust user too, who can build out their whole domain model in F#, knowing full well that they do not have to commit to either scenario. If they then find they have a Rc<ThingWithDeepTree>, and actually they suddenly need to throw it over a thread move closure (asking for send and sync) and need an Arc<ThingWithDeepTree>, they could just call myThing:.cloneToArc(), and the whole subtree is then switched. For any functions, you might have a fn_name(...Rcs) -> Rc and a fn_name_parallel(...Arc's) -> Arc, where _parallel functions would recursively redirect all function calls onto their _parallel dependents.

Again parallelization is not something that has to be tackled any time soon for this to be useful, but in order to plug into web frameworks and other more high level abstractions, it is probably something that needs a solution down the road.

[ncave]

@alexswan10k I agree we should delay this optimization, as it might not be needed. I also like the easy mental model of passing everything by reference, it's just easy to remember and reason about when looking at the generated code.

I added some basic list support, should be able to extend it further. Also tried to rework the imports, wrangling the Rust module system is not for the faint of heart and is not very fun. Documentation doesn't really help, you just have to try things.
Anyway, it more or less works now, hopefully it doesn't break any of your stuff.

[ncave]

@alexswan10k We should be able to generate either single-threaded Rc/Cell, multi-threaded Arc/Mutex or Gc/GcCell, based on compiler switch. I don't think we need to do both at the same time, since Fable-compatible libraries are distributed with source code in the package, so they can always be recompiled for the right target.

[alexswan10k]

I do agree that the simpler models are probably the best fit for now, Let's see where we end up i guess (I am just thinking way ahead).

Nice work on list. I might have another look at classes the next time I get some time, although I was really struggling with these generated constructors. Perhaps one of the other language targets has done some legwork we can copy.

[ncave]

@alexswan10k On the other hand, passing everything by value (including Rc) was a simple model too. And I'm not entirely sure (without doing any benchmarking) that the cost of Rc cloning justifies the additional complexity and edge cases we introduced with the reference tracking. What would be a good benchmark in your opinion to test the performance difference of the generated code (by val vs by ref)?

[alexswan10k]

I imagine something like this would do it:

  type Wrapped = {Value: int}
  let Sum(a: Wrapped, b: Wrapped, c: Wrapped, d: Wrapped, e: Wrapped, f: Wrapped, g: Wrapped) =
    a.Value + b.Value + c.Value + d.Value + e.Value + f.Value + g.Value
  let Test () =
    let mut x = 0
    let a = {Value = 1}
    let b = {Value = 1}
    let c = {Value = 1}
    let d = {Value = 1}
    let e = {Value = 1}
    let f = {Value = 1}
    let g = {Value = 1}
    for i in 0..1000000 do
      x <- x + Sum(a, b, c, d, e, f, g)
    ()

An extreme example I know, but you can see how this sort of thing might be problematic in tight loops. Also just bear in mind that Arc has a higher penalty as clones need to be atomically counted.

I appreciate your desire to try to simplify the model as much as possible, but it is probably also worth considering what a Rust user would expect from any generated API surface. I think it is generally expected that you borrow if you can.

Are references giving you trouble?

[ncave]

@alexswan10k

Are references giving you trouble?

No, not really, I was thinking more about potential future edge cases, and the fact that any new feature we add needs to be included in the ref counting. But that's ok.

I was just saying we need empirical evidence to see which one performs better. I've run your example with two magnitudes higher and yes, it looks like passing by value is 5x slower than passing by reference in release mode (7x slower in debug mode), most probably because of the extra cloning in tight loops, not the actual passing by value.

So I guess references are here to stay.

Release:
byRef count: 100000000, result: 700000007, elapsed: 0.1596173
byVal count: 100000000, result: 700000007, elapsed: 0.7738353

Debug:
byRef count: 100000000, result: 700000007, elapsed: 9.9654719
byVal count: 100000000, result: 700000007, elapsed: 70.0745736

[alexswan10k]

@ncave it is good to have some quantifiable evidence around this though. I would have been happy to concede if there really wasn't any difference! In some ways passing a Rc by value is sort of like passing by reference anyway (pointer to a value), but details.

Yes it was always the cloning that was my worry, pretty sure by-value will actually be faster because you do not have to dereference the pointer to access it, which is why I am still a bit frustrated that primitives have to also be passed by reference now. My initial thought process was really "how do we reduce cloning", and borrowing by reference is a good way to only clone something when you actually plan to hand it over to something else, not when you just read it. Although cloning is very cheap for Rc's, i think the problem is that it still has to hit the allocator as Rc is itself a small struct.

[ncave]

@alexswan10k Right, I wouldn't worry about passing primitives by ref for now, it will probably be hard to show that it's slower than passing by value, as the cost will be dwarfed by the rest of the code in the callee. In any way it's going to be a very very small cost, if any.

[alexswan10k]

Agree. Also the aliasing thing implies that (an optimization rust can make is that) values are dereferenced and bound locally on the stack frame anyway, so it might literally be a single dereference for that parameter, even if there are many useages of it in that function.

Bigger fish to fry :p

[alexswan10k]

Yeah are you happy if I try and get generated ctor classes working next? I am making some progress but a bit fiddly. Seems like a nice self contained thing. No worries if you are already in that area though.

Just some simple cases like this:

type CTest(x: int, y: string) =
    let a = x + x
    let y = y
    member this.Add m = a + m

[<Fact>]
let ``Class hellp`` () =
    let a = CTest(1, "hello")
    let r = a.Add(1)
    r |> equal 3

I am just thinking, if we can make some progress with classes then things like Enumerator can potentially be tackled.

[ncave]

@alexswan10k
Sure, if that's the area you want to tackle, hack away. Perhaps first we need support for classes and class members, then interfaces as traits, and after that IEnumerable as Iterator trait. Classes are not that different from records, just have additional members (static and non-static) with specific func signatures, look at the JS or other lang impl.

[ncave]

@alexswan10k Pro tip ;)
If you need to use a reserved symbol like Self, make it a raw string to avoid the sanitization, e.g. rawIdent "Self".

[alexswan10k]

I am completely stuck. Any idea how transformClassWithCompilerGeneratedConstructor works? If I make a trivial class, I cannot work out how to get to the inputs x and y, and the expression that defines the value of a.

type CTest(x: int, y: int) =
    let a = x + y
    member this.A = a

If I follow Fable2Babel transformClassWithCompilerGeneratedConstructor and the python/php equivalents, they all seem to read out FSharpFields and assume that each generated constructor parameter is a field and map 1-1 FieldName = AssumedParameterFromFieldName, which does not work for me as they are not 1-1. The field is defined by an expression that is not Ident fieldName. If I copy this approach I end up with this incorrect output:

            pub fn CTestnew(a: &i32) -> Fable::Tests::ClassTests::CTest<i32> {
                Rc::from(Fable::Tests::ClassTests::CTest::<i32>{a:
                                                                    a.clone(),})
            }

What makes even less sense is I have just tried this exact code in the online Fable REPL and the js coming out correctly. Somewhere there is an expression that defines what a is bound to, but I cannot find it anywhere, and yet Fable2Babel does not even seem to use it. The code makes no sense but yet it works for them and not for me! Clearly there is a piece I am missing here.

I have tried digging through the ctor also, but no avail. I can find the parameters by name, but not the expressions that set the fields. No other example does this either so I am probably barking up the wrong tree here anyway.

        let ctor = ent.MembersFunctionsAndValues 
                            |> Seq.tryFind (fun q -> q.CompiledName = ".ctor") 
                           // |> Option.map (fun ctor -> ctor.CurriedParameterGroups)

Fable.ClassDecl.Constructor is null for this example, which is also a little unexpected as presumably this would contain the expression body normally. In this case you are forced to create an AutoGeneratedConstructor, so presumably this is achievable.

Any ideas?

[ncave]

@alexswan10k If I'm not mistaken, you don't have to do that, the constructor body itself will initialize the fields.
You just need to define the fields and make declarations for all class members (the constructor is one of them).
Sorry, writing without reading first, let me take a look.

[alexswan10k]

There is no constructor body. If I run the sample, there is no output for a constructor at all ModuleMember, although there is an output for the add method interestingly. I am wondering if it could be something upstream?

[ncave]

@alexswan10k I think transformClassWithCompilerGeneratedConstructor just generates a constructor that takes all the fields as arguments, and the body is to set each field to its argument in the constructor. You can probably reuse a lot of that code.

[ncave]

@alexswan10k I made some changes in option matching, please merge with your work to avoid issues.
Also added initial support for Console.Write, Console.WriteLine and String.Format, as they map nicely to print!, println! and format! (obviously using the default formatting, so can't expect the same exact output). We can most probably rewrite the format string to translate .NET string formatting to Rust formatting, but that's a bigger task better left for later.

[alexswan10k]

This is great, i will update.

I agree, that was my understanding of transformClassWithCompilerGeneratedConstructor too. The problem here is that - from my example I am not sure that this should even be a generated constructor, but the AST does not contain a Constructor, so I have nothing else to go on. I wonder if in the Fable2Babel example there is something happening upstream to put this in, but I could not find anything obviously different in the pipeline of this or any other language output.

[ncave]

@alexswan10k I'm not sure I follow what the issue is, here is the logic that determines whether to use it or not:

match decl.Constructor with
| Some cons ->
    withCurrentScope ctx cons.UsedNames <| fun ctx ->
        transformClassWithImplicitConstructor com ctx decl classMembers cons
| None ->
    let ent = com.GetEntity(ent)
    if ent.IsFSharpUnion then transformUnion com ctx ent decl.Name classMembers
    else transformClassWithCompilerGeneratedConstructor com ctx ent decl.Name classMembers

we shoud be able to use that as is.

[alexswan10k]

Decl.constructor is none for the case I described

[ncave]

@alexswan10k

Decl.constructor is none for the case I described

Yes, so it will call transformClassWithCompilerGeneratedConstructor to make a member declaration for a (generated) constructor that takes a list of values for each field, and a body that assigns them to each field (by order).

Perhaps I'm missing entirely your issue, please add a small example.

[alexswan10k]

Compiler generated assumes symmetry between params and fields. My example has different params.

type CTest(x: int, y: int) =
    let a = x + y
    member this.A = a

[ncave]

@alexswan10k This looks like a normal constructor, not an implicitly generated one. The implicitly generated one will always have the same number of arguments as the fields, cause that's how it is generated (from fields).

In other words, the class's decl.Constructor should not be None in this case, if it is we'll have to figure out why.

[alexswan10k]

I agree. So would you expect decl.constructor to have a definition in it?

[ncave]

@alexswan10k In this particular case, yes.

[alexswan10k]

Yeah, I figured we might be missing something upstream but cannot see anything different between rust pipeline and fable/Python/php.

[ncave]

@alexswan10k Perhaps decl.Constructor is only set for classes with implicit constructor?
Anyway, even if it's None, you just need to call transformClassWithCompilerGeneratedConstructor to generate the implicitly generated one from fields. All other constructors are already in the AST as members.

[ncave]

@alexswan10k Perhaps things changed over time.
Looks like right now the decl.Constructor is always None, but don't let that stop you :)
You can just follow the same implementation as the other languages and create the implicit generated constructor, the code will call the right constructor.
Or you can do whatever you think is best, that works too.

[alexswan10k]

Oh ok, good to know. The problem is when I do that, the parameters don't line up and the code won't work. I end up with a Constructor with 1 param as I expect it to have 2. Something is missing. Don't worry, I will have another look tomorrow, must be missing something.

[ncave]

@alexswan10k On a side note (in case I forgot to mention it):
Thank you for tackling the closures and the reference tracking, great work! I really appreciate your help!

[alexswan10k]

Most welcome :) hopefully it hasn't complicated stuff too much haha

[ncave]

@alexswan10k Classic off-by-one error! Just don't do that.

(that was a reference to a joke: "A lot of beginner C programmers, what they do is, they write bugs in their code. Classic mistake. Don't do that.")

More seriously, I can only guess, without stepping on your toes and trying to implement it myself.
There should be 2 constructors for that class above (one from AST, one generated).
Both need to be expressed as Impl, and the code should be calling the correct one.

[alexswan10k]

Yeah funny you say that, when I run the fable repl there is what looks like 2 ctors but neither is consistent with generated. One is a static wrapper fn, and the es6 ctor has the logic. I can't find any reference to the body logic in my fable ast or the static wrapper. I only get instance methods.

Let me give it 1 more pass 2mo in case I missed something, just annoying me cos this needs to work heh. We will get there.

[ncave]

@alexswan10k
The primary constructor should be there in the members list (from AST), the other one you generate.
Since JavaScript does not support Function Overloading, the machinery to call the correct constructor is already there, you just need to use it.

[alexswan10k]

So this is probably the other half of the problem. It is not in the members list. Well, it is on the entity but those are fsharp compiler pass throughs, they are not fable constructs, and there is no expression that can be used.

[ncave]

@alexswan10k I figured it out after a bit of debugging. It was my fault after all, I'm afraid. I broke it when I enabled the nested modules in Fable for Rust (normally module decls are turned off for JavaScript), so it wasn't attaching the members to the class declarations. Sorry about that, it's fixed now, there should be decl.Constructor with something in it.

[alexswan10k]

Awesome work. Don't worry, stuff sometimes breaks when you move fast, just super happy we (you) got to the bottom of it! Should hopefully be quite straightforward now to get the example up and running. Thanks!