Almost double the performance of parsing Guids by hughbe · Pull Request #3965 · dotnet/coreclr

hughbe · 2016-03-29T14:08:13Z

Remove substring and other allocations
Bring in a utility helper to avoid calling native code

Average improvement of -40% + for parsing each type of Guid.

Benchmark Results:

Time is measured in milliseconds
Formula for counting the change in time: (newTime - oldTime) / oldTime * 100

Benchmark Code:

Click Here

public static void TimeAction(string prefix, Action action)
{
    var sw = new Stopwatch();
    for (int iter = 0; iter < 5; iter++)
    {
        int gen0 = GC.CollectionCount(0);
        sw.Restart();
        for (int i = 0; i < 10000000; i++)
        {
            action();
        }
        sw.Stop();
        Console.WriteLine($"{prefix}Time: {sw.Elapsed.TotalSeconds}\tGC0: {GC.CollectionCount(0) - gen0}");
    }
}

static void Main(string[] args)
{
    Console.WriteLine("**** Measure \"00000000000000000000000000000000\" ****");
    TimeAction("Old: ", () => new Guid("a8a110d5fc4943c5bf46802db8f843ff"));
    TimeAction("New: ", () => new GuidNew("a8a110d5fc4943c5bf46802db8f843ff"));

    Console.WriteLine("**** Measure \"00000000-0000-0000-0000-000000000000\" ****");
    TimeAction("Old: ", () => new Guid("a8a110d5-fc49-43c5-bf46-802db8f843ff"));
    TimeAction("New: ", () => new GuidNew("a8a110d5-fc49-43c5-bf46-802db8f843ff"));

    Console.WriteLine("**** Measure (00000000-0000-0000-0000-000000000000) ****");
    TimeAction("Old: ", () => new Guid("(a8a110d5-fc49-43c5-bf46-802db8f843ff)"));
    TimeAction("New: ", () => new GuidNew("(a8a110d5-fc49-43c5-bf46-802db8f843ff)"));

    Console.WriteLine("**** Measure {00000000-0000-0000-0000-000000000000} ****");
    TimeAction("Old: ", () => new Guid("{a8a110d5-fc49-43c5-bf46-802db8f843ff}"));
    TimeAction("New: ", () => new GuidNew("{a8a110d5-fc49-43c5-bf46-802db8f843ff}"));

    Console.WriteLine("**** Measure {0x00000000,0x0000,0x0000,{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}} ****");
    TimeAction("Old: ", () => new Guid("{0xa8a110d5,0xfc49,0x43c5,{0xbf,0x46,0x80,0x2d,0xb8,0xf8,0x43,0xff}}"));
    TimeAction("New: ", () => new GuidNew("{0xa8a110d5,0xfc49,0x43c5,{0xbf,0x46,0x80,0x2d,0xb8,0xf8,0x43,0xff}}"));

    Console.ReadLine();
}

jakobbotsch · 2016-03-29T16:16:32Z

src/mscorlib/src/System/Guid.cs

-            else {
-                if ((flags & GuidStyles.RequireDashes) != 0) {
-                    // dashes are required
+                for (int i = end; i > start; i--)


This can be
for (int i = end - 1; i > start; i--)
instead, since guidString[end] is guaranteed to be '\0'.
However, I think it would be more elegant to write these two loops as

int start = 0; while (start < g.Length && char.IsWhitespace(guidString[start])) start++; int end = g.Length; while (end > start && char.IsWhitespace(guidString[end - 1])) end--;

Thanks, I pushed a commit adopting your more elegant code

mattwarren · 2016-03-30T09:21:04Z

@hughbe Just to back up your findings, here's a benchmark of

new Guid(0x12345678, 0xb, 0xc, 0, 1, 2, 3, 4, 5, 6, 7)
v
new Guid("{12345678-000b-000c-0001-020304050607}")

For reference, here is the full benchmark code (note that it's using BenchmarkDotNet)

ellismg · 2016-04-07T21:03:09Z

src/mscorlib/src/System/Guid.cs

+            length = newLength;
+            fixed (char* newGuidString = new string(chars, 0, newLength))
+            {
+                return newGuidString;


I believe this is unsafe, after the fixed block exits, there's nothing preventing the GC from relocating the managed object backing newGuidString and preventing the pointer from dangling.

ellismg · 2016-04-07T21:07:53Z

First, it's great to see the improvements here, so thanks a lot for starting the conversation. Looking over the diff, I suspect some of the wins come from being smarter about the sorts of allocations we do (I see a bunch of places where calls to .Substring have been removed).

I don't like how much unsafe code we end up having to add to the system. Seeing char* everywhere makes me nervous. Can we get a majority of the wins without having to use char* across most of these methods (e.g. could we continue to use the parse methods that take a String but provide offsets that we apply after we do the fixed in there?).

If we do need to introduce more places we use char*, It would also be ideal if we could scope unsafe usage to either small methods or limited blocks in larger methods, just to reduce the area of code we need to audit where we risk breaking type safety. I think this will help with the long term maintenance of the code.

hughbe · 2016-04-07T21:10:31Z

Thanks for the review. I recall that plenty of those char* were used because I think that indexing into a char* is quicker than indexing into a string. Could you let me know there is no change or in fact it's better to index into a string?
I'll take a look at places to remove unsafe code

hughbe · 2016-04-07T21:25:30Z

src/mscorlib/src/System/Guid.cs

                }
            }
-            catch(IndexOutOfRangeException ex) {
+            catch (IndexOutOfRangeException ex)


@ellismg Is catching this IndexOutOfRangeException and the ArgumentException below necessary, or actually hiding a bug that has the potential to happen?

hughbe · 2016-04-07T22:05:50Z

I've updated the PR just squashing the old commits (so thats easier to review) and shortening the diff for you.
I've added benchmarks too - looks like we're not suffering (maybe we're even benefitting) from removing unsafe code

hughbe · 2016-04-08T12:34:35Z

Just updated the PR to not allocate anything if a hex string contains whitespace - this more than doubled the perf of creating a hex guid

ranma42 · 2016-05-04T12:36:28Z

src/mscorlib/src/System/Guid.cs

-                {
-                    chArr[newLength++] = curChar;
-                }
+                char c = guidString[start];


Is this really needed? It looks like c is never used.

Use a bit of (safe) unsafe code to speed up code and remove allocations Bring in a utility helper to avoid calling native code Average improvement of 40% + for parsing each type of Guid

hughbe · 2016-09-26T18:03:32Z

Closing for now, fails corefx tests - will send in an updated PR soon

dnfclas added the cla-already-signed label Mar 29, 2016

hughbe changed the title ~~Considerably improve the performance of parsing Guids~~ Almost double the performance of parsing Guids Mar 29, 2016

jakobbotsch reviewed Mar 29, 2016
View reviewed changes

ellismg reviewed Apr 7, 2016
View reviewed changes

hughbe reviewed Apr 7, 2016
View reviewed changes

ranma42 reviewed May 4, 2016
View reviewed changes

Considerably improve the performance of parsing Guids

ba6d2ce

Use a bit of (safe) unsafe code to speed up code and remove allocations Bring in a utility helper to avoid calling native code Average improvement of 40% + for parsing each type of Guid

hughbe mentioned this pull request Sep 6, 2016

Add tests for Guid.Parse dotnet/corefx#11448

Merged

hughbe closed this Sep 26, 2016

hughbe deleted the guid-performance branch September 26, 2016 18:03

hughbe mentioned this pull request Jan 31, 2020

Parse integers in a string based on an index dotnet/runtime#16933

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Almost double the performance of parsing Guids#3965

Almost double the performance of parsing Guids#3965
hughbe wants to merge 1 commit intodotnet:masterfrom
hughbe:guid-performance

hughbe commented Mar 29, 2016

Uh oh!

jakobbotsch Mar 29, 2016

Uh oh!

hughbe Mar 30, 2016

Uh oh!

mattwarren commented Mar 30, 2016

Uh oh!

ellismg Apr 7, 2016

Uh oh!

ellismg commented Apr 7, 2016

Uh oh!

hughbe commented Apr 7, 2016

Uh oh!

hughbe Apr 7, 2016

Uh oh!

hughbe commented Apr 7, 2016

Uh oh!

hughbe commented Apr 8, 2016

Uh oh!

ranma42 May 4, 2016

Uh oh!

hughbe commented Sep 26, 2016 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

hughbe commented Mar 29, 2016

Benchmark Results:

Benchmark Code:

Uh oh!

jakobbotsch Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

hughbe Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

mattwarren commented Mar 30, 2016

Uh oh!

ellismg Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

ellismg commented Apr 7, 2016

Uh oh!

hughbe commented Apr 7, 2016

Uh oh!

hughbe Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

hughbe commented Apr 7, 2016

Uh oh!

hughbe commented Apr 8, 2016

Uh oh!

ranma42 May 4, 2016

Choose a reason for hiding this comment

Uh oh!

hughbe commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hughbe commented Sep 26, 2016 •

edited

Loading