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

Change sync::rwlock to use atomics for read_count/read_mode instead of locking #7066

Closed
bblum opened this issue Jun 11, 2013 · 0 comments
Closed
Labels
A-concurrency Area: Concurrency related issues. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@bblum
Copy link
Contributor

bblum commented Jun 11, 2013

The rwlocks are implemented following the algorithm described here: http://en.wikipedia.org/wiki/Readers-writers_problem#The_third_readers-writers_problem , which has a note:

Note that sections protected by counter_mutex could be replaced by a suitable fetch-and-add atomic instruction, saving two potential context switches in reader's code.

I should implement this, convince myself that it's right, and profile its performance against the old version.

You might argue that doing this the more clever way makes the code less maintainable, that it might accidentally have a bug that would be a lot harder to find. This is possible, although I might argue in response that the logic should never need to be changed, so if I get it right, it will be right forever. We'll have to see how much of a performance improvement it is.

@ghost ghost assigned bblum Jun 11, 2013
bors added a commit that referenced this issue Jun 15, 2013
r? @brson

links to issues: #7065 the race that's fixed; #7066 the perf improvement I added. There are also some minor cleanup commits here.

To measure the performance improvement from replacing the exclusive with an atomic uint, I edited the ```msgsend-ring-rw-arcs``` bench test to do a ```write_downgrade``` instead of just a ```write```, so that it stressed the code paths that accessed ```read_count```. (At first I was still using ```write``` and saw no performance difference whatsoever, whoooops.)

The bench test measures how long it takes to send 1,000,000 messages by using rwarcs to emulate pipes. I also measured the performance difference imposed by the fix to the ```access_lock``` race (which involves taking an extra semaphore in the ```cond.wait()``` path). The net result is that fixing the race imposes a 4% to 5% slowdown, but doing the atomic uint optimization gives a 6% to 8% speedup.

Note that this speedup will be most visible in read- or downgrade-heavy workloads. If an RWARC's only users are writers, the optimization doesn't matter. All the same, I think this more than justifies the extra complexity I mentioned in #7066.

The raw numbers are:
```
with xadd read count
        before write_cond fix
                4.18 to 4.26 us/message
        with write_cond fix
                4.35 to 4.39 us/message
                
with exclusive read count
        before write_cond fix
                4.41 to 4.47 us/message
        with write_cond fix
                4.65 to 4.76 us/message
```
@bblum bblum closed this as completed in 57cb44d Jun 15, 2013
@bblum bblum removed their assignment Jun 16, 2014
flip1995 pushed a commit to flip1995/rust that referenced this issue Apr 22, 2021
useless use of format! should return function directly

fixes rust-lang#7066

changelog: [`useless_format`] wraps the content in the braces when it's needed.

r? `@giraffate`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency related issues. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

No branches or pull requests

1 participant