Remove libuuid usage #16643

janvorli · 2018-02-28T00:57:10Z

This change removes dependency on the libuuid library that is used for GUID creation only.
It implements it using a random generator instead.

It also modifies return type of PAL_Random to VOID since it was always
returning TRUE and none of the existing callers were checking it.

Close #16642

This change removes dependency on the libuuid library that is used for GUID creation only. It implements it using a random generator instead. It also modifies return type of PAL_Random to VOID since it was always returning TRUE and none of the existing callers were checking it.

janvorli · 2018-02-28T00:57:30Z

CC: @Petermarcu

jkotas · 2018-02-28T02:14:06Z

src/dlls/mscordac/mscordac_unixexports.src

 PAL_TerminateEx
 PAL_IsDebuggerPresent
 PAL_ProbeMemory
+PAL_Random


Why do we need to export PAL_Random here?

Because the CoCreateGuid depends on it and it is now in palrt. I've moved CoCreateGuid to palrt.

To be more precise mscordbi depends on symbols exported by mscordac and mscordbi uses CoCreateGuid, so it needs access to the underlying PAL_Random.

jkotas · 2018-02-28T02:22:35Z

src/dlls/mscoree/mscorwks_unixexports.src


 ; Win32 API and other PAL functions used by the mscorlib
 CloseHandle
-CoCreateGuid


CoreLib is still PInvoking this one. The CI tests are failing on Unix because of it. I think we should:

Add https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Guid.Unix.cs and https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Guid.Windows.cs to shared CoreLib partition

Change Guid.Unix.cs to have this RFC algorithm

Delete https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Guid.CoreCLR.cs

That's a good idea, I will do it.

stephentoub · 2018-02-28T06:48:29Z

src/palrt/guid.cpp

+    // time_hi_and_version
+    pguid->Data3 = (pguid->Data3 & ~VersionMask) | RandomGuidVersion;
+    // clock_seq_hi_and_reserved
+    pguid->Data4[0] = (pguid->Data4[0] & ~ClockSeqHiAndReservedMask) | ClockSeqHiAndReservedValue;


At one point we had an issue related to guid perf:
https://github.com/dotnet/corefx/issues/3573
Have you looked to see how this compares to the previous implementation?

Good point, I've completely forgotten that we were using PAL_Random and had that perf issue in the past. I'll make the change recommended by @jkotas and try to make sure the perf doesn't degrade.

1. Port the GUID creation to managed code. 2. Modify the PAL_Random to have 6 times better perf so that the perf of the CoCreateGuid that is used in the native runtime doesn't degrade that much w.r.t the previous state when the libuuid was used.

janvorli · 2018-03-01T00:53:50Z

@jkotas, @stephentoub I've added the managed implementation and also improved the performance of the native implementation (by improving the PAL_random performance 6 times). The largest part of the improvement was achieved by reading all the bytes at once instead of byte by byte. A smaller part by dropping the usage of /dev/random and using only /dev/urandom. It seems there was a myth about the /dev/random being "better" random, but I've found a document that demystifies that and also a recent change to the man page of the /dev/random and /dev/urandom that made it clearer that there is nothing wrong with just using the /dev/urandom. See https://www.2uo.de/myths-about-urandom/ if you are interested.

jkotas · 2018-03-01T00:59:18Z

src/mscorlib/shared/System/Guid.Unix.cs

+{
+    partial struct Guid
+    {
+        static Random _random = new Random();


I do not think that Random is good enough for GUID generation. It needs to go to PAL Random, like this: https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Guid.Unix.cs

jkotas · 2018-03-01T01:01:17Z

src/mscorlib/shared/System/Guid.Windows.cs

+            // how extensively it checks for known values.
+
+            Guid g;
+            int hr = Interop.mincore.CoCreateGuid(out g);


mincore should be renamed to Ole32 to follow the current naming conventions. (CoreRT is using the obsolete naming convention for this. Was not cleaned up yet.)

jkotas · 2018-03-01T01:03:18Z

src/pal/src/misc/miscpalapi.cpp

        else
        {
-            for( ; i < dwLength; i++)
+            size_t index = 0;


We have copy of this for the corelib/framework use here: https://github.com/dotnet/corefx/blob/master/src/Native/Unix/System.Native/pal_random.cpp

Could you please update that one as well?

Created PR for the change: dotnet/corefx#27601

danmoseley · 2018-03-01T06:28:33Z

src/mscorlib/shared/System/Guid.Windows.cs

+        // we need a method that allows users to create a guid.
+        public static Guid NewGuid()
+        {
+            // CoCreateGuid should never return Guid.Empty, since it attempts to maintain some


I looked at CoCreateGuid and I do not see any place it checks for known GUIDs.

This file was copied from CoreRT. That part of the comment isn't supported by anything in the MSDN doc either. I'll remove it.

Also do a little cleanup

By keeping the file descriptor for /dev/urandom open permanently, another 2x perf improvement was achieved. The total perf improvement measured on my physical machine is now 13.75x.

jkotas · 2018-03-01T15:28:34Z

src/mscorlib/shared/System/Guid.Windows.cs


            Guid g;
-            int hr = Interop.mincore.CoCreateGuid(out g);
+            int hr = Microsoft.Win32.Win32Native.CoCreateGuid(out g);


This will break CoreRT. Could you please copy over https://github.com/dotnet/corert/blob/master/src/Common/src/Interop/Windows/mincore/Interop.CoCreateGuid.cs, rename it Ole32, and delete Microsoft.Win32.Win32Native.CoCreateGuid ?

stephentoub · 2018-03-01T15:29:01Z

src/mscorlib/shared/System/Guid.Unix.cs

+{
+    partial struct Guid
+    {
+        // This will create a new guid.  Since we've now decided that constructors should 0-init,


Since we've now decided that constructors should 0-init

Is this comment from, like, 1999? 😄

No idea :-) Came from CoreRT as well

jkotas · 2018-03-01T15:29:57Z

src/mscorlib/shared/System/Guid.Unix.cs

-            Guid g = new Guid(randomData);
+            const int GuidSize = 16;
+            byte* randomData = stackalloc byte[GuidSize];
+            Interop.GetRandomBytes(randomData, GuidSize);


I think it would be better to do:

Guid g; Interop.GetRandomBytes(&g, sizeof(Guid));

stephentoub · 2018-03-01T15:45:14Z

src/pal/src/misc/miscpalapi.cpp

+    // in case /dev/urandom is not really random

-    for(i = 0; i < dwLength; i++)
+    for (i = 0; i < dwLength; i++)


Can someone help me understand why this loop with xor'ing with mrand48 is necessary? My understanding is that /dev/urandom will produce as much cryptographically-strong random data as it can and then fall back to pseudo-random data. How does then mixing it with other prng data from mrand48 help?

I don't think it would help. In fact, looking at the man page, it is not thread safe. The main reason why it was here was to work as a fallback in case the /dev/urandom is not present or cannot be opened for some reason.
I guess we can just say that the /dev/urandom is required to be present and if opening it fails, just abort the process. Then we can remove this loop.

jkotas · 2018-03-01T16:53:07Z

What is the performance of NewGuid with all these changes, compared to Windows and uuid implementation?

janvorli · 2018-03-01T16:58:48Z

It is 35% faster than the previous uuid version. It is still 8 times slower than on Windows (I've two identical machines, one running Linux and one Windows).

jkotas · 2018-03-01T17:56:41Z

src/mscorlib/shared/System/Guid.Windows.cs

+{
+    partial struct Guid
+    {
+        // This will create a new guid.  Since we've now decided that constructors should 0-init,


The comment can be deleted here as well.

jkotas · 2018-03-01T17:57:10Z

src/mscorlib/shared/System/Guid.Unix.cs

+        // This will create a new random guid based on the https://www.ietf.org/rfc/rfc4122.txt 
+        public static unsafe Guid NewGuid()
+        {
+            Guid g = new Guid();


Nit: I do not think you need = new Guid() part

jkotas · 2018-03-01T17:58:14Z

src/pal/src/misc/miscpalapi.cpp

+            do
            {
-                ASSERT("PAL__open() failed, errno:%d (%s)\n", errno, strerror(errno));
+                fd = open("/dev/urandom", O_RDONLY, O_CLOEXEC);


I do not think PAL_Random is used for anything perf critical in the runtime itself. I do not think that the caching is worth it in this version.

Isn't any CoCreateGuid call in the runtime perf sensitive? I really cannot tell in some of the cases.
And is it worth having a slightly different implementation in corefx and coreclr just to save one open handle? I've answered your question on the count of open handles in simple hello world app - it is 21 without my change.

If CoCreateGuid is used for anything perf sensitive, then the 8x perf difference between Windows and Unix is still quite a bit of a problem.

The same actually true for the framework side of the implementation. 8x is still better than 16x compared to Windows, but you probably do not want to be calling it in too often in any case.

I guess if we wanted to make this better, we would need to cache (read say 256 bytes at a time) or have a local random number generator (with larger seed than the 32-bits of built-in one).

jkotas · 2018-03-01T20:08:53Z

LGTM modulo a few nits

And fixing a little nits.

janvorli · 2018-03-01T23:53:15Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

janvorli · 2018-03-01T23:53:30Z

@dotnet-bot test Windows_NT arm Cross Checked Innerloop Build and Test

janvorli · 2018-03-01T23:53:47Z

@dotnet-bot Windows_NT arm64 Cross Checked Innerloop Build and Test

janvorli · 2018-03-01T23:54:03Z

@dotnet-bot Windows_NT arm64 Cross Checked Innerloop Build and Test

janvorli · 2018-03-01T23:54:25Z

@dotnet-bot Windows_NT armlb Cross Checked Innerloop Build and Test

janvorli · 2018-03-01T23:56:09Z

@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test

janvorli · 2018-03-01T23:56:38Z

@dotnet-bot test Windows_NT armlb Cross Checked Innerloop Build and Test

janvorli · 2018-03-02T02:06:15Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

janvorli · 2018-03-02T09:06:33Z

@dotnet-bot test Windows_NT armlb Cross Checked Innerloop Build and Test

janvorli · 2018-03-02T09:06:41Z

@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test

janvorli · 2018-03-02T09:07:00Z

@dotnet-bot test Windows_NT arm Cross Checked Innerloop Build and Test

janvorli · 2018-03-02T19:00:41Z

@dotnet-bot test Windows_NT arm Cross Checked Innerloop Build and Test

janvorli · 2018-03-05T10:09:35Z

@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test

janvorli · 2018-03-05T10:09:56Z

@dotnet-bot test Windows_NT armlb Cross Checked Innerloop Build and Test

janvorli · 2018-03-05T10:10:09Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

* Remove libuuid usage This change removes dependency on the libuuid library that is used for GUID creation only. It implements it using a random generator instead. It also modifies return type of PAL_Random to VOID since it was always returning TRUE and none of the existing callers were checking it. 1. Port the GUID creation to managed code. 2. Modify the PAL_Random to have 6 times better perf so that the perf of the CoCreateGuid that is used in the native runtime doesn't degrade that much w.r.t the previous state when the libuuid was used. 3. Use Interop.GetRandomBytes on Unix and fix Windows Signed-off-by: dotnet-bot <[email protected]>

* Remove libuuid usage This change removes dependency on the libuuid library that is used for GUID creation only. It implements it using a random generator instead. It also modifies return type of PAL_Random to VOID since it was always returning TRUE and none of the existing callers were checking it. 1. Port the GUID creation to managed code. 2. Modify the PAL_Random to have 6 times better perf so that the perf of the CoCreateGuid that is used in the native runtime doesn't degrade that much w.r.t the previous state when the libuuid was used. 3. Use Interop.GetRandomBytes on Unix and fix Windows Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>

* Remove libuuid usage This change removes dependency on the libuuid library that is used for GUID creation only. It implements it using a random generator instead. It also modifies return type of PAL_Random to VOID since it was always returning TRUE and none of the existing callers were checking it. 1. Port the GUID creation to managed code. 2. Modify the PAL_Random to have 6 times better perf so that the perf of the CoCreateGuid that is used in the native runtime doesn't degrade that much w.r.t the previous state when the libuuid was used. 3. Use Interop.GetRandomBytes on Unix and fix Windows Signed-off-by: dotnet-bot <[email protected]>

Related to dotnet/coreclr#16643

* Remove libuuid usage This change removes dependency on the libuuid library that is used for GUID creation only. It implements it using a random generator instead. It also modifies return type of PAL_Random to VOID since it was always returning TRUE and none of the existing callers were checking it. 1. Port the GUID creation to managed code. 2. Modify the PAL_Random to have 6 times better perf so that the perf of the CoCreateGuid that is used in the native runtime doesn't degrade that much w.r.t the previous state when the libuuid was used. 3. Use Interop.GetRandomBytes on Unix and fix Windows Signed-off-by: dotnet-bot <[email protected]>

Related to dotnet/coreclr#16643

…NET Core <2.1 when running on BSD would use v1 (MAC-address-based) GUIDs. Fixed [here](dotnet/coreclr#16643). https://blog.stephencleary.com/2010/11/few-words-on-guids.html#comment-2ff12cb0-dac5-11ec-af31-8b5ce13aabba

janvorli added the area-PAL label Feb 28, 2018

janvorli added this to the 2.1.0 milestone Feb 28, 2018

janvorli self-assigned this Feb 28, 2018

janvorli requested a review from adityamandaleeka February 28, 2018 00:57

jkotas reviewed Feb 28, 2018

View reviewed changes

stephentoub reviewed Feb 28, 2018

View reviewed changes

Reflect PR feedback

172eb0d

1. Port the GUID creation to managed code. 2. Modify the PAL_Random to have 6 times better perf so that the perf of the CoCreateGuid that is used in the native runtime doesn't degrade that much w.r.t the previous state when the libuuid was used.

jkotas reviewed Mar 1, 2018

View reviewed changes

danmoseley reviewed Mar 1, 2018

View reviewed changes

janvorli added 3 commits March 1, 2018 11:11

Use Interop.GetRandomBytes on Unix and fix Windows

33be069

Remove alloca and generate right to the lpBuffer

b5c698a

Also do a little cleanup

Further 2x perf improvement

f623c66

By keeping the file descriptor for /dev/urandom open permanently, another 2x perf improvement was achieved. The total perf improvement measured on my physical machine is now 13.75x.

janvorli force-pushed the remove-libuuid-usage branch from 847257a to f623c66 Compare March 1, 2018 13:23

jkotas reviewed Mar 1, 2018

View reviewed changes

stephentoub reviewed Mar 1, 2018

View reviewed changes

jkotas reviewed Mar 1, 2018

View reviewed changes

stephentoub reviewed Mar 1, 2018

View reviewed changes

Reflect PR feedback

2b5cb4a

Delete forgotten close to the rand_res

be5426e

jkotas reviewed Mar 1, 2018

View reviewed changes

Remove the random FD caching

3b9f14d

And fixing a little nits.

janvorli merged commit 76245a3 into dotnet:master Mar 6, 2018

janvorli deleted the remove-libuuid-usage branch March 6, 2018 01:21

jkotas mentioned this pull request Mar 8, 2018

Remove libuuid usage dotnet/corert#5521

Merged

jkotas added a commit to dotnet/corert that referenced this pull request Mar 8, 2018

Remove libuuid usage (#5521)

842f475

Related to dotnet/coreclr#16643

kbaladurin pushed a commit to kbaladurin/corert that referenced this pull request Mar 15, 2018

Remove libuuid usage (dotnet#5521)

77302ec

Related to dotnet/coreclr#16643

Remove libuuid usage #16643

Remove libuuid usage #16643

Uh oh!

Conversation

janvorli commented Feb 28, 2018

Uh oh!

janvorli commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janvorli commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Mar 1, 2018

Uh oh!

janvorli commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Mar 1, 2018

Uh oh!

janvorli commented Mar 1, 2018

Uh oh!

jkotas Feb 28, 2018 •

edited

Loading

jkotas Mar 1, 2018 •

edited

Loading

jkotas Mar 1, 2018 •

edited

Loading

jkotas Mar 1, 2018 •

edited

Loading