Remove redundancies in symbol declarations #740
Conversation
| @@ -637,7 +632,7 @@ private void DrainLongRunning() | |||
| // delayError: true - because of | |||
| // ObserveOn_LongRunning_HoldUpDuringDispatchAndFail | |||
| // expects something that almost looks like full delayError | |||
| if (DrainStep(q, downstream, true)) | |||
| if (DrainStep(downstream, true)) | |||
There was a problem hiding this comment.
Please don't remove this argument. As the comment states above the atomics inside DrainStep forces the re-read of this queue field instead of keeping it in a register/private stack.
There was a problem hiding this comment.
http://igoro.com/archive/volatile-keyword-in-c-memory-model-explained/
"You may find it surprising that a volatile read refreshes the entire cache, not just the read value. Similarly, a volatile write (i.e., every C# write) flushes the entire cache, not just the written value. These semantics are sometimes referred to as “strong volatile semantics”."
There was a problem hiding this comment.
It's not obvious. _queue is readonly, why does it have to be passed into DrainStep as q when it doesn't touch q at all?
The problem is not that I doubt correctness but that if correctness depends on an unused argument being passed, this should be elaborated on in the comments because this is gonna come up again in R# reviews.
There was a problem hiding this comment.
http://afana.me/archive/2015/07/10/memory-barriers-in-dot-net.aspx/
"Load memory barrier (or read memory barrier)
A load memory barrier ensures that no LOAD operation can move across the barrier. All the LOAD operations that appear before the barrier will appear to happen before all the LOAD operations that appear after the memory barrier. "
So "LOAD _queue" has to happen after any load or full barriers, and _queue.TryDequeue has these types of barriers. So practically dequeueing from it "invalidates" the reference to it and has to be re-read upon next access from memory, even though the reference will be the same.
There was a problem hiding this comment.
unused argument being passed
It is supposed to be used, but apparently there is a mistake in the DrainStep body:
var empty = !_queue.TryDequeue(out var v);should be
var empty = !q.TryDequeue(out var v);There was a problem hiding this comment.
I'll remove this, but it should be commented really well. Somebody will remove it eventually, because this is far from obvious. It's also not good code because it's not intention revealing at all, it misguides sophisticated tools as ReSharper and it's easily broken (by afforementioned reviews).
Please try to work around this or comment it really well.
There was a problem hiding this comment.
If you correct DrainStep as I suggested, the argument won't show up as unused.
There was a problem hiding this comment.
Ok, that does make sense.
There was a problem hiding this comment.
It's restored for now, fix will go into a separate PR.
|
Ok. Could you as well just read |
No description provided.