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

Use simple mutexes for cases where RDLOCK is not native. #23

Closed
wants to merge 10 commits into
base: simplify
from

Conversation

Projects
None yet
4 participants
@t3rmin4t0r

t3rmin4t0r commented Jun 25, 2013

This is exactly the same as using two mutexes because
for RDLOCK you do
LOCK(lock->read);
for WRLOCK you do
LOCK(lock->read);
the second lock on ->write is irrelevant because it exists within this mutex always.

So it is simpler & faster to use just a single lock.

Once that is in place, the RDLOCK for the native READLOCK has a race condition

ref_count++ | ref_count--

for the cache entry can run in two parallel processes with ambigous results.

The gcc/vc++ intrinsics for the atomic increments remove this race condition

__sync_add_and_fetch(ref_count) | __sync_sub_and_fetch(ref_count)

will run safely in parallel, as they lock each other out during the read-modify-write sections.

Use simple mutexes for cases where RDLOCK is not native.
This is exactly the same as using two mutexes because
for RDLOCK you do
  LOCK(lock->read);
for WRLOCK you do
  LOCK(lock->read);
the second lock on ->write is irrelevant because it exists within this mutex always.

So it is simpler & faster to use just a single lock.

Once that is in place, the RDLOCK for the native READLOCK has a race condition

ref_count++ | ref_count--

for the cache entry can run in two parallel processes with ambigous results.

The gcc/vc++ intrinsics for the atomic increments remove this race condition

__sync_add_and_fetch(ref_count) | __sync_sub_and_fetch(ref_count)

will run safely in parallel, as they lock each other out during the read-modify-write sections.
@krakjoe

This comment has been minimized.

Show comment
Hide comment
@krakjoe

krakjoe Jun 25, 2013

Owner

The reason two mutex are used is to simplify logic, assume a rwlock always exists, and prefer readers. If you can produce test results that I can see that say that using a single mutex keeps the application moving faster I'd be interested to see them, but during testing it did not seem to be the case.

Owner

krakjoe commented Jun 25, 2013

The reason two mutex are used is to simplify logic, assume a rwlock always exists, and prefer readers. If you can produce test results that I can see that say that using a single mutex keeps the application moving faster I'd be interested to see them, but during testing it did not seem to be the case.

@peterbowey

This comment has been minimized.

Show comment
Hide comment
@peterbowey

peterbowey Apr 6, 2014

@t3rmin4t0r and @weltling It does appear, on 'code' examination that most of the above have been included in the later releases (in various misc. modified code additions)!

So why is this pull #23 still showing a open status?

peterbowey commented Apr 6, 2014

@t3rmin4t0r and @weltling It does appear, on 'code' examination that most of the above have been included in the later releases (in various misc. modified code additions)!

So why is this pull #23 still showing a open status?

@krakjoe

This comment has been minimized.

Show comment
Hide comment
@krakjoe

krakjoe Nov 3, 2015

Owner

I have implemented the simpler mutex for seven branch, I'm still looking at the other race condition thing ... I will eventually backport to 5 branch before doing a final release for 5 series ...

Owner

krakjoe commented Nov 3, 2015

I have implemented the simpler mutex for seven branch, I'm still looking at the other race condition thing ... I will eventually backport to 5 branch before doing a final release for 5 series ...

krakjoe added a commit that referenced this pull request Nov 12, 2015

@krakjoe

This comment has been minimized.

Show comment
Hide comment
@krakjoe

krakjoe Nov 13, 2015

Owner

The seven branch now has these, and hopefully some other problems solved ...

Owner

krakjoe commented Nov 13, 2015

The seven branch now has these, and hopefully some other problems solved ...

@krakjoe krakjoe closed this Nov 19, 2015

@krakjoe

This comment has been minimized.

Show comment
Hide comment
@krakjoe

krakjoe Nov 20, 2015

Owner

@t3rmin4t0r omg ... I just realised what this was trying to fix ... I feel stupid ...

Owner

krakjoe commented Nov 20, 2015

@t3rmin4t0r omg ... I just realised what this was trying to fix ... I feel stupid ...

@krakjoe

This comment has been minimized.

Show comment
Hide comment
@krakjoe

krakjoe Nov 20, 2015

Owner

I've genuinely fixed it now ...

@t3rmin4t0r I just didn't realise I had omitted to put the atomics back in when I tore apart APC to make APCu ... a stupid stupid mistake, my fault ...

Owner

krakjoe commented Nov 20, 2015

I've genuinely fixed it now ...

@t3rmin4t0r I just didn't realise I had omitted to put the atomics back in when I tore apart APC to make APCu ... a stupid stupid mistake, my fault ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment