I encountered a multi-thread issue recently, and tried to solve it by using Thread.VolatileRead/Write.But when I saw the IL2CPP implementation of these two functions, it seems that the implementation is wrong.
For example, the IL2CPP implementation of ‘int Thread.VolatileRead(ref int address)’ can be found in Unity\Hub\Editor\2019.4.31f1\Editor\Data\il2cpp\libil2cpp\icalls\mscorlib\System.Threading\Thread.cpp:
The ‘MemoryBarrier’ should be called after loading from ‘address’, here is the implementation of .Net Framework (Reference Source):
public static int VolatileRead(ref int address)
{
int ret = address;
MemoryBarrier(); // Call MemoryBarrier to ensure the proper semantic in a portable way.
return ret;
}
I’m certainly not an expert here, but I think it doesn’t really make a difference. A memory barrier does not somehow lock threads or anything. They just ensure whatever code was before that instruction has been executed before any of the code after that barrier instruction. It’s a simply ordering instructions which prevents CPUs from applying out of order optimisations which could result in wrong behaviour.
So using the VolatileRead in code does simply read the current value but as a whole sets a marker in code to ensure the CPU does not mess with the ordering. As I said, I’m not an expert in that field. What exact issue do you have encountered and why do you thing a VolatileRead should solve it? It certainly does not solve any of the usual concurrency issues due to multithreading when two or more threads mess with the same variables. It just ensures that the ordering of instructions within the same thread is not messed up for optimisation purposes. A compiler does not know about other threads that may run at the same time. So within a single thread a CPU could reorder certain operations for performance improvements. For example imagine you have code like this
a = 5;
b = 6;
c = 5;
done = true;
Logically we would assume that variable “a” is assigned first, then “b”, then “c” and finally “done”. However the CPU could, for what reason ever, actually assign “done” first, then “c” then “a” then “b” because for this thread it doesn’t make any difference as all 4 variables are not used in between. It may group “a” and “c” since we assign the same value to it. “done” may be assigned first because it already has a value of “1” / true loaded in a register from code before it so the code becomes faster.
However such an optimisation could cause issues when you don’t have proper synchronisation between multiple thread and the other thread “assumes” that the other thread would execute those things in order. In a hypothetical scenario thread one may execute the “done = true” line and then gets interrupted. Now the other thread may watch the “done” variable to start its own processing and in turn reads the values of a, b and c. Of course those values have not yet been assigned. A memory fence before the “done = true” line would ensure that a, b and c are assigned before done is set to true.
Back to my question, as far as I know, VolatileRead should implement the acquire semantic:
For the implementation of Get method of DefaultJsonNameTable from Newtonsoft.Json, it’s important that ‘_mask’ should be read prior to ‘_entries’. IL2CPP implementation of Thread.VolatileRead uses MemoryBarrier before the load, so I think it can’t guarantee that hardware don’t reorder the two loads of ‘_mask’ and ‘_entries’.
Thanks for that detailed reply. Though this seems to me like a misguided issue. The whole class doesn’t look to be thread-safe to begin with and all the issues seems to come from the lack of proper synchronisation as far as I can tell. To me that test case you presented in the issue is already flawed in several ways. You have one thread adding items to the collection and 3 threads reading from it at the same time. You have a lock inside the writing thread which is pretty pointless if that’s the only thread using the lock, so the lock is pointless. Locks only work if two or more threads use it.
On the other hand, as I said the “DefaultJsonNameTable” class is not thread safe at all as both the Add as well as the Get method are not in any way atomic or suggest that they are. No amount of volatile of prevention of reordering would fix that fundamental flaw in that test case. Look inside the Grow method (which is the only real issue here) it will set a new mask and the new entities array, both belong together. The method first sets the entities array, then the mask. So even if the ordering is fine in both threads, this code would cause the wrong result in case of a race condition as it may use the old mask but the new entities array. So you would index into the wrong array element. Yes, the wrong ordering would cause an index out of bounds exception because using the new mask on the old entities array would cause that since after Grow the mask is twice as large but the old array has half the size the new mask would cover.
So the issue here is that the class is not thread safe in the first place and having more than one thread accessing it simply won’t work. The reordering issue is irrelevant when proper synchronisation between threads is used.
So you’re essentially trying to duct tape the index out of bounds exception away that is caused by your race condition. However that does not fix the fundamental flaw in the first place. So just as likely it is that you currently get an out of bounds exception, it’s just as likely to that the get call returns null, even though the key does exist.
While some race conditions could be catched afterwards, maybe even automatically because you have a guard clause that already checks if the value returned is null, depending on the usage you just get inconsistant behaviour which you want to avoid at all cost.
I think the author implemented this way is due to performance. Provided that adding a new string is not frequently, the Get method can return the string from the cache without using any synchronization. In rare case that the Get method returns null even the string exists in the cache, creating a new string seems to be acceptable.
Well, I haven’t really looked that deep into the classes, but the most obvious fix would be to simply add a lock to every call of Get. Like the line you mentioned:
I did a simple test to compare the performance between using lock and Volatile.Read/Write. The test programs are built using Release configuration and .net5.
Windows 10, using Volatile.Read/Write
CPU Arch: X64, Processor count: 16
100: 997ms
200: 961ms
300: 967ms
400: 954ms
500: 963ms
Windows 10, using lock
CPU Arch: X64, Processor count: 16
100: 1267ms
200: 1237ms
300: 1235ms
400: 1235ms
500: 1232ms
Raspberry Pi, using Volatile.Read/Write
CPU Arch: Arm, Processor count: 4
100: 6143ms
200: 6069ms
300: 6076ms
400: 6162ms
500: 6106ms
Raspberry Pi, using lock
CPU Arch: Arm, Processor count: 4
100: 7527ms
200: 7452ms
300: 7391ms
400: 7361ms
500: 7365ms
As you can see, the performance becomes worse when using lock.
I believe you are current in noticed that the IL2CPP implementation here is incorrect. However, the code is a bit misleading, as IL2CPP will not actually use this code - it will remap the managed function call to an intrinsic function named VolatileRead that is templated on the argument type. This method does have the proper implementation, and will be inlined, so should provide better performance.
I saw the code genereated by IL2CPP, it’s different from what you described.
For the code below
public class Test : MonoBehaviour
{
public int Value;
public void Test1()
{
Thread.VolatileRead(ref Value);
}
public void Test2()
{
Volatile.Read(ref Value);
}
}
IL2CPP (Unity 2019.4.31) generated the following code for Test1 method:
As you can see, Thread.VolatileRead finally calls the C++ method I mentioned which I think the implementation is not correct, and Volatile.Read finally calls the function you mentioned:
template<typename T>
inline T VolatileRead(T* location)
{
T result = *location;
il2cpp_codegen_memory_barrier();
return result;
}