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
Atomic: SPINLOCK is not safe in iOS #2619
Comments
I'm not even sure what "illegal" means. |
I'm just curious, as I wasn't around from the beginning, but why was Was Spinlocks are not something I'd think to use in a non-kernel, non-device-driver context, which is why I ask. If anything, I'd say that the bug here is that |
To add a counter-point to my comment above, this isn't as big a deal as the title might make it out to be: https://twitter.com/Catfish_Man/status/676854531615883265
As for the comment above from @kastiglione:
I don't have the answer, but I would guess that something in the ARM CPU doesn't allow the spinlock primitive to enjoy many of the same benefits/guarantees/assumptions as it does on Intel CPUs. (But likely not all of them, according to https://twitter.com/Catfish_Man/status/676851988596809728) Anyway, there's likely "nothing to see here" in practice, and instead some investigation should be done to replace with the safer primitive and measure for any performance regressions, as @NachoSoto suggests. |
See https://twitter.com/jspahrsummers/status/679245419931660288 and follow-up, including #278. |
I do think we should replace our uses of |
Yeah, and thanks for the relevant history commits. Not sure why GitHub doesn't do a At any rate, I will reiterate that "it's probably fine." Using a "now faster" mutex is probably a straightforward task, however it is less so without knowing how you profiled / tested them to begin with. Got any suggestions/pointers there? As for a way forward if/when performance becomes a concern, lock-free queues are worth investigating. (Here's a pretty good article for readers unfamiliar with the concept: http://www.linuxjournal.com/content/lock-free-multi-producer-multi-consumer-queue-ring-buffer?page=0,0). I built something similar for an internal audio library a while ago, and I may be able to adapt it for general use like this. |
I was running GitHub Desktop in Instruments' Time Profiler and Allocations, and focusing on RAC-heavy stacks. You'll have to talk to @joshaber or @mdiep for that now. 😉 |
OK good to know. I imagine the Swift-side stuff will require a similarly large/deep code base to test with. I have grown some of my own over the last few months1 but I don't know whether I push RAC "hard enough" to get enough samples out of it. Plenty of other things tend to bubble up in the Instruments charts with apps like mine. 😄 1 Plug! http://capoapp.com, with http://capoapp.com/neptune being the most recent, totally-designed-using-RAC4 component. |
I wrote a blog post today that goes into more detail about why spinlocks are "illegal" on iOS: http://engineering.postmates.com/Spinlocks-Considered-Harmful-On-iOS/ |
I broke off Atomic a little while ago and migrated it to pthread_mutex_lock. By all accounts it's the best lock there is – no dynamic dispatch like NSLock, instant lock if not contended like spinlock, doesn't waste energy unlike spin lock. Feel free to copy-paste the source from my repo or do whatever you like with it. https://github.com/Adlai-Holler/Atomic |
@kballard that's a great explanation, thanks! I don't have strong opinions on this as I'm not an expert on the matter. I'll defer to others on the decision to change (or not change) what type of lock we use, though I'd be happy to make the change myself! |
A benchmark like that is completely useless without source. What's more, you're only showing a single time for each construct, without documenting what that time actually represents. The various constructs will behave differently depending on whether they're under contention, the priorities of the threads that are contending on the lock, etc. |
NB: |
@jspahrsummers @kballard I have an implementation using atomic CAS. If there is any interest I'm happy to submit a PR. edit: Looking at the use of |
I found out this thread today, and made an investigation of different types of synchronization and did execute a benchmark on iOS simulator and real devices. |
Your numbers look very suspect to me. How many times did you run the tests? For comparison, I ran my own benchmark (source) on OS X 10.11.5 using a hacky benchmark harness I wrote a while back. I ran the entire benchmark 10 times, dropped the bottom and top 2 numbers from each test (to get rid of outliers), and the rest of the numbers produced the following ranges: no sync: 22-41ns From this you can see that spinlock is by far the cheapest, followed by semaphore, then mutex, then NSLock just slightly behind mutex (since it's basically mutex + objc_msgSend), then synchronized is almost twice as expensive, and finally queues end up being extremely expensive, much more-so than I expected. |
@kballard as you can see, my benchmark is quite simple, but it uses default xctest mechanism for measuring execution time. Numbers are average run time for each test, not for each sync. In general, your and my results are correlating excepting queues, which are not so bad, as we used to think |
okok |
https://twitter.com/steipete/status/676851647042203648
I'll refrain from making any comments (I'll just say... if it's illegal on iOS, why is it a public API???)
Apparently
pthread
mutexes are faster now. We should create a benchmark to compare, and try to convertAtomic
to this (alsoRACCompoundDisposable
andRACSerialDisposable
?)The text was updated successfully, but these errors were encountered: