PathMarkupParser rewrite

[Gillibald]

• What does the pull request do?
  This PR changes the way how path markup is parsed in avalonia to make parsing more testable.
• What is the current behavior?
  The current implementation just writes commands to a StreamGeometry.
• What is the updated/expected behavior with this PR?
  The updated version produces PathGeometry instead of calling methods on the StreamGeometry directly. That way we can write better unit tests for the SVG spec.
• How was the solution implemented (if it's not obvious)?
  The new parser uses a regex to split the path data into command tokens that hold all arguments specified in the data string between command identifiers. That way we can identify implicit commands and process them later.

Checklist:

• Added unit tests (if possible)?
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/Avaloniaui.net with user documentation

Fixes #1606
Fixes #1640
Fixes #1659

[grokys]

Thanks for this @Gillibald - looks much better than before, the only thing I'm worried about is that it looks to be creating a lot of objects during parsing, I'm a bit worried about the memory traffic it will introduce. It might be worth creating some benchmarks with Benchmark.Net to see what's happening there. Don't know what you think @jkoritzinsky?

[jkoritzinsky]

I'm always up for more memory benchmarks! It's always better to have quantitative data when possible.

[Gillibald]

Will do some profiling. Converting to a PathGeometry introduces a bigger memory footprint but will habe a look where is room for improvements.

[grokys]

Yeah, I'm sure PathGeometry does use more memory, and I'm fine with that. What I'm worried about here is more memory traffic.

[Gillibald]

Looks like I am not the right person to write some benchmarks fot the new component. I probably need some help or atleast some advice.

[jkoritzinsky]

You'll probably want to use dotMemoryUnit for measuring memory traffic. So these tests would probably go in our LeakTests project.

[jkoritzinsky]

@Gillibald Here's the documentation for testing memory traffic: https://www.jetbrains.com/help/dotmemory-unit/Checking_Memory_Traffic.html

[Gillibald]

Will have a look. Thanks for your help.

[grokys]

@Gillibald I hope you don't mind but I just pushed a benchmark for PathMarkupParser. The results are:

           Method |     Mean |    Error |   StdDev |   Gen 0 |  Gen 1 | Allocated |
----------------- |---------:|---------:|---------:|--------:|-------:|----------:|
 Parse_Large_Path | 474.2 us | 10.94 us | 31.05 us | 27.3438 | 6.8359 | 168.17 KB |

It's hard to tell how much of that is from the parser and how much is from the PathGeometry but it doesn't look too bad on first glance.

[grokys]

Given that, should we merge this? Or did you want to take a look at the benchmark first?

[grokys]

One way to test how much memory is used by the parser might be:

1. Add an IGeometryContext interface with the same API as IStreamGeometryContext
2. Add a PathGeometryContext that implements it, and writes to a PathGeometry
3. Make the PathGeometryParser accept an IGeometryContext and use that to create the PathGeometry

This might be nice as it would also allow creation of StreamGeometrys via PathGeometryParser. This could be a separate PR though,

[Gillibald]

I did exactly the same as you did with the benchmark. Wasnt sure how to compare the results :) Your proposed additions make so much sense and I will implement them to get more flexibility. We even get more insides what the actual memory usage of the parser itself.

[Gillibald]

This is my current benchmark result:

BenchmarkDotNet=v0.10.8, OS=Windows 10.0.17134
Processor=Intel Core i7-4500U CPU 1.80GHz (Haswell), ProcessorCount=4
Frequency=2338342 Hz, Resolution=427.6534 ns, Timer=TSC
dotnet cli version=2.1.300
  [Host]     : .NET Core 4.6.26328.01, 64bit RyuJIT
  DefaultJob : .NET Core 4.6.26328.01, 64bit RyuJIT

Method	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
Parse_Large_Path	171.5 us	3.389 us	5.936 us	18.3105	2.4414	37.66 KB

[grokys]

Ok, that's not too bad. I think that maybe with the new Span<T> stuff we could probably parse a path with almost 0 allocations, but this is definitely an improvement on before, so let's merge!

[Gillibald]

.Net Core 2.1 will probably improve a lot

[jkoritzinsky]

I can go through and Span-ify in a new PR. The Span-ification of the PathMarkupParser will be blocked on the same bug that #1668 is blocked on.

[grokys]

Merged! Thanks @Gillibald !

[jkoritzinsky]

I started prototyping a Span-ified version. I've included the benchmark results below. The perf change is insane!

           Method |     Mean |    Error |   StdDev |  Gen 0 |  Gen 1 | Allocated |
----------------- |---------:|---------:|---------:|-------:|-------:|----------:|
 Parse_Large_Path | 40.55 us | 1.147 us | 3.309 us | 4.6997 | 1.0986 |   9.63 KB |

[Gillibald]

Looks promising👍The benefit is quit obvious.

[jkoritzinsky]

Result from my newest prototype:

BenchmarkDotNet=v0.10.14, OS=Windows 10.0.17134
Intel Core i5-6300U CPU 2.40GHz (Skylake), 1 CPU, 4 logical and 2 physical cores
.NET Core SDK=2.1.300
  [Host]     : .NET Core 2.0.7 (CoreCLR 4.6.26328.01, CoreFX 4.6.26403.03), 64bit RyuJIT
  DefaultJob : .NET Core 2.0.7 (CoreCLR 4.6.26328.01, CoreFX 4.6.26403.03), 64bit RyuJIT

Method	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
Parse_Large_Path	29.60 us	0.5703 us	0.5602 us	0.8850	0.2136	4.76 KB

I'll put out a PR with the Span-ified version in a bit.