make AtomTable consurrency safe #1980
Conversation
|
Ah, the joys of yaml, bviously 1.70 is the string "1.7" the zero is definitly unimportant 🙄. |
|
All build-test CI jobs appear to take ~7 Minutes longer with these changes. |
|
Incredible contribution, thank you a lot for working on this! A remaining problematic aspect is currently a severe slowdown also in other applications (in addition to the CI tests). For instance, with these changes applied, just launching Scryer already takes a few seconds on my machine: $ time scryer-prolog -g halt real 0m5.283s user 0m5.268s sys 0m6.425s That's a slowdown of roughly 2 orders of magnitude compared to |
|
This paper may be helpful in improving performance: https://arxiv.org/pdf/1608.00989.pdf |
Yeah, I think something like that will be needed, I was hoping that adding the lock wouldn't impackt performance too much especially in the singel threaded case, but that appears to be not the case. I don't think I will get to that durring the week, so maybe next weekend. Flamegraph ( |
|
Also I think |
It was to allow the |
|
With my latest changes I am back to more reasonable times though as I mentioned I think F64Table looks like it also needs to be fixed to support concurrency. I think I saw another global somewhere, that should maybe also be checked if it is correct under concurrency. |
|
Nice, the speed looks a lot better now! I measured between 20% and 40% slowdown in several benchmarks. I think that's quite acceptable, if in return we get the ability to run multiple threads which is very exciting! It would still be nice if this could be sped up further eventually, especially if only a single thread is running. |
|
Ok, I think I am done now. F64Ptr looks a bit dangerous with the The remainig statics appear mostly fine, though some could probably be |
|
rebased to resolve conflicts |
|
The most important aspect is that this now even works correctly at all! As long as the speed is even somewhat acceptable, this is an awesome new feature, thank you a lot! I hope you plan to attend the Scryer meetup in November? Please also consider talking about these improvements there, they are very interesting! |
|
I won't be able to attend as I have an important event at work that week (Tuesday to Friday) for which I am indisposable. |
|
|
there are technically still two locks - the Mutex to serialize consurrent updates to the AtomTable - the RwLock as part of GLOBAL_ATOM_TABLE the former is the age lock of RESIZE ATOM TABLE in <https://arxiv.org/pdf/1608.00989.pdf>, though we use it for the whole update as we don't match the whole structure and can't reserve an atom slot as is, so we need to also lock concurrent updates without resizing the later should be uncontendet as we only write AtomTable::new iff the current value is a dangling Weak
|
rebased again to fix the |
This makes it possible to use multiple machines at the same time and removes one obstacle for moving a Machine to a different thread.
Note: currently searching for the reason that all integration test appear deadlock immediatly, unittest already work.Update: Found the deadlock, now investigating the slow down, though its probably just a bunch of RwLock locking that can't be easily remedied.