Skip to content

Conversation

@ncave
Copy link
Collaborator

@ncave ncave commented Feb 11, 2017

@alfonsogarciacaro Thanks, fixing U2 serialization worked. Babel AST can't be used directly so serialization/deserialization is still needed, but it works great now. I've reduced the dependencies back to the original set as that's all that's needed for most scripts, but more can be added if need be.

@ncave
Copy link
Collaborator Author

ncave commented Feb 11, 2017

Anecdotally, Fable on Chrome seems to be 2x as fast as Firefox, and 4x as fast as Edge. Not sure why. It's still at least a magnitude slower than BuckleScript, but hopefully some performance tuning can help.

@alfonsogarciacaro alfonsogarciacaro merged commit 09bc338 into fable-compiler:narumi Feb 11, 2017
@alfonsogarciacaro
Copy link
Member

alfonsogarciacaro commented Feb 11, 2017

Wow, I got it working! Awesome work, @ncave! 👏 🎉 💃

I'll upload it to the web (without any link from the main page yet) so people can give it a try before polishing it and making it official.

About the AST, you're right, the typed objects cannot be passed to Babel directly. I'll try to modify the Babel AST to use Pojo types instead.

On performance, we can try to play with webworkers to see if this gives us a boost. I'm also concerned about the time to download the binaries when the REPL is hosted online.

The innerworkings of FCS remain very mysterious to me, and most of my knowledge comes from a couple of tutorials and trial-and-error, but from my experience the biggest bottleneck is the warm up time to parse a project with a fresh FSharpChecker. That's why in watch mode (and in the original REPL) I always recycle the checker. Apparently, it doesn't mind if you parse an entirely new project or script, a hot checker will be much faster. I don't know where this warm-up time is being spent, and whether it applies to the JS version or not, but if it does, we could try to keep a reference of the checker to speed up the compilations after the first one (or maybe even eagerly trigger a first dummy compilation in the background).

BTW, what's the difference between the FSharpChecker and the InteractiveChecker?

@ncave
Copy link
Collaborator Author

ncave commented Feb 11, 2017

@alfonsogarciacaro The InteractiveChecker is a quickly hacked together trimmed-down POC version of the FSharpChecker and FsiInteractiveChecker, with all the good parts removed (incremental build, file system, project support, etc.), so it's only single file parsing left. All the heavy initialization is happening in the InteractiveChecker constructor, so there may be some room for improvement if we don't create it every time and instead keep it around.

@ncave
Copy link
Collaborator Author

ncave commented Feb 11, 2017

@alfonsogarciacaro As you've already seen, the demo page is just a copy of the babel try it out page, with a really minor plug at the bottom to load the metadata, and in the middle to call the Fable compiler, for the purpose of a quick demo.

I'm not really sure if the demo page is the best foundation for a good live playground page, there may be better ones out there, but I'll leave that decision to you. I'm sure the community will contribute to make it a great page, if there is a branch for it.

@ncave
Copy link
Collaborator Author

ncave commented Feb 12, 2017

@alfonsogarciacaro There seems to be some difference in the babel processing in the demo page, can't figure out if it is an issue with the deserialization of the Babel AST, or something else:

If you compile this simple statement: printf "%d" 42 and get the AST:

Same AST, with Demo page, will compile to: (0, _String.fsFormat)("%d")("x=>{console.log(x)}")(42);
Same AST, Fable compiler, will correctly be: (0, _String.fsFormat)("%d")(x => {console.log(x);})(42);

@alfonsogarciacaro
Copy link
Member

Ah, damn! I forgot about that. The emit expressions are actually resolved by a custom Babel plugin, so this needs to be added to the demo page too. I'll send you a link when I'm home, but you can also easily find them in src/typescript/fable-compiler

I also found some issues debugging the compiler with a bit more complex code, I'll try to push the fixes later.

@alfonsogarciacaro
Copy link
Member

@ncave Great! I'm building on your amazing job and it seems little by little we're getting there ;)

  • It was not that easy to add the plugins because they had a dependency of babel-template, but I solved that by creating a custom build of babel-standalone that includes babel-template.
  • You don't need to copy the fable-core files, as I added the coreLib option to fableconfig, but you've to run npm i now manually.
  • If we compile FCS/Fable to ES5, there seem to be problems when calling methods from the parent class. So fableconfig compiles now to es2015 (as you had before) but I added the Babel plugin to change let/const as it seems this prevents some optimizations in V8 at the moment.
  • I've moved the compiler to a worker and create a cache for the checker. This improves the performance but still takes 3-4 seconds for long code (like the Sudoku test). I'm using Chrome. Also checked Bucklescript and it's very impressive btw.
  • There's a flash in the first compilation, I don't why, I've to investigate.
  • I set the options to output ES2015 for now so we can focus on the raw code generated by Fable and not on the Babel transformations.
  • I managed to minify the bundle using the harmony branch of UglifyJS. To try it, clone its repo, check out harmony, run npm i and then from the demo folder, run: node ..\..\..\..\..\UglifyJS2\bin\uglifyjs .\out\bundle.js -c -m -o .\out\bundle.js (If you output bundle.min.js instead, you need to edit worker.js).

You can see these changes in my latest commit. There are still some issues and for sure a lot of room for improvement. We can use the Chrome dev tools to find the bottlenecks. This will probably also help the dotnet version :)

@ncave
Copy link
Collaborator Author

ncave commented Feb 13, 2017

@alfonsogarciacaro

  • Yes, the first-time flashing is really annoying, it may be coming from the editor component.
  • fcs-fable was rebased to latest, please update.

@alfonsogarciacaro
Copy link
Member

@ncave Wow, that was fast. I updated it, thanks! However, after updating Fable to FCS 10.0.1 I'm getting the following error when trying to compile a script:

Cannot read project options: Error opening binary file 'c:\Users\Alfonso\dev\Fable\build\Fable\bin\FSharp.Core.dll':
The referenced or default base CLI library 'mscorlib' is binary-incompatible
with the referenced F# core library 'c:\Users\Alfonso\dev\Fable\build\Fable\bin\FSharp.Core.dll'.
Consider recompiling the library or making an explicit reference to a version of this library
that matches the CLI version you are using.

I need to solve this (if you've any hint, please tell me), but for now I just edited the Fable.Client.Browser.fsproj so using scripts as project files is not necessary. Please let me know what you think.

@dsyme
Copy link

dsyme commented Feb 13, 2017

@alfonsogarciacaro I wonder what that problem is. Do you have an explicit mscorlib reference on that command line?

@ncave
Copy link
Collaborator Author

ncave commented Feb 13, 2017

@dsyme @alfonsogarciacaro Yes, it seems to be about script references, so perhaps there is an issue there with fcs 10.01. It was working fine with 9.01, before this commit. We don't have a script test for this in FCS (well, we actually do, I added one in PR 654 but that's not merged yet, perhaps I can redo it to just have the test).

@alfonsogarciacaro
Copy link
Member

@dsyme Yes, it's as @ncave says. The problem happened after updating FCS to 10.0.1. It occurs when calling GetProjectOptionsFromScript on any script (like printfn "Hello").

Wild guess: FCS is resolving the core assemblies with the full .NET framework but for FSharp.Core it takes the one referenced by the calling assembly which in this case is the netstandard version (latest Fable is a netcore app).

@ncave
Copy link
Collaborator Author

ncave commented Feb 14, 2017

@dsyme FCS PR 654 was redone to just include the test.

@dsyme
Copy link

dsyme commented Feb 14, 2017

@ncave Ah ok. Do you know what we need to fix that failing test? I'm not at all sure.

@dsyme
Copy link

dsyme commented Feb 14, 2017

@alfonsogarciacaro
Copy link
Member

Thanks for the tip @dsyme, that may do the trick. Unfortunately I cannot test it, because the exposed method in FSharpChecker doesn't let you set assumeDotNetFramework. I'll send a PR to include it.

@alfonsogarciacaro
Copy link
Member

@dsyme @ncave Compiling scripts works again after updating FCS and setting assumeDotNetFramework to false. Thanks!

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