Skip to content
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

Add a thread safe mutable dict class #1794

Merged
merged 5 commits into from Mar 19, 2014
Merged

Add a thread safe mutable dict class #1794

merged 5 commits into from Mar 19, 2014

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 12, 2014

Just now I got like 4 crashes in objectWithIdentifier: so I thought it was finally time to make objectDictionary thread safe.

This will have performance implications, but my limited testing hasn't shown it to be problematic.
Of course, this is just a poor man's fix for a badly designed multithreading mess, but what can we do?

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 12, 2014

Seems fine, but two questions:

  1. We call @synchronized everywhere that mutates that dictionary. Why wasn’t that preventing the crashes? Do we also need to call it when reading values back out?
  2. If this is now a QSThreadSafeMutableDictionary with its own locking technique, do we still need the @synchronized calls, or is that just unnecessary additional overhead now?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 12, 2014

  1. We call @synchronized everywhere that mutates that dictionary. Why wasn’t that preventing the crashes? Do we also need to call it when reading values back out?

Do we really though? There might be cases where we’ve missed out (OK, you’re right - I can’t see anything in the code). Synchronizing when reading values is especially important.
Picture the scenario: we read the value for the identifier “Some ID”. In the process of doing this, some other thread removes that value so we get gobeldy gook when we return the read value (before we’ve retained it)
This is kind of the same principal behind the idea that setters AND getters need to be atomic (thread safe) to work properly

  1. If this is now a QSThreadSafeMutableDictionary with its own locking technique, do we still need the @synchronized calls, or is that just unnecessary additional overhead now?

Probably not. I supposed we could either:
a) make all getters use @synchronized
b) switch to the Lock() method and remove the @synchronized calls. I’m guessing the atomic lock is faster than @synchronized, but I have nothing to justify that

On 12 Mawrth 2014, at 22:00, Rob McBroom notifications@github.com wrote:

Seems fine, but two questions:

We call @synchronized everywhere that mutates that dictionary. Why wasn’t that preventing the crashes? Do we also need to call it when reading values back out?
If this is now a QSThreadSafeMutableDictionary with its own locking technique, do we still need the @synchronized calls, or is that just unnecessary additional overhead now?

Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 12, 2014

switch to the Lock() method and remove the @synchronized calls. I’m guessing the atomic lock is faster than @synchronized, but I have nothing to justify that

I’m running that way now, and it seems faster, which is complete subjective BS, but the important thing is it definitely isn’t hurting. Anyway, it gets my vote.

@skurfer skurfer added this to the 1.2.0 milestone Mar 12, 2014
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 13, 2014

I knew there'd be stuff online about this, I just hadn't been bothered to look it up:
http://perpendiculo.us/2009/09/synchronized-nslock-pthread-osspinlock-showdown-done-right/comment-page-1/

I can't believe @synchronized is more than 10 times slower than OSSpinLock!
The only worrying thing is what is said about OSSpinLock being wasteful in real lock situations (it polls, waiting for the unlock). Maybe pthread_mutex is the way to go. Here's the code in the above post updated slightly and the results on my MBP:

Results

NSLock: 2.541979 sec
pthread_mutex: 1.432258 sec
OSSpinlock: 0.333578 sec
@synchronized: 4.647569 sec

Code

#import <Foundation/Foundation.h>
#import <objc/runtime.h>
#import <objc/message.h>
#import <libkern/OSAtomic.h>
#import <pthread.h>

#define ITERATIONS (1024*1024*32)

static unsigned long long disp=0, land=0;

int main()
{
    double then, now;
    NSUInteger i, count;
    pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
    OSSpinLock spinlock = OS_SPINLOCK_INIT;

    @autoreleasepool {

    NSLock *lock = [NSLock new];
    then = CFAbsoluteTimeGetCurrent();
    for(i=0;i<ITERATIONS;++i)
    {
        [lock lock];
        [lock unlock];
    }
    now = CFAbsoluteTimeGetCurrent();
    printf("NSLock: %f sec\n", now-then);

    then = CFAbsoluteTimeGetCurrent();
    for(i=0;i<ITERATIONS;++i)
    {
        pthread_mutex_lock(&mutex);
        pthread_mutex_unlock(&mutex);
    }
    now = CFAbsoluteTimeGetCurrent();
    printf("pthread_mutex: %f sec\n", now-then);

    then = CFAbsoluteTimeGetCurrent();
    for(i=0;i<ITERATIONS;++i)
    {
        OSSpinLockLock(&spinlock);
        OSSpinLockUnlock(&spinlock);
    }
    now = CFAbsoluteTimeGetCurrent();
    printf("OSSpinlock: %f sec\n", now-then);

    id obj = [NSObject new];

    then = CFAbsoluteTimeGetCurrent();
    for(i=0;i<ITERATIONS;++i)
    {
        @synchronized(obj)
        {
        }
    }
    now = CFAbsoluteTimeGetCurrent();
    printf("@synchronized: %f sec\n", now-then);
    }
    return 0;
}

We're using an OSSpinLock now instead
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 13, 2014

OK so I removed the @synchronized calls and did some more testing/profiling. Can't really find any difference between the two. If there's a bottleneck, it's not with objectDictionary.

Let's let @tiennou chime in (if he wants to, he normally likes threading) before we merge... :)

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 13, 2014

The only worrying thing is what is said about OSSpinLock being wasteful in real lock situations (it polls, waiting for the unlock). Maybe pthread_mutex is the way to go.

True, but all the code we’re locking for amounts to a single in-memory operation, so it shouldn’t be sitting and polling for too long. Also, it looks like the two have diverged quite a bit from his post as hardware has gotten faster:

NSLock: 1.907065 sec
NSLock+IMP Cache: 1.764769 sec
pthread_mutex: 1.165217 sec
OSSpinlock: 0.269465 sec
@synchronized: 2.385904 sec

pthread_mutex has hardly changed since 2009, while OSSpinlock has gotten much faster. Not here, but long term, we should look for other places where QSThreadSafeMutableDictionary makes sense.

I’m happy with this as is, but I’ll wait for @tiennou to comment.

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 13, 2014

As soon as we come to a decision on this, we can get the next 1.2.0 preview out. So… have you looked at it yet? 😉

#1792 needs to be addressed before the final release, but I won’t let it hold up a dev preview.

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 14, 2014

Not here, but long term, we should look for other places where QSThreadSafeMutableDictionary makes sense.

So much for “long term”. I looked into this and there are so few places, maybe we should do it now? I didn’t want to commit here in case there’s disagreement, so I created a patch you can look at. Just run git apply threads.patch on this branch to see/test the changes.

Yes, I’m ignoring the “standard optimization disclaimers” from the post, but this was less about performance, and more about stability.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 18, 2014

I meant to reply...

I'm torn between whether or not we should include the patch. Since there may still be some kind of issues with what we've done (we just don't know about it), perhaps we should roll with what we've got. If we don't get any horrible crash reports from the next build, then we can roll your patch into the first pre-release build.

How's that sound?

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 18, 2014

Sounds fine. For what it’s worth, I’ve been running with a patched build and everything’s been smooth, but it can certainly wait. I’ll just do a pull request after this is merged.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 19, 2014

OK, cool. Shall we merge? :)

skurfer added a commit that referenced this issue Mar 19, 2014
Add a thread safe mutable dict class
@skurfer skurfer merged commit 4169291 into master Mar 19, 2014
1 check passed
@skurfer skurfer deleted the threadsafe-objdict branch Mar 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants