Refactor extra::sync, and extra::arc #7701
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @bblum |
|
I'm not sure about the name |
|
On the other hand |
|
|
|
One strategy I was thinking about doing for naming was to have the simple names like Also, I've been still working on this pull request and double checking it. |
|
You've deleted several testcases: presumably the functionality is still there, and so the tests just need to be adjusted for the new modules? (This isn't my area of expertises, so I could easily be wrong.) (Also, you appear to have included the compiled |
|
The About the test cases, instead of using an explicit I did xfail https://github.com/mozilla/rust/pull/7701/files#diff-15 , and https://github.com/mozilla/rust/pull/7701/files#diff-16 though which when modified gave me strange internal compiler errors. I plan on submitting a full bug report on the issue but have not isolated a suitable small test case yet. Also, I temporarily disabled the https://github.com/mozilla/rust/pull/7701/files#diff-38 test case that failed on my machine, and forgot to change that back. I'll fix that in a bit. |
|
Overall the RAII implementation looks good, but there are a lot of interface changes that I disagree with, and one showstopper race condition. |
|
Okay, so I can't get this pull request to work due to issue #7804 so I'm going to break this pull request up into a few smaller pull requests that don't touch on that issue. Also, if anyone thinks it'd be cleaner to close this specific pull request, and have me open a new one when that weird issue that's blocking me is fixed instead of leaving this one open I would not be opposed. |
|
So huonw pointed out issue #7899 and that let me figure out a workaround so this pull request is now not blocked on that. I'm not exactly happy with how the current system for condition variables works so I might make a few more changes. As well, I haven't yet benchmarked the performance impact of these changes so I might have to make changes for that as well. Finally, I can still probable polish this pull request up a bit more. |
|
Okay, so using the following test script I got the benchmarks at the bottom. #! /bin/sh
for COMMAND in \
./x86_64-unknown-linux-gnu/test/bench/msgsend-ring-mutex-arcs.stage1-x86_64-unknown-linux-gnu \
./x86_64-unknown-linux-gnu/test/bench/msgsend-ring-rw-arcs.stage1-x86_64-unknown-linux-gnu \
./x86_64-unknown-linux-gnu/test/bench/graph500-bfs.stage1-x86_64-unknown-linux-gnu
do
echo ${COMMAND}
for II in `seq 0 5`
do
${COMMAND}
done
echo
doneThe results are pretty good for the performance of RWArcs but are slightly worse for the performance of MutexArcs. The graph500-bfs results are included in performance but shouldn't, and didn't change much. Old results on my computer. New results |
Convert functionality into RAII style. Split functionality up into the modules. Cleanup dependencies, and redundancies.
|
So I'm going to break this up into smaller pull requests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a really big pull request for me so I probable got stuff wrong (also concurrency is really really really difficult.) Because I'm so uncertain about if this is pull request is correct I really want other people to look over it, and second guess it.
Convert functionality into RAII style.
Split functionality up into the modules.
Cleanup dependencies, and redundancies.