Merge pull request #20 from MichalStrehovsky/Flags

Move thread safe flag manipulation into a helper
dotnet · Oct 2, 2015 · f1a39a7 · f1a39a7
2 parents 648dff6 + 6745182
commit f1a39a7
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 43 deletions.
diff --git a/src/ILToNative/src/ILToNative.csproj b/src/ILToNative/src/ILToNative.csproj
@@ -93,6 +93,9 @@
     <Compile Include="..\..\TypeSystem\src\Common\TargetDetails.cs">
       <Link>TypeSystem\TargetDetails.cs</Link>
     </Compile>
+    <Compile Include="..\..\TypeSystem\src\Common\ThreadSafeFlags.cs">
+      <Link>TypeSystem\ThreadSafeFlags.cs</Link>
+    </Compile>
     <Compile Include="..\..\TypeSystem\src\Common\TypeCast.cs">
       <Link>TypeSystem\TypeCast.cs</Link>
     </Compile>

diff --git a/src/TypeSystem/src/Common/FieldLayout.cs b/src/TypeSystem/src/Common/FieldLayout.cs
@@ -4,7 +4,6 @@
 using System;
 using System.Linq;
 using Debug = System.Diagnostics.Debug;
-using Interlocked = System.Threading.Interlocked;
 
 namespace Internal.TypeSystem
 {
@@ -458,7 +457,7 @@ private class StaticBlockInfo
             public StaticsBlock ThreadStatics;
         }
 
-        volatile int _fieldLayoutFlags;
+        ThreadSafeFlags _fieldLayoutFlags;
 
         int _instanceFieldSize;
         int _instanceByteCount;
@@ -471,24 +470,19 @@ public bool ContainsPointers
         {
             get
             {
-                if ((_fieldLayoutFlags & FieldLayoutFlags.HasContainsPointers) == 0)
+                if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.HasContainsPointers))
                 {
-                    var flagsToAdd = FieldLayoutFlags.HasContainsPointers;
-
-                    if (ComputeTypeContainsPointers())
-                        flagsToAdd |= FieldLayoutFlags.ContainsPointers;
-
-                    EnableFieldLayoutFlags(flagsToAdd);
+                    ComputeTypeContainsPointers();
                 }
-                return (_fieldLayoutFlags & FieldLayoutFlags.ContainsPointers) != 0;
+                return _fieldLayoutFlags.HasFlags(FieldLayoutFlags.ContainsPointers);
             }
         }
 
         public int InstanceFieldSize
         {
             get
             {
-                if ((_fieldLayoutFlags & FieldLayoutFlags.HasInstanceFieldLayout) == 0)
+                if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.HasInstanceFieldLayout))
                 {
                     ComputeInstanceFieldLayout();
                 }
@@ -500,7 +494,7 @@ public int InstanceFieldAlignment
         {
             get
             {
-                if ((_fieldLayoutFlags & FieldLayoutFlags.HasInstanceFieldLayout) == 0)
+                if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.HasInstanceFieldLayout))
                 {
                     ComputeInstanceFieldLayout();
                 }
@@ -512,7 +506,7 @@ public int InstanceByteCount
         {
             get
             {
-                if ((_fieldLayoutFlags & FieldLayoutFlags.HasInstanceFieldLayout) == 0)
+                if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.HasInstanceFieldLayout))
                 {
                     ComputeInstanceFieldLayout();
                 }
@@ -524,7 +518,7 @@ public int NonGCStaticFieldSize
         {
             get
             {
-                if ((_fieldLayoutFlags & FieldLayoutFlags.HasStaticFieldLayout) == 0)
+                if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.HasStaticFieldLayout))
                 {
                     ComputeStaticFieldLayout();
                 }
@@ -536,7 +530,7 @@ public int NonGCStaticFieldAlignment
         {
             get
             {
-                if ((_fieldLayoutFlags & FieldLayoutFlags.HasStaticFieldLayout) == 0)
+                if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.HasStaticFieldLayout))
                 {
                     ComputeStaticFieldLayout();
                 }
@@ -558,7 +552,7 @@ internal void ComputeInstanceFieldLayout()
                 fieldAndOffset.Field.InitializeOffset(fieldAndOffset.Offset);
             }
 
-            EnableFieldLayoutFlags(FieldLayoutFlags.HasInstanceFieldLayout);
+            _fieldLayoutFlags.AddFlags(FieldLayoutFlags.HasInstanceFieldLayout);
         }
 
         internal void ComputeStaticFieldLayout()
@@ -584,13 +578,18 @@ internal void ComputeStaticFieldLayout()
                 }
             }
 
-            EnableFieldLayoutFlags(FieldLayoutFlags.HasStaticFieldLayout);
+            _fieldLayoutFlags.AddFlags(FieldLayoutFlags.HasStaticFieldLayout);
         }
 
-        private bool ComputeTypeContainsPointers()
+        private void ComputeTypeContainsPointers()
         {
+            int flagsToAdd = FieldLayoutFlags.HasContainsPointers;
+
             if (!IsValueType && HasBaseType && BaseType.ContainsPointers)
-                return true;
+            {
+                _fieldLayoutFlags.AddFlags(flagsToAdd | FieldLayoutFlags.ContainsPointers);
+                return;
+            }
 
             foreach (var field in GetFields())
             {
@@ -606,24 +605,19 @@ private bool ComputeTypeContainsPointers()
                     // case TypeFlags.MethodGenericParameter?
                     case TypeFlags.GenericParameter:
                     case TypeFlags.ByRef:
-                        return true;
+                        _fieldLayoutFlags.AddFlags(flagsToAdd | FieldLayoutFlags.ContainsPointers);
+                        return;
                     case TypeFlags.ValueType:
                         if (((MetadataType)fieldType).ContainsPointers)
-                            return true;
+                        {
+                            _fieldLayoutFlags.AddFlags(flagsToAdd | FieldLayoutFlags.ContainsPointers);
+                            return;
+                        }
                         break;
                 }
             }
 
-            return false;
-        }
-
-        private void EnableFieldLayoutFlags(int flagsToAdd)
-        {
-            var originalFlags = _fieldLayoutFlags;
-            while (Interlocked.CompareExchange(ref _fieldLayoutFlags, originalFlags | flagsToAdd, originalFlags) != originalFlags)
-            {
-                originalFlags = _fieldLayoutFlags;
-            }
+            _fieldLayoutFlags.AddFlags(flagsToAdd);
         }
     }
 

diff --git a/src/TypeSystem/src/Common/ThreadSafeFlags.cs b/src/TypeSystem/src/Common/ThreadSafeFlags.cs
@@ -0,0 +1,38 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Runtime.CompilerServices;
+using Interlocked = System.Threading.Interlocked;
+
+namespace Internal.TypeSystem
+{
+    struct ThreadSafeFlags
+    {
+        private volatile int _value;
+
+        public int Value
+        {
+            [MethodImpl(MethodImplOptions.AggressiveInlining)]
+            get
+            {
+                return _value;
+            }
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public bool HasFlags(int value)
+        {
+            return (_value & value) == value;
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void AddFlags(int flagsToAdd)
+        {
+            var originalFlags = _value;
+            while (Interlocked.CompareExchange(ref _value, originalFlags | flagsToAdd, originalFlags) != originalFlags)
+            {
+                originalFlags = _value;
+            }
+        }
+    }
+}
diff --git a/src/TypeSystem/src/Ecma/EcmaField.cs b/src/TypeSystem/src/Ecma/EcmaField.cs
@@ -9,8 +9,6 @@
 
 using Internal.TypeSystem;
 
-using Interlocked = System.Threading.Interlocked;
-
 namespace Internal.TypeSystem.Ecma
 {
     public sealed class EcmaField : FieldDesc
@@ -31,7 +29,7 @@ static class FieldFlags
         FieldDefinitionHandle _handle;
 
         // Cached values
-        volatile int _fieldFlags;
+        ThreadSafeFlags _fieldFlags;
         TypeDesc _fieldType;
         string _name;
 
@@ -152,22 +150,15 @@ private int InitializeFieldFlags(int mask)
 
             Debug.Assert((flags & mask) != 0);
 
-            // Atomically update flags
-            var originalFlags = _fieldFlags;
-            while (Interlocked.CompareExchange(ref _fieldFlags, (int)(originalFlags | flags), originalFlags) != originalFlags)
-            {
-                originalFlags = _fieldFlags;
-            }
-
-            _fieldFlags |= flags;
+            _fieldFlags.AddFlags(flags);
 
             return flags & mask;
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private int GetFieldFlags(int mask)
         {
-            int flags = _fieldFlags & mask;
+            int flags = _fieldFlags.Value & mask;
             if (flags != 0)
                 return flags;
             return InitializeFieldFlags(mask);

diff --git a/src/TypeSystem/src/TypeSystem.csproj b/src/TypeSystem/src/TypeSystem.csproj
@@ -44,6 +44,7 @@
     <Compile Include="Common\PointerType.cs" />
     <Compile Include="Common\SignatureVariable.cs" />
     <Compile Include="Common\TargetDetails.cs" />
+    <Compile Include="Common\ThreadSafeFlags.cs" />
     <Compile Include="Common\TypeCast.cs" />
     <Compile Include="Common\TypeFlags.cs" />
     <Compile Include="Common\TypeHashingAlgorithms.cs" />