Uh oh!
There was an error while loading. Please reload this page.
Rewrite mpsc::shared - #42883
Conversation
Previous version had too much state: `cnt`, `steals`, `port_dropped` fields, and it's too hard to consistently update all of them during upgrade. I tried to fix issue rust-lang#39364, but there are too many corner cases. In this version all of these fields removed, and shared state is basically managed by two fields: `queue` and `to_wait`. `to_wake` field now stores both `SignalToken` and "disconnected" flag. All tests still pass, and bug rust-lang#39364 no longer reproduces. Patch includes a couple of stress tests. This version should have the same performance characteristics, because roughly the same number of atomics and wait/notify operations involved.
rust-highfive
commented
Jun 24, 2017
(rust_highfive has picked a reviewer for you, use r? to override) |
aidanhs
commented
Jun 28, 2017
I've pinged @alexcrichton, but I suspect he may not get to this until next week. |
alexcrichton
commented
Jun 29, 2017
Thanks for the ping, I'd definitely like to look at this but I won't be able to get to this until next week at the earliest, sorry for the delay! |
alexcrichton
commented
Jul 5, 2017
Sorry this is still in my queue, I haven't forgotten about this. I predict this will take a significant amount of time and energy to review, so I'm trying to make sure I'm ready for that. If others would like to review in the meantime that's also of course ok! |
carols10cents
commented
Jul 10, 2017
friendly ping to save up some time and energy, @alexcrichton ! |
alexcrichton
commented
Jul 13, 2017
Unfortunately still haven't had the time to look into this. I'd welcome any others from @rust-lang/libs to help take a look if possible though. @stepancheg if you can also help review by detailing and changes and/or providing an architectural overview that would also be helpful. |
aturon
commented
Jul 14, 2017
r? @aturon @stepancheg I wasn't quite able to tell from your PR description -- are you saying this fixes #39364, or that you were trying to fix it and it no longer reproduces, but you're unsure? In general, as @alexcrichton mentioned reviewing this kind of code is extremely expensive, so we've tended not to take changes unless they are strongly motivated. |
stepancheg
commented
Jul 17, 2017
@aturon yes it fixes the issue #39364. The logic is different now, so that problem is very likely gone now, but I'm not 100% sure that I haven't introduced another bugs. As I said, before this patch code seems to be too complicated, and I believe there's no simple fix for that issue (I understood what was the problem, but couldn't code the fix; to be more precise I fixed it in my local copy, but I couldn't convince myself that it was correct, and I reverted it and instead made this patch). I'll try to write more more explanations of how patch works (a little later). And if anyone's have any specific questions about the patch (what does it do, and why some code is written how it is written), or point to missing docs in code, I'll gladly explain. |
shepmaster
commented
Jul 21, 2017
@aturon do you wish to wait for the further explanation from @stepancheg before doing your review? Otherwise, this is your friendly 3-day ping for review. |
aturon
commented
Jul 25, 2017
Yes, I'd appreciate getting more of an overview. |
aidanhs
commented
Aug 3, 2017
@stepancheg have you had a chance to work on the explanation to help guide review yet? Just checking in! |
arielb1
commented
Aug 8, 2017
🕸 This PR had been inactive for 2 weeks. Closing it to keep our queue clean. Feel free to reopen. 🕸 |
dten
commented
Feb 13, 2018
is this going to make a comeback ? #39364 makes |
`concurrent_recv_timeout_and_upgrade` reproduces a problem 100% times on my MacBook with command: ``` ./x.py test --stage 0 ./src/test/run-pass/mpsc_stress.rs ``` Thus it is commented out. Other tests cases were useful for catching another test cases which may arise during the fix. This diff is a part of my previous rewrite attempt: rust-lang#42883 CC rust-lang#39364
Stress test for MPSC `concurrent_recv_timeout_and_upgrade` reproduces a problem 100% times on my MacBook with command: ``` ./x.py test --stage 0 ./src/test/run-pass/mpsc_stress.rs ``` Thus it is commented out. Other tests cases were useful for catching another test cases which may arise during the fix. This diff is a part of my previous rewrite attempt: rust-lang#42883 CC rust-lang#39364
Stress test for MPSC `concurrent_recv_timeout_and_upgrade` reproduces a problem 100% times on my MacBook with command: ``` ./x.py test --stage 0 ./src/test/run-pass/mpsc_stress.rs ``` Thus it is commented out. Other tests cases were useful for catching another test cases which may arise during the fix. This diff is a part of my previous rewrite attempt: rust-lang#42883 CC rust-lang#39364
Stress test for MPSC `concurrent_recv_timeout_and_upgrade` reproduces a problem 100% times on my MacBook with command: ``` ./x.py test --stage 0 ./src/test/run-pass/mpsc_stress.rs ``` Thus it is commented out. Other tests cases were useful for catching another test cases which may arise during the fix. This diff is a part of my previous rewrite attempt: rust-lang#42883 CC rust-lang#39364
Stress test for MPSC `concurrent_recv_timeout_and_upgrade` reproduces a problem 100% times on my MacBook with command: ``` ./x.py test --stage 0 ./src/test/run-pass/mpsc_stress.rs ``` Thus it is commented out. Other tests cases were useful for catching another test cases which may arise during the fix. This diff is a part of my previous rewrite attempt: rust-lang#42883 CC rust-lang#39364
Previous version had too much state:
cnt,steals,port_droppedfields, and it's too hard to consistently update all of them during
upgrade. I tried to fix issue #39364, but there are too many corner
cases. In this version all of these fields removed, and shared
state is basically managed by two fields:
queueandto_wait.to_wakefield now stores bothSignalTokenand "disconnected"flag.
All tests still pass, and bug #39364 no longer reproduces.
Patch includes a couple of stress tests.
This version should have the same performance characteristics,
because roughly the same number of atomics and wait/notify operations
involved.