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

Moar dart collections #2862

Merged
merged 9 commits into from
Apr 27, 2022
Merged

Moar dart collections #2862

merged 9 commits into from
Apr 27, 2022

Conversation

alfonsogarciacaro
Copy link
Member

@alfonsogarciacaro alfonsogarciacaro commented Apr 26, 2022

This implements most of the Array, Seq and List modules in Dart, although I haven't added many tests yet. @ncave for Lists I'm trying to reuse your work from #2154, let's see if we can make the optimizations work for Dart :)

@dbrattli I had to (partially) comment a test in Python. I'll try to have a look, but it shouldn't be very serious because it's about converting lambdas to delegate and specifying only one single generic argument, which should be rare.

@ncave Unfortunately I also broke Rust 😅 Because Dart is statically typed, it also tells me where types went wrong during Fable transforms. I'm trying to fix this but it may happen that this breaks some of your workarounds. Could you please have a look? If you cannot spot the issue I can try reverting some of the changes.

@alfonsogarciacaro alfonsogarciacaro changed the title [WIP] Moar dart collections Moar dart collections Apr 26, 2022
@ncave
Copy link
Collaborator

ncave commented Apr 27, 2022

@alfonsogarciacaro Fixed in #2865.

Also, another option for Dart linked list could be the simple list implementation for Rust. IMO it's quite hard to optimize List performance in the general case without compromising too much with either memory footprint, or degraded performance in certain use cases (e.g. heavy use of the list as a stack, under heavy load of push/pulls).

Ideally (eventually) we would come up with a single List implementation that performs well in all languages, but that's way in the future, so whatever works.

@alfonsogarciacaro
Copy link
Member Author

Thanks for fixing this so quickly @ncave, let's merge this! Let's see how this works out, my hope is that because we're not planning to self-compile FCS/Fable to Dart we can just ignore the case of list heavily used as a stack 🙈 🙉 🙊 But you're right we'll probably need to optimize for the case when you add to a list when the hidden count differs from the actual count by a certain threshold (say 4) so we reduce the chances of big tails lingering around with inaccessible items that cannot be released from memory.

Not sure it will be possible to have an "optimized" version of List compatible with all languages. Here I can use an underlying mutable list because Dart as JS is single-threaded, but I assume this would be riskier in Rust/Python because two threads may try to push into the list at the same time.

BTW, forgot to mention this PR also touches Python/Rust code because I made a change in the way arrays are represented in the Fable AST. Now they can represent resize/fixed mutable/fixed immutable, which hopefully will help with optimizations. I also tried to make ArrayValues/ArrayFrom more consistent and five ArrayAlloc its own entry. I was thinking to remove ArrayAlloc because it's not really used for JS (allocations are made with runtime code) and they're not useful for Dart either because of non-nullness. But maybe it can be useful for Python/Rust. If that's not the case please let me know and we can consider removing it.

I've also added the XmlDoc to class and member/function declarations. I'm not using them yet but hopefully this makes it easier to build native libraries using F#/Fable. I'd like this to be the last big change in Fable AST so we can lock it for Snake Island, update the plugins, and start pushing alpha releases also for JS users.

@alfonsogarciacaro alfonsogarciacaro merged commit f5534cf into snake_island Apr 27, 2022
@ncave
Copy link
Collaborator

ncave commented Apr 27, 2022

@alfonsogarciacaro I guess what I was trying to say is, a simple list implementation is probably better as it can be more easily reasoned about and inter-oped with from native code. But whatever works is fine too.

@dbrattli
Copy link
Collaborator

dbrattli commented Apr 27, 2022

@alfonsogarciacaro I'm looking into the failing Python test. It's related to function getting unit as the first argument will not receive anything which does not work for lambdas in Python. But I'll also update the partially apply code to handle more than arity 8 as with JS.

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.

3 participants