Remove unnecessary volatile from fields in Libraries#125274
Open
stephentoub wants to merge 1 commit intodotnet:mainfrom
Open
Remove unnecessary volatile from fields in Libraries#125274stephentoub wants to merge 1 commit intodotnet:mainfrom
stephentoub wants to merge 1 commit intodotnet:mainfrom
Conversation
Per the .NET memory model (docs/design/specs/Memory-model.md), many volatile field annotations in Libraries are unnecessary and impose overhead without benefit. This removes volatile from 187 fields across 99 files. **Reference-type fields (159 fields, 86 files):** Object assignment to a shared location is a release with respect to the instance's fields (line 135), and data-dependent reads are ordered (line 118). This means lazy-init patterns like `??=` and sentinel-check-then-assign on reference-type fields do not need volatile: the publishing write ensures constructor side-effects are visible, and readers accessing the object through the reference see those effects. The memory model doc explicitly shows singleton patterns working without volatile (lines 217-277). Fields protected by locks or Interlocked operations also have volatile removed since those primitives provide stronger ordering guarantees. **Value-type fields (28 fields, 13 files):** The memory model now prohibits read-introduction: 'Reads cannot be introduced' (line 50). Previously, volatile was needed on value-type lazy-init fields to prevent the JIT from replacing a local variable with a re-read of the field (TOCTOU risk). With read-introduction now prohibited, volatile is unnecessary on value-type fields where: - The field is read once into a local and the local is used - The value IS the result (no associated shared state needs acquire semantics) - The field is write-once with a sentinel (no anti-coalescing concern) This includes IntPtr handle caches (crypto digest algorithms), sbyte tri-state flags, and int sentinel-based lazy-init fields like s_processId, s_systemPageSize, and s_osArchPlusOne. **Fields deliberately kept volatile:** - Value-type fields that gate access to other shared mutable state (e.g. _hasCachedAlgorithmGroup gates _cachedAlgorithmGroup) - Boolean disposal/initialization flags read across code paths - Mutable fields with multiple writers (not write-once) - All fields in concurrent data structures and threading primitives - All Volatile.Read/Write call sites (deliberate per-access annotations) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @dotnet/area-meta |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes volatile from a large set of lazily-initialized and cache fields across Libraries, based on updated guidance in docs/design/specs/Memory-model.md, with the goal of reducing unnecessary barriers/overhead while preserving correctness.
Changes:
- Remove
volatilefrom many reference-type lazy caches that rely on safe publication via object assignment / locks / interlocked operations. - Remove
volatilefrom select value-type sentinel caches (e.g.,int,sbyte,IntPtr) where read-introduction concerns are no longer applicable per the memory model. - Keep
volatilewhere fields gate access to other mutable state, have multiple writers, or participate in concurrency primitives (per PR description).
Reviewed changes
Copilot reviewed 95 out of 95 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/UnicodeCategoryConditions.cs | Removes volatile from lazily initialized regex BDD caches. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs | Removes volatile from lazy chain status cache. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs | Removes volatile from multiple lazy certificate caches. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Certificate.cs | Removes volatile from multiple lazy certificate caches (keeps some volatile flags). |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X500DistinguishedName.cs | Removes volatile from lazily computed distinguished name string. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CryptoConfig.cs | Removes volatile from static dictionaries populated lazily. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CngKey.StandardProperties.cs | Removes volatile from cached key properties while retaining volatile “has cached” flags. |
| src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/KeyTransRecipientInfo.cs | Removes volatile from lazy decoded PKCS recipient fields. |
| src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/KeyAgreeRecipientInfo.cs | Removes volatile from multiple lazy decoded PKCS fields. |
| src/libraries/System.Security.AccessControl/src/System/Security/AccessControl/Privilege.cs | Removes volatile from cached process token handle. |
| src/libraries/System.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/Formatters/Binary/Converter.cs | Removes volatile from lazily initialized lookup arrays. |
| src/libraries/System.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryParser.cs | Removes volatile from cached MessageEnd instance. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoType.cs | Removes volatile from multiple type-loading lazy caches. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoGenericParameterType.cs | Removes volatile from generic parameter lazy caches. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoFunctionPointerType.cs | Removes volatile from cached ToString representation. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/Ecma/EcmaGenericMethodParameterType.cs | Removes volatile from lazy declaring-method cache. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/Ecma/EcmaDefinitionType.cs | Removes volatile from generic-parameter cache. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Properties/RoProperty.cs | Removes volatile from multiple lazy property caches. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Modules/Ecma/EcmaModule.MetadataTables.cs | Removes volatile from lazy metadata table cache. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Methods/RoMethod.cs | Removes volatile from lazy generic-arguments cache. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/MethodBase/RoMethodBody.cs | Removes volatile from lazy IL byte[] cache. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Fields/RoField.cs | Removes volatile from lazy field-type caches. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Events/RoEvent.cs | Removes volatile from lazy event accessor caches. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/CustomAttributes/RoPseudoCustomAttributeData.cs | Removes volatile from lazy custom-attribute argument lists. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/CustomAttributes/Ecma/EcmaCustomAttributeData.cs | Removes volatile from lazy custom-attribute argument lists. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Assemblies/RoAssembly.cs | Removes volatile from lazy assembly metadata caches. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/MetadataLoadContext.KnownConstructors.cs | Removes volatile from lazy known-constructor caches. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/MetadataLoadContext.CoreAssembly.cs | Removes volatile from lazy binder cache. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Internal/NamespaceCache.cs | Removes volatile from lazy namespace table cache. |
| src/libraries/System.Private.Xml/src/System/Xml/Xsl/IlGen/XmlILTrace.cs | Removes volatile from lazy trace directory cache. |
| src/libraries/System.Private.Xml/src/System/Xml/Xsl/IlGen/XmlILConstructAnalyzer.cs | Removes volatile from lazy default construct info. |
| src/libraries/System.Private.Xml/src/System/Xml/Xsl/IlGen/OptimizerPatterns.cs | Removes volatile from lazy singleton pattern caches. |
| src/libraries/System.Private.Xml/src/System/Xml/XmlConvert.cs | Removes volatile from cached date-time formats array. |
| src/libraries/System.Private.Xml/src/System/Xml/XPath/XPathNavigatorReader.cs | Removes volatile from empty navigator singleton cache. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs | Removes volatile from default namespaces cache. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSchemas.cs | Removes volatile from cached schema instances. |
| src/libraries/System.Private.Xml/src/System/Xml/Schema/DataTypeImplementation.cs | Removes volatile from compat simple-type caches. |
| src/libraries/System.Private.Xml/src/System/Xml/DiagnosticsSwitches.cs | Removes volatile from diagnostics switch caches. |
| src/libraries/System.Private.Xml/src/System/Xml/Core/XsdValidatingReader.cs | Removes volatile from cached Type value. |
| src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImplHelpers.cs | Removes volatile from cached NodeData.None singleton. |
| src/libraries/System.Private.Xml/src/System/Xml/BinaryXml/XmlBinaryReader.cs | Removes volatile from cached token type map. |
| src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs | Removes volatile from cached local TimeZoneInfo reference. |
| src/libraries/System.Private.CoreLib/src/System/TimeZone.cs | Removes volatile from cached current TimeZone reference. |
| src/libraries/System.Private.CoreLib/src/System/Text/EncodingProvider.cs | Removes volatile from global provider list. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/RuntimeInformation.Windows.cs | Removes volatile from cached OS arch sentinel. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/RuntimeInformation.Unix.cs | Removes volatile from cached OS arch sentinel. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/Opcode.cs | Removes volatile from opcode name cache. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs | Removes volatile from dynamically-hosted module cache. |
| src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs | Removes volatile from cached default culture references. |
| src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.cs | Removes volatile from cached culture dictionaries. |
| src/libraries/System.Private.CoreLib/src/System/Environment.cs | Removes volatile from multiple process/system cached values. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs | Removes volatile from global DebugProvider instance. |
| src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.NonWasm.cs | Removes volatile from cached network-derived values. |
| src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.cs | Removes volatile from cached store references. |
| src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs | Removes volatile from cached HttpClient instance. |
| src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs | Removes volatile from cached feature detection sentinel. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs | Removes volatile from cached switch/sentinel. |
| src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Enumerables/EmptyEnumerable.cs | Removes volatile from cached singleton instances. |
| src/libraries/System.Linq.Expressions/src/System/Runtime/CompilerServices/CallSite.cs | Removes volatile from cached call-site statics. |
| src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Expression.cs | Removes volatile from cached lambda factory table. |
| src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Linux.cs | Removes volatile from cached proc/pid-namespace sentinel. |
| src/libraries/System.Diagnostics.PerformanceCounter/src/System/Diagnostics/SharedPerformanceCounter.cs | Removes volatile from cached process data reference. |
| src/libraries/System.Diagnostics.PerformanceCounter/src/System/Diagnostics/PerformanceCounterLib.cs | Removes volatile from multiple cached strings/hashtables. |
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/AggregatorStore.cs | Removes volatile from internal union state and lookup cache. |
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DsesFilterAndTransform.cs | Removes volatile from cached PropertyFetch field. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Diagnostics/DiagnosticsConfiguration.cs | Removes volatile from cached config section reference (retains init-state volatile). |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/SettingValueElement.cs | Removes volatile from static property collection cache. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/IgnoreSection.cs | Removes volatile from static property collection cache. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/EmptyImpersonationContext.cs | Removes volatile from static singleton instance. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/DefaultSection.cs | Removes volatile from static property collection cache. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/ConfigurationValues.cs | Removes volatile from cached empty collection. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/ConfigurationManagerInternalFactory.cs | Removes volatile from lazily created singleton. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/ConfigurationElement.cs | Removes volatile from per-instance ElementInformation cache. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientSettingsStore.cs | Removes volatile from cached config factory instance. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigurationHost.cs | Removes volatile from cached file path. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs | Removes volatile from cached current config paths. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/AppSettingsSection.cs | Removes volatile from static property caches. |
| src/libraries/System.Composition.Hosting/src/System/Composition/Hosting/Core/ExportDescriptorRegistry.cs | Removes volatile from cached part definition map. |
| src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/LicenseManager.cs | Removes volatile from cached context/provider tables. |
| src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/BooleanConverter.cs | Removes volatile from cached standard values. |
| src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/ReflectionModel/ReflectionComposablePartDefinition.cs | Removes volatile from multiple lazy caches (still uses explicit lock). |
| src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/ReflectionModel/ReflectionComposablePart.cs | Removes volatile from caches used during composition. |
| src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Primitives/Export.cs | Removes volatile from exported value cache sentinel field. |
| src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Primitives/ComposablePartCatalog.cs | Removes volatile from cached queryable parts. |
| src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Hosting/TypeCatalog.cs | Removes volatile from cached part list. |
| src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Hosting/AssemblyCatalog.cs | Removes volatile from cached assembly/catalog fields. |
| src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Hosting/ApplicationCatalog.cs | Removes volatile from cached inner catalog reference. |
| src/libraries/Common/src/System/Security/Cryptography/Pkcs/Pkcs9MessageDigest.cs | Removes volatile from lazy decoded digest cache. |
| src/libraries/Common/src/System/Security/Cryptography/Pkcs/Pkcs9DocumentName.cs | Removes volatile from lazy decoded document name cache. |
| src/libraries/Common/src/System/Security/Cryptography/Pkcs/Pkcs9DocumentDescription.cs | Removes volatile from lazy decoded document description cache. |
| src/libraries/Common/src/System/Security/Cryptography/Pkcs/Pkcs9ContentType.cs | Removes volatile from lazy Oid cache. |
| src/libraries/Common/src/System/Console/ConsoleUtils.cs | Removes volatile from cached feature switch sentinel. |
| src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EVP.DigestAlgs.cs | Removes volatile from cached EVP digest pointers. |
| src/libraries/Common/src/Interop/Unix/System.Native/Interop.MemfdCreate.cs | Removes volatile from cached memfd support sentinel. |
| src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Evp.DigestAlgs.cs | Removes volatile from cached EVP digest pointers (Android). |
src/libraries/System.Private.CoreLib/src/System/Text/EncodingProvider.cs
Show resolved
Hide resolved
Member
Author
Member
|
I think it is fine to delete the unnecessary |
jkotas
reviewed
Mar 6, 2026
| private static volatile IntPtr s_evpSha256; | ||
| private static volatile IntPtr s_evpSha384; | ||
| private static volatile IntPtr s_evpSha512; | ||
| private static IntPtr s_evpMd5; |
Member
There was a problem hiding this comment.
This makes assumptions about memory model of C/C++ code that this pointer is used by. C/C++ memory model is weaker than what we provide. I do not think it is safe to delete the barriers for unmanaged "handles" like this one if you go by the spec. It is probably ok on the architectures we currently support, but C/C++ compilers surprised us many times in how they are able to push the optimizations to the limit of what's allowed by the spec.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Per the .NET memory model (docs/design/specs/Memory-model.md), many
volatilefield annotations in Libraries are unnecessary and impose overhead without benefit. This removesvolatilefrom 187 fields across 95 files.Reference-type fields (159 fields, 86 files)
Object assignment to a shared location is a release with respect to the instance's fields (line 135), and data-dependent reads are ordered (line 118). This means lazy-init patterns like
??=and sentinel-check-then-assign on reference-type fields do not need volatile: the publishing write ensures constructor side-effects are visible, and readers accessing the object through the reference see those effects. The memory model doc explicitly shows singleton patterns working without volatile (lines 217-277). Fields protected by locks or Interlocked operations also have volatile removed since those primitives provide stronger ordering guarantees.Value-type fields (28 fields, 13 files)
The memory model now prohibits read-introduction: \Reads cannot be introduced\ (line 50). Previously, volatile was needed on value-type lazy-init fields to prevent the JIT from replacing a local variable with a re-read of the field (TOCTOU risk). With read-introduction now prohibited, volatile is unnecessary on value-type fields where:
This includes IntPtr handle caches (crypto digest algorithms), sbyte tri-state flags, and int sentinel-based lazy-init fields like \s_processId\, \s_systemPageSize\, and \s_osArchPlusOne\.
Fields deliberately kept volatile