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

Span-ify PathMarkupParser #1685

Merged
merged 12 commits into from Jun 26, 2018

Conversation

Projects
None yet
5 participants
@jkoritzinsky
Member

jkoritzinsky commented Jun 21, 2018

  • What does the pull request do?
    Change PathMarkupParser to use ReadOnlySpan<char>s instead of regular expressions to parse paths.
  • What is the current behavior?
    Currently the parser uses regular expressions and data-structures on the heap to parse paths.
  • What is the updated/expected behavior with this PR?
    Better performance.

Old perf (from #1661):

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

New perf:

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

So, this PR uses approximately 13% of the managed memory that the current implementation uses in our benchmark.

@Gillibald

This comment has been minimized.

Contributor

Gillibald commented Jun 21, 2018

This looks so much better. A very elegant solution.

@grokys

This comment has been minimized.

Member

grokys commented Jun 21, 2018

Really nice @jkoritzinsky !

@KvanTTT

This comment has been minimized.

Contributor

KvanTTT commented Jun 23, 2018

Is it properly to measure performance on different processors? I see Intel Core i7-4500U CPU 1.80GHz (Haswell), ProcessorCount=4 in old perf and Intel Core i5-6300U CPU 2.40GHz (Skylake), 1 CPU, 4 logical and 2 physical cores in the new one.

@Gillibald

This comment has been minimized.

Contributor

Gillibald commented Jun 23, 2018

This is mainly about memory allocation so it shouldn't matter.

@grokys

This comment has been minimized.

Member

grokys commented Jun 25, 2018

@jkoritzinsky is this no longer blocked on dependency?

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Jun 25, 2018

I don't believe so. The android project build would be failing if it was blocked.

@grokys

grokys approved these changes Jun 26, 2018

@grokys grokys merged commit 8b8ba12 into AvaloniaUI:master Jun 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jkoritzinsky jkoritzinsky deleted the jkoritzinsky:spanify-PathMarkupParser branch Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment