[WIP] Optimized List module

[ncave]

@alfonsogarciacaro

• Moving the List object to the List module enables more optimizations.
• Optimized the List module.
• Fixed issue with GetHashCode overwrites in objects.

It's passing the Fable test suite (Travis CI), but is still broken on larger projects like FCS.
Perhaps trying it on some other projects will help narrow down the remaining issues.
Works now, but is about 10% slower than stock list, so further optimizations need to be explored.

[gitpod-io]

[ncave]

Rebased to latest.

[alfonsogarciacaro]

I'm wondering if having actual tails would help in reducing memory duplication. I tried to do it here (still in Typescript) but it makes the tests for Set equality/comparison fail: 9b5d2f2#diff-5d6eaef3f7bddeafd82b3ce2088460a3R91

[ncave]

@alfonsogarciacaro It's fixed in #2127.

[alfonsogarciacaro]

It'd be great if we could add an optimized version of toArray and maybe also toReverseArray and use it for the reverse function (or just allocate the array, fill it, and assign it to the new reversed list).

[ncave]

@alfonsogarciacaro Implemented the reverse optimization. It doesn't seem to have huge performance benefits (although it will save some allocations), most probably my benchmark (FCS) is not bottlenecked by it.

[alfonsogarciacaro]

I think it would be faster to allocate the array and then use a for loop to fill it (that way we save the reverse step too). In some benchmarks in the past Fable's Array.map was faster than native JS Array.map and I think it's because of this. The following code is illegal in .NET but it will work in JS:

let values = ResizeArray<_>(xs.Count)
for i = 0 to xs.Count - 1 do
    values.[i] <- f xs.Values.[xs.Count - 1 - i]

Btw, ResizeArray<_>(xs.Count) currently compiles to [] I need to fix it compile to ArrayAlloc instead (we cannot use Array.zeroCreate because of the type and because it already runs .fill)

[alfonsogarciacaro]

Actually I just noticed we cannot compiler ResizeArray(5) as new Array(5) because this would make the array of length 5 already. We will have to emit some js if we want to allocate the array.

[alfonsogarciacaro]

[<Emit("new Array($0)")>]
let allocateArray (capacity: int): ResizeArray<'T> = jsNative

let values = allocateArray xs.Count
for i = 0 to xs.Count - 1 do
    values.[i] <- f xs.Values.[xs.Count - 1 - i]

[alfonsogarciacaro]

@ncave This is great. Definitely having more F# in fable-library is the way to go. Easier to maintain and more dogfooding. With the comeback of the plugins in Fable 3 I'm thinking how we can split and move the Replacements module away from Fable.Transforms to reduce the magic and also to make it possible for users to create their own replacements for their projects.

Chalk it to complete coincidence if you must, but I've just spent the last few days thinking (and testing) the same exact thing (literally the same, reverse dynamic arrays as lists). Minds thinking alike and all that, but the exact timing is intriguing, to say the least.

That's funny. I hope those who say I'm ncave and have a personality disorder don't read this 😂 For me probably the trigger was reading that in the next version of ReasonML, arrays will be default. Maybe it was the same for you?

Also, I've been looking into the possibility to replace the Seq module with the F# implementation (pretty much as it is in FSharp.Core), so we don't have to use iterators when downstream compilers don't support JS iterators (e.g. assemblyscript).

I made seq compile to JS iterators to improve the interoperability with JS, but I don't think many users are sending F# list to JS so we can use IEnumerator or something more optimized instead. On a related note, I want to support better your work in Fable 3 and avoid fragmentation, so for example, I think we should compile to class types by default. I have already started work on that but I haven't pushed it yet because I go distracted by another AST cleanup (the last one, I promise!) and I was a bit busy the last days.

[alfonsogarciacaro]

Reply to #2122 (comment)

@krauthaufen The ideas for the optimization are:

• Different lists can share the same stack, we only create a new stack when rebranching a list from another one that is not pointing to the end of the stack. But maybe in that case we could use an actual tail, see: [WIP] Optimized List module #2124 (comment)
• We try to avoid using Cons as much as possible. For list literals in code, Fable builds the stack directly at compile time, and most of the optimized functions in the List module would work with the underlying stack.

But sharing the stack maybe problematic in some scenarios as this seems to cause issues when compiling FCS/Fable, let's see.

[alfonsogarciacaro]

It's passing the Fable test suite (Travis CI), but is still broken on larger projects like FCS.
Perhaps trying it on some other projects will help narrow down the remaining issues.

@ncave I will try with some of my projects to see if I can pinpoint the issue.

[ncave]

@alfonsogarciacaro

• I had to restructure the List module to use only the List object API. Optimizations are based on assumption of random access in List.Item. Hopefully this would make it a bit easier to switch between the List object in Types.ts and the one in List.fs.

For me probably the trigger was reading that in the next version of ReasonML, arrays will be default. Maybe it was the same for you?

No, it was just a random thought to optimize the list performance using a dynamic array (due to better memory locality). But the timing was too perfect, so I can't exclude telepathy from the realm of possibility.

[alfonsogarciacaro]

Now that you're back to the Typescript definition and CI is working again I'm thinking that may be the problem is Equals wasn't overriden in the F# definition so the default record equality was used when the type couldn't be resolved at compile time.

This made think as well on the way we're attaching behavior to F# object through inheritance from the types defined in Types.ts, in connection with your findings that compiling with classes resulted in a performance decrease. Maybe this could be related to the fact that all the compiled F# objects are inheriting. This post says that inheriting with classes is much slower than with prototypes, it's a bit old but the responses contain links to jsperf tests that give similar results with modern browsers: https://medium.com/@gregsolo/es6-classes-vs-prototypes-performance-overview-dcab1e2fca9b

If this is true, maybe we could avoid the inheritance by adding a __kind field to objects so we know what kind of default equality/comparison behavior should be applied.

[ncave]

@alfonsogarciacaro Yeah, I figured that much. Now that I think of it, of course the default Record equality and comparison are insufficient, there can be different representations of lists that are equal. I'll fix it.

[ncave]

maybe we could avoid the inheritance by adding a __kind field to objects so we know what kind of default equality/comparison behavior should be applied.

I guess you mean for Records and Unions and Value types, yeah, that can work (instead of having to implement structural equality somewhere in the prototype).
Yes, I've read similar articles before that inheritance in es6 is slower, hopefully if we take it out of the F# types, we'll gain back most of the lost performance for JS classes.

[alfonsogarciacaro]

Well, also for classes, because right now everything is inheriting from SystemObject by default. This could be also an opportunity to implement support for records/unions with NoEquality and the like attributes, which hasn't been really implemented in Fable 2. https://docs.microsoft.com/en-us/archive/blogs/dsyme/equality-and-comparison-constraints-in-f

[ncave]

@alfonsogarciacaro Moving the list implementation to F# makes it slower than the TS impl, but it turns out the reason is entirely in the list iteration, and more specifically the use of property getters (e.g. IEnumerator.Current) instead of methods. Getters seem a lot slower than instance methods, and even more so in Firefox than in Chrome, see https://jsben.ch/O6EXA.
I'm wondering if using property getters/setters instead of instance methods is impacting performance elsewhere too.

[alfonsogarciacaro]

@ncave Sorry for the late reply. Wow, it's incredible you managed to pinpoint the issue. Hmm, we cannot compile interface getter as methods if we don't want to break current JS interop. Though in Fable 3 we'll have the mangled interfaces (and interfaces from System namespace will be mangled by default) and those compile getters/setters as methods #2084 I added an exception for IEnumerable because that interface is used in Typescript too, but that could change if we port the remaining TS parts in fable-library to F#...

[alfonsogarciacaro]

Instead of adding ad-hoc exceptions, we can get this "for free" in Fable 3 by removing Enumerator from the list of non-mangled System interfaces here (though in that case we need to change the TypeScript code that uses it) as getters/setters in mangled interfaces compile to methods:

Fable/src/Fable.Transforms/FSharp2Fable.Util.fs

Lines 1030 to 1046 in e84aa4d

	let isMangledAbstractEntity (ent: FSharpEntity) =
	match ent.TryFullName with
	// By default mangle interfaces in System namespace as they are not meant to interact with JS
	// except those that are used in fable-library Typescript files
	| Some fullName when fullName.StartsWith("System.") ->
	match fullName with
	| Types.object
	| Types.idisposable
	| Types.icomparable
	| "System.IObservable`1"
	| "System.IObserver`1"
	| Types.ienumerableGeneric
	| Types.ienumerator
	// These are used for injections
	| Types.comparer
	| Types.equalityComparer -> false
	| _ -> true

[ncave]

@alfonsogarciacaro I'm not sure we can do that reliably without replicating the name mangling in TS. Also, this PR targets v.current, not v.next. Anyway, this is still WIP, since the perf in this PR is still below the original Types.List.

[ncave]

Closing this in favor of #2154 (towards next branch).