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.
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
Memory model document. #75790
Memory model document. #75790
Changes from 5 commits
b1dc5ee
1fa2ecc
c4b4765
b872aba
aae251b
f13605d
a8a1003
b15a921
d554b05
38f0585
fe0a65e
e8861ec
5a25cab
be72e2e
75adccb
eec3b96
aaeadfc
4cdac19
05694e3
804942f
f1e13ed
d7c179c
615c434
20b6b52
5579343
31431ed
8b23a88
e02a5df
280014d
608f45d
ed89b04
136e637
34a074d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about members of structs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fields are variables.
Regardless how you get something that has, for example, type
int
, it will be aligned accordingly. The runtime (i.e. JIT, GC, TypeSystem) will ensure alignment for all variables under runtime's control.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FieldOffsetAttribute
could possibly introduce unaligned fields.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably, by using
FieldOffsetAttribute
you are not letting the runtime to manage locations of the fields.I will add a note about
FieldOffsetAttribute
as something that may cause unaligned variables.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also want to mention that reading and storing object references and unmanaged pointers is atomic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object references are always aligned. We can mention that (if it is already not mentioned).
But unmanaged pointers are just numbers with dereference operation defined. They may not be aligned, then reading/storing is not necessarily atomic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you mean the pointers themselves. When managed by the runtime the pointers and references are "properly aligned" and thus read/writes are atomic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the value of unmanaged pointer. Unmanaged pointer is not a primitive type, so the current wording does not cover it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it may be worth mentioning this. It follows from the "properly aligned" but pointers/references is a very common scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
unaligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The rest of the doc is present tense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: The doc still has a mix of present and future tenses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this forbid memory-mapped files mapped at multiple virtual addresses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Runtime will assume that the memory is ordinary memory. If there is some additional semantics to the writes (as write to multimapped location will update other views), that is an Undefined Behavior. There can be no guarantees from the runtime on what will happen.
It may work as expected in simple cases, but the runtime would not help or prevent that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are "Speculative writes are not allowed" and "Reads cannot be introduced" really consequences of the above assumption, or additional guarantees made by the runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a design choice, but it follows from assuming that ordinary writes/reads can have observable sideeffects.
If you assume that a write may be visible from other threads (unless the location is known to be local to the current thread), then you can't replace
with
Memory models that require only singlethreaded correctness allow this (ex: ordinary writes in C++), which is often unexpected and may result in subtle bugs in multithreaded scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we are basically saying that the jit cannot introduce a race, but is allowed to remove one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ordinary memory accesses - yes. A program cannot condition its correctness on a nondeterministic race, that is not guaranteed to ever happen, so we can remove races. Adding a race would be observable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there are rare scenarios when a program explicitly wants to observe races - like making multiple or repeated reads of the same location and checking if the value has changed.
In such case it needs to use
Volatile.Read
, or a proposed compiler fence (#75874)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the following RyuJit behavior is a bug?
(This particular example needs
DOTNET_JitNoCSE=1
to be reproduced)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
DOTNET_JitNoCSE
is a supported scenario, then this would seem like a bug."b == c" in the source and compiler introduced a race. @AndyAyersMS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JitNoCSE
was just to create an easy and small sample; without it the compiler would have folded the duplicated load back, of course, that's not guaranteed in the general case.Opened #75916 to track this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should avoid duplicating reads like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are more aggressive than this today. If a program does ordinary reads of a field twice without any possibility of an intervening synchronization construct or aliasing write, the jit may do just one read. The reads to not have to be adjacent.
For example
may be transformed to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but in a case of "without any possibility of an intervening synchronization construct or aliasing write" you can mentally move the reads around so that they are adjacent and then coalesce.
JIT reasons in different terms which are more convenient in the implementation, but I think in the end there is equivalency, and JIT conforms to the rules in this document. (modulo unintentional bugs, if there are any)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting #47261 here as that would seem to be one such bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#47261 is actually ByDesign.
When the reads are unused, there can be no race. It is permitted to remove unused reads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
volatile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention c# volatile keyword in here? Since most readers will commonly use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically C# volatile belongs to C# spec, but it is common, so a few notes on that would be useful.
Adding "volatile" on a variable in C# is just a decoration that makes no difference to CLR, but it is a hint to C# compiler itself (and few other compilers) to emit reads and writes of that variable as volatile reads/writes - this is the part that CLR will honor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be great as an "aside" here, although it's not a CLR concept it is useful context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does any of this map to C# that can be described?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think C# emits these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe off-topic, but doesn't this also need compiler memory barriers to be effective?
e.g.
runtime/src/libraries/System.Threading/tests/InterlockedTests.cs
Lines 677 to 682 in 118a162
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Process-wide barrier is typically paired with a compiler barrier. A not-inlineable getter or a volatile read can work as such, but maybe it is time to have an official compiler barrier helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: #75874
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to reference the actual attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danmoseley - here is somehting that could differ on Mono. Mono has a switch to turn this off. It is not the default behavior though, so I do not think it needs to be mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment wherever CLR is used, is Mono relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Mono is the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will be changing
CLR
reference to.Net
, as discussed a few pages above. This doc tries to be applicable to all common implementations of.Net
(we will treat disagreements with the final form of this doc as bugs, if such disagreements are found)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must or will?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More precisely "will become observable not later than effects of accessing any member of the type..." or something like that.
Probably needs some re-wording.
Easy way to see it as if there is a lock that is taken when static constructor runs and releasing the lock is a release. The lock also guarantees that static constructor runs only once.
The implementation is not always a lock though, sometimes static constructors run at JIT time, NativeAOT may not run some static constructors at all and just use precomputed results embedded in the binary. The ordering result is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - "prior to" or "before"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this relevant today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but people keep bringing this up as a concern. - "I heard that some CPUs do not honor data dependency" - Yes, some flavors of Alphas had that issue, possibly a mistake in the design, no, noone does that again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localObj.ToString()
might not be a good example, because.ToString()
could possibly read static fields that were written by the constructor (without usingvolatile
). Static field reads are not dependent reads, and do not have the ordering guarantee. Probably better to explicitly read some instance field here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example is expected to work correctly even if
.ToString
accessesstring
's static fields.it would be highly inconvenient if it did not work as caller does not often know these details.
The rule about static constructors above tells that the cctor runs before any member is accessed and it would be useless to guarantee that static constructor "runs before" accessing without implying that the access will see the complete results.
The mechanism will vary and often the results of the static constructors will be computed well in advance before the actual access (at JIT time or Load time).
As a last resort this will be a dependent read from the type-attached storage, and the type would be derived from the instance, so there is a dependency chain from the instance to the static data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean static constructor. Consider
There is no dependency between reading
C.c
inC.PrintSingleton()
and readingC.name
inC.ToString()
. Both threads may not print "My Name".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is just a write to some shared memory from the constructor. It could be a static variable in another class or unmanaged memory. Constructor can do many things. It can also launch a thread that will write to statics or it can do
c = this
orlockObj = null
.Having a constructor with sideeffects other than constructing and initializing the instance would make the type a poor choice for the singleton pattern.
The sample here is not about that, but about guarantees provided by the runtime.
I will add a definition of a simple
MyClass
so there is no confusion about what constructor does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private readonly object
Then it's consistent with
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock#guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be better to use volatile read here to ensure side effects of the constructor, e.g. static field writes, are visible.
LazyInitializer
currently also uses volatile reads.runtime/src/libraries/System.Private.CoreLib/src/System/Threading/LazyInitializer.cs
Lines 247 to 248 in c59b517
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for volatile reads. Any data written directly or indirectly prior to the publishing of the instance will be visible via the instance. (see comments about the statics in the previous comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be better to use volatile read here. See above for reasons.