Replace DotLiquid with Fluid

[lahma]

TLDR; C# generation is now 15x faster and TS generation is 35x faster

There were some problems due to DotLiquid allowing invalid Liquid syntax (elseif and some c# constructs) so some templates also needed to be changed. DotLiquid also didn't have correct whitespace handling for stripping trailing whitespace so that needed to be changed in templates. Target framework is now netstandard2.0/net461 which reflects what Microsoft officially supports.

• change target frameworks to be minimal nestandard2.0 and net461
• fix templates to use {%- %} instead of {% -%}
• fix invalid Liquid, elseif should be elsif, some invalid if conditions

fixes #1345

Benchmarks

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
AMD Ryzen 7 2700X, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=5.0.201
  [Host]     : .NET Core 5.0.4 (CoreCLR 5.0.421.11614, CoreFX 5.0.421.11614), X64 RyuJIT
  DefaultJob : .NET Core 5.0.4 (CoreCLR 5.0.421.11614, CoreFX 5.0.421.11614), X64 RyuJIT

Diff	Type	Method	Mean	Error	Gen 0	Gen 1	Gen 2	Allocated
Old	CsharpGeneratorBenchmark	GenerateFile	13,388.5 μs	95.55 μs	781.2500	46.8750	0.0000	3212.81 KB
New			826.3 μs (-94%)	0.80 μs	110.3516 (-86%)	0.9766 (-98%)	0.0000	453.05 KB (-86%)
Old	JsonSchemaBenchmark	FromJsonAsync	708.9 μs	13.46 μs	81.0547	7.8125	0.0000	331.29 KB
New			714.1 μs (+1%)	4.08 μs	81.0547 (0%)	7.8125 (0%)	0.0000	331.29 KB (0%)
Old	JsonSchemaGeneratorBenchmark	GenerateFile	1,802.4 μs	10.39 μs	123.0469	1.9531	0.0000	502.4 KB
New			1,814.5 μs (+1%)	12.39 μs	123.0469 (0%)	1.9531 (0%)	0.0000	502.45 KB (0%)
Old	TypeScriptGeneratorBenchmark	GenerateFile	45,957.4 μs	236.39 μs	3000.0000	0.0000	0.0000	12322.81 KB
New			1,213.0 μs (-97%)	3.64 μs	179.6875 (-94%)	1.9531	0.0000	735.84 KB (-94%)

[microalps]

Just out of curiousity, we made some changes to the parser in dotliquid. How does that benchmark for you? There are more improvements to be made but an idea of what needs to be done to keep DotLiquid would be good. Try compiling latest with this patch https://github.com/dotliquid/dotliquid/pulls/422

[lahma]

@microalps honestly I think it would serve DotLiquid best to have a benchmark suite in its own repository. I would think @sebastienros wouldn't mind if you use Fluid's comparison suite as starting point, you get a nice comparison report with it.

[sebastienros]

@microalps have you looked at the benchmarks on the Fluid's page? It has DotLiquid ones. You can clone them and reference your fork to run them. But I doubt you would be able to "change" DL to the point it's comparable. At least for rendering it should be possible since it's less intrusive, no Regex should be necessary.

[RicoSuter]

Thanks for this PR - looks promising.
It's just important that we do not break anything (keep same output).
Will you also update the NSwag template as soon as this NJS version is updated there?
Apart from that do we need to do any code changes in NSwag?

[lahma]

It's just important that we do not break anything (keep same output).

Currently the "regressions" I see are some whitespace changes on NSwag side, like newlines and like removed whitespace for empty lines. These can be tweaked on NSwag side. I have already a branch that has template changes that fix most of the findings I have against TS/C# generation.

Will you also update the NSwag template as soon as this NJS version is updated there?

I'm committed to create a PR to integrate with NSwag and tweak until acceptable. Templates require mostly changes like from -%} to {%- usage. Some newlines etc.

Apart from that do we need to do any code changes in NSwag?

Biggest change is the target frameworks, issue here: RicoSuter/NSwag#3384 . Easier maybe to do in iterations if we have open communication line to get things rolling.

Here's the current diff for changes: https://github.com/RicoSuter/NSwag/compare/master...lahma:fluid?expand=1

[lahma]

I've rebased against master and updated the benchmarks (master with new Namotion.Reflection and this branch with new Namotion.Reflection).

[RicoSuter]

This is a bit a problem as i know there are ppl using this lib with netstd < 2.0 and also with .NET 4.0
Is this really required?

[lahma]

I restored the old targets, seems that we can keep this project targeting as-is. Good catch.

[lahma]

Converted to draft as I still need to one bigger round verifying the generated output (both our internal code base and NSwag outputs).

[RicoSuter]

didn't have correct whitespace handling for stripping trailing whitespace

So Fluid correctly handles this as specified by Shopify?

Converted to draft as I still need to one bigger round verifying the generated output

Please ping me when it's ready then ill have a look and we can continue integrating this (eg in NSwag).

[lahma]

didn't have correct whitespace handling for stripping trailing whitespace

So Fluid correctly handles this as specified by Shopify?

Yes, Fluid is very much stricter and follows the Shopify spec by the dot, correct me if I'm wrong @sebastienros .

Converted to draft as I still need to one bigger round verifying the generated output

Please ping me when it's ready then ill have a look and we can continue integrating this (eg in NSwag).

I'll work with these, the latest changes in master just broke things a bit more, I'll probably submit a PR to fix regression and new performance issue.

[sebastienros]

It follows Shopify as much as it can. And uses options to diverge. Same for filters, all based on specs, tests or manual testing on shopify.com directly.

[lahma]

Seems that upgrading to latest Namotion.Reflection fixed the test problems, back to the original quest...

[RicoSuter]

Any updates on this PR?

[lahma]

@RicoSuter I'm glad that you are still interested in this. I just returned from summer vacation so I'll try to sync this, it's a bit of a hurdle as I need to test both against the NJS and NSwag. But unfortunately there probably still be regressions due to Fluid being more standards compliant.

[RicoSuter]

@lahma good point, let's merge this ASAP... do you also have the work ready for the NSwag side so we can do this "quickly"?
And could you quickly fix the conflict? Then I merge this tomorrow.

[lahma]

@RicoSuter working on it, I'll ping back when I have verified that I get decent results out from the full toolchain

[RicoSuter]

PR looks good, thanks for that.
Can we merge this?

[lahma]

@RicoSuter I think we are good to go, please be prepared that we might need couple iterations (hence beta), as this is quite complex beast. I have NSwag repo with required changes locally (local csproj references), let's see how it integrates with new package in real life...

[RicoSuter]

As this is quite the breaking change, I'd like to push NJS with a new major version and also add more breaking changes...
How can we continue on this feature in NSwag... can you create a PR with all your changes except the pkg updates or do we need a preview package of NJS for now?

[lahma]

I can create a draft PR which references packages from say, a lib folder as a temporary solution.

[RicoSuter]

I have the fear that this change is not thread-safe:

If I run all tests multiple I see strange failures...

[lahma]

I need to run some tests and try to reproduce, the template engine should be thread safe.

[RicoSuter]

I just run the csharp tests multiple times and sometimes it fails in the code output.

[sebastienros]

I don't see how it is related to thread safety. Do I miss something or is the issue that a space is missing between double and SomeOptionsProperty?

[RicoSuter]

I don't see how it is related to thread safety. Do I miss something or is the issue that a space is missing between double and SomeOptionsProperty?

yes, in this case, but i saw also other failures (rarely) where the output is missing other text...

[RicoSuter]

So IFluidTemplate.RenderAsync is thread-safe (if called in parallel)?

[sebastienros]

That's the goal ... even for the same template. But your description of the issue feels familiar, I fixed a bug like this by the past, checking...

[sebastienros]

Can you point me to the TextWriter and TextEncoder that is used? I remember it was a bug in a dotnet 5.0 hot fix (a breaking change) and I want to check if that could be this one. Unless you are not using 5.0.

[lahma]

The template usage code is here. It uses default writers and encoders as it uses a strings output.

[lahma]

@RicoSuter we are investigating and report back when we are wiser.

[RicoSuter]

@lahma can you repro this? Not sure where the problem is... must not be something with your change.

[sebastienros]

Good news is that we can both repro locally by adding some concurrent runs, we should manage to isolate the problem.

[sebastienros]

I believe I found the bug in Fluid. I suspected some code, read it, then understood how it could be an issue which happens to be exactly what we are seeing. Fixed the piece of code and now I can repro the bug. Will ship a new Fluid version in a few minutes.

[lahma]

@RicoSuter OK so @sebastienros identified the problem, fixed it and now there's a new release (thank you!).

I've opened #1426 with the new version, I can no longer get unit tests fail even running the parallel test we used in "Run until failure" mode.

Thanks @RicoSuter for testing and reporting the problem!

And I see it's already merged 😄

[lahma]

For those interested, NSwag migration to use Fluid is here: RicoSuter/NSwag#3679