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

csharp: sync v1.1.0 from John-Paul-R/FlexVer #19

Merged
merged 8 commits into from
Jul 23, 2023

Conversation

John-Paul-R
Copy link
Collaborator

This PR deals with some build/packaging details, and reworks the comparer implementation to be faster and make no heap allocations.

Benchmark results, using the version strings in the test harness:

Method Mean Error StdDev Gen0 Allocated
CompareAll_New 6.248 us 0.1228 us 0.2245 us - -
CompareAll_Existing 30.432 us 0.6077 us 1.1112 us 0.3967 33672 B

NuGet listing from these build changes: https://www.nuget.org/packages/FlexVer


  • I own the full rights to my contribution, and agree to release it under the terms of the Creative Commons Zero Public Domain Dedication

John-Paul-R and others added 4 commits July 22, 2023 13:20
* Fixup packaging

* add separate readme to get ride of plaintext-rendered img tag
* v1

* fully non-heap-allocating

|                            Method |      Mean |     Error |    StdDev |   Gen0 | Allocated |
|---------------------------------- |----------:|----------:|----------:|-------:|----------:|
| CompareAll_AV2_StackAlloc_ToArray |  4.066 us | 0.0598 us | 0.0559 us |      - |         - |
|               CompareAll_Existing | 29.137 us | 0.5789 us | 0.6434 us | 0.3967 |   33672 B |

* merge into main FlexVerComparer

* add benchmark harness

* fixup comment, formatting

* todo

* make GetNextVersionComponent a bit more decipherable

* less-allocating VersionComponent ToString, why not

* update dotsettings for limited code style

* fixes/simplifications based on tests/spec

* consts for char literals

* use `ushort` instead of `int` for numeric bits, since `char` is 2-bytes

* minor changes from perf test
* Switch to central build/package management

* don't pack benchmarks
@John-Paul-R John-Paul-R changed the title csharp: sync from John-Paul-R/FlexVer csharp: sync v1.1.0 from John-Paul-R/FlexVer Jul 22, 2023
Copy link
Owner

@unascribed unascribed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing now that this implementation never followed the spec — it uses UTF-16 code units instead of Unicode codepoints. I didn't notice due to my lack of familiarity with C#. Please switch back to ints and use something like EnumerateRunes.

csharp/FlexVerBenchmarkHarness/Program.cs Outdated Show resolved Hide resolved
csharp/FlexVerBenchmarkHarness/Program.cs Outdated Show resolved Hide resolved
csharp/FlexVer/FlexVerComparer.cs Outdated Show resolved Hide resolved
* Use `Rune`s (Unicode codepoint) instead of incorrectly using UTF-16 units

* Read benchmark test cases from `test/test_vectors.txt` instead of string literal
instead of incorrectly incrementing based on IsHighSurrogate
Copy link
Owner

@unascribed unascribed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@unascribed unascribed merged commit e718a28 into unascribed:trunk Jul 23, 2023
1 check passed
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.

None yet

2 participants