Skip to content
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

Using F# List and Seq types #2348

Merged
merged 8 commits into from
Mar 3, 2021
Merged

Conversation

ncave
Copy link
Collaborator

@ncave ncave commented Jan 17, 2021

  • Replaces JS List type with F# List type.
  • Replaces JS Seq type with F# Seq type.
  • Performance is on par (or slightly faster) than the main branch.

@ncave ncave marked this pull request as draft January 17, 2021 04:09
@ncave ncave mentioned this pull request Jan 17, 2021
19 tasks
@alfonsogarciacaro
Copy link
Member

Hmm, if we rule out the overhead of inheriting from Record, the only thing I can think of now is the transformation from IEnumerator to JS Iterator (as we've seen several times small changes in iteration make big performance difference). Right now the Seq module works with JS iterators, so for classes implementing IEnumerable a method is automatically attached like this:

    [Symbol.iterator]() {
        return toIterator(this.GetEnumerator());
    }

This calls the following code:

export function toIterator<T>(en: IEnumerator<T>): IterableIterator<T> {
return {
[Symbol.iterator]() { return this; },
next() {
const hasNext = en["System.Collections.IEnumerator.MoveNext"]();
const current = hasNext ? en["System.Collections.IEnumerator.get_Current"]() : undefined;
return { done: !hasNext, value: current } as IteratorResult<T>;
},
};
}

A way to test if this, could be to compiler fable-library with your changes first, then manually replace List[Symbol.iterator] with:

    [Symbol.iterator]() {
        let cur = this;
        return {
            next: () => {
                const value = cur === null || cur === void 0 ? void 0 : cur.head;
                const done = (cur === null || cur === void 0 ? void 0 : cur.tail) == null;
                cur = cur === null || cur === void 0 ? void 0 : cur.tail;
                return { done, value };
            },
        };
    }

And after that, running the benchmark to see if there's any difference. If there is, it would make sense to transform Seq.ts into F# and use IEnumerable as Fable code most of the time will iterate over F# structures (besides arrays). In the case of JS iterables we could make the inverse conversion from JS iterator to IEnumerator.

@ncave
Copy link
Collaborator Author

ncave commented Jan 18, 2021

@alfonsogarciacaro Iteration doesn't seem to be a problem in micro-benchmarks, but I'll try that.

@ncave
Copy link
Collaborator Author

ncave commented Jan 22, 2021

@alfonsogarciacaro I stand corrected, just replacing the iterator (as you suggested above) improves the FCS-JS benchmark performance by 20%.
It may be worth exploring a switch to F# seq, to see if this perf gain can be retained.

@ncave ncave force-pushed the next1 branch 3 times, most recently from 10dcd15 to fb508f7 Compare February 16, 2021 21:19
@ncave ncave changed the title [WIP] Using F# List type [WIP] Using F# List and Seq types Feb 28, 2021
@ncave
Copy link
Collaborator Author

ncave commented Feb 28, 2021

@alfonsogarciacaro

it would make sense to transform Seq.ts into F#

Done :)

Performance is more or less on par with the main branch (about 5% slower).

@ncave ncave marked this pull request as ready for review March 1, 2021 22:14
@ncave ncave changed the title [WIP] Using F# List and Seq types Using F# List and Seq types Mar 1, 2021
@ncave
Copy link
Collaborator Author

ncave commented Mar 2, 2021

@alfonsogarciacaro

Performance of generated code is now at the same level as the main branch (or slightly better), so this is ready for merging.

@alfonsogarciacaro
Copy link
Member

Awesome @ncave! Fantastic work as always 👏 👏 👏 Looking at the code, it seems classes implementing IEnumerable will still get JS Symbol.iterator and getting the enumerator is still compatible with JS iterables so there shouldn't be compatibility issues 👍 Hopefully this also helps @dbrattli by making fable-library less JS-dependent.

@alfonsogarciacaro alfonsogarciacaro merged commit da4eb73 into fable-compiler:nagareyama Mar 3, 2021
@ncave ncave deleted the next1 branch March 3, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants