<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace"><span style="font-family:arial,sans-serif">On Thu, Nov 2, 2017 at 3:39 AM, Nick Coghlan </span><span dir="ltr" style="font-family:arial,sans-serif"><<a href="mailto:ncoghlan@gmail.com" target="_blank">ncoghlan@gmail.com</a>></span><span style="font-family:arial,sans-serif"> wrote:</span><br></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-">On 2 November 2017 at 04:29, Koos Zevenhoven <span dir="ltr"><<a href="mailto:k7hoven@gmail.com" target="_blank">k7hoven@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div style="font-family:monospace,monospace">SPOILER ALERT! At the moment, Nick's statement is in fact <i>*always*</i> true in <i>*all*</i> cases (at least when ignoring untypical cases and some inaccuracies in phrasing). ​Another question is whether the statement <i>*should*</i> be true at all.​<div style="display:inline">​ </div></div></div><div><span style="font-family:monospace,monospace"><div style="font-family:monospace,monospace;display:inline"><br></div></span></div><div><span style="font-family:monospace,monospace"><div style="font-family:monospace,monospace;display:inline">PyErr_CheckSignals(), the function that checks for pending signals, now <i>*implicitly*</i> uses the strictest possible memory-order requirement (SEQ_CST) for checking the `is_tripped` flag, a value which can be used to peek whether there are any pending signals. This means that two threads that call PyErr_CheckSignals can't even <i>*check*</i> the value of that flag at the same time, and they'll have to wait for each other and whatever the CPU hardware implementation does for cache synchronzation. </div></span></div></div></div></div></blockquote><div><br></div></span><div>Nice, that's deeper than I went - I just assumed there was an interlocked_increment in there somewhere, without checking whether or not there were any optimised code paths that managed to avoid that call :)<br></div><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div style="font-family:monospace,monospace">​From a correctness point of view, that is absolutely great: if PyErr_CheckSignals() is called, it is guaranteed to notice a new signal regardles of how small the number of picoseconds after the `is_tripped` flag has been set.  But is that really important? Do we really need things to be slowed down by this? And do we want people to "optimize" code by avoiding signal checking?</div></div></div></div></div></blockquote><div><br></div></span><div>It isn't signal handling latency that's the concern here, it's potentially missing signals. Consider the case where we have 3 threads: A, B, C. A is the main thread that will actually handle signals, B and C both happened to receive them. We'll also assume  the two threads receive *different* signals (otherwise they'll potentially get collapsed into one notification regardless).</div><div><br></div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div>The scenario we want to avoid is having one or both of the signals set, but is_tripped cleared. With an interlocked check (where both 0->1 and 1->0 are memory barriers), that clearly can't happen. I suspect you're right that this could also be achieved with a less strict memory sync requirement on the 0->1 check, but that's much harder to reason about than simply limiting the number of iterations we make through an iterator consumption loop before checking for signals.<br></div><span class="gmail-"><div></div></span></div></div></div></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace">​​Missing signals is a concern, and there's another concern that one received signal might be interpreted as two separate occurrences. But it doesn't prevent us from *checking* is_tripped in a more relaxed manner, as long as the 0->1 check is *verified* using something like SEQ_CST and the associated signal handling is synchronized properly. </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">And that's why my PyErr_PROBE_SIGNALS below should work, and wouldn't prevent multiple threads from handling signals either.</div><div class="gmail_default" style="font-family:monospace,monospace">​<span style="font-family:arial,sans-serif"> </span></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div style="font-family:monospace,monospace">The signals won't be caught until checked anyway, and as we've seen, one solution is to use a counter to determine if enough time has passed that another check for potential signals should happen. That does, however, raise the question of who should own the counter, and if it's OK for multiple threads to use the same counter. If yes, then would we be able to avoid slow atomic decrements (or increments)?</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">But another solution might be to make a less strict but almost equivalent functionality with much less overhead. Something like this in a header file:</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">static inline int PyErr_PROBE_SIGNALS() {</div><div style="font-family:monospace,monospace">    static int volatile *flag = (int volatile *) &is_tripped;</div><div style="font-family:monospace,monospace">    if (*flag) {</div><div style="font-family:monospace,monospace">        return PyErr_CheckSignals();</div><div style="font-family:monospace,monospace">    }</div><div style="font-family:monospace,monospace">    else {</div><div style="font-family:monospace,monospace">        return 0;</div><div style="font-family:monospace,monospace">    }</div><div style="font-family:monospace,monospace">}</div><br></div><div><div style="font-family:monospace,monospace">​Here, the flag is casted to volatile to force a check to happen each time from memory/cache. However, to make it more standard and to make sure it works with all compilers/platforms, it might be better to, instead of *flag, use an atomic load with "MEMORY_ORDER_RELAXED".​ Another thing is that `is_tripped`, which is currently declared as static inside signalmodule.c [4], should somehow be exposed in the API to make it available for the inline function in headers.</div><br></div><div><div style="font-family:monospace,monospace">​This solution might be fast enough for a lot of cases, although still slightly slower than the counter approach, at least if the counter approach would completely avoid per-iteration memory access by requiring each function to own the counter that is used for this.</div></div></div></div></div></blockquote><div><br></div></span>One thing I like about a nested inner loop is that it's easy to understand the rationale for it *without* knowing any of the details of how memory synchronisation works, as it's just:</div><div class="gmail_quote"><br></div><div class="gmail_quote">- we want to check for signals regularly so the latency in interrupt handling isn't too high</div><div class="gmail_quote">- PyErr_CheckSignals() is too expensive to call on every iteration</div><div class="gmail_quote">- so we only check every N iterations<br></div></div><div class="gmail_extra"><br></div></div></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace">My initial "negative" reaction to that potential solution ("<span style="font-size:12.8px">I don't believe nesting loops is strictly a requirement." –K) </span>was because going for manually splitting the loops into smaller inner loops sounds like preparing a nightmare for all C programmers. And I don't think it's possible to write a preprocessor macro that does that for you. Maybe things like Cython could in principle do this automatically, though.</div></div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">Using a counter can have a very small overhead, because things can happen completely in processor registers.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"></div><div class="gmail_extra">Memory synchronisation then only comes up if someone asks why "PyErr_CheckSignals" is expensive to call. And while it wouldn't surprise at all if you're right and there are ways to make that call cheaper, they're still never going to be cheaper than explicitly reducing the frequency with which it is called.<br clear="all"></div><span class="gmail-"><div class="gmail_extra"><br></div></span></div></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace">​There's a limit to how cheap the call to PyErr_CheckSignals() can be. As I mentioned earlier, even just the fact that it's a C function call​ can be too much. </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">That's why, in the above, I used a new name PyErr_PROBE_SIGNALS() instead of optimizing the existing PyErr_CheckSignals() –– turning PyErr_CheckSignals() into a static inline function would change the ABI. I also didn't call it PyErr_CHECK_SIGNALS() because the functionality is not strictly equivalent to the existing function.</div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace">​––Koos</div></div><div><br></div></div><div><br></div>-- <br><div class="gmail_signature">+ Koos Zevenhoven + <a href="http://twitter.com/k7hoven" target="_blank">http://twitter.com/k7hoven</a> +</div>
</div></div>