<div dir="ltr"><div><br></div><div style>Hi Edward, thanks for your work on the new scheduler!</div><div style><br></div><div style>I have a done a super light-weight review.  I think documenting the code a little more would help readability.</div>

<br><div><a href="http://hackage.haskell.org/trac/ghc/attachment/ticket/7606/0002-Stride-scheduling-draft-11-thread-migrating-implemen.patch">http://hackage.haskell.org/trac/ghc/attachment/ticket/7606/0002-Stride-scheduling-draft-11-thread-migrating-implemen.patch</a><br>

</div><div><br></div><div style>TSO.h:</div><div style><br></div><div style>Could you document ss_*?  These are all important variables in the scheduler, and not documented.  For example, the code in Threads.c for setting these is can act as some documentation, but something needs to be documented here.</div>

<div style><br></div><div style>(Please don&#39;t point to a paper as primary documentation.)</div><div style><br></div><div><div>    // 64-bit to prevent overflows; only ever accessed by the task which owns TSO. </div><div>

 <span class="" style="white-space:pre">        </span>170<span class="" style="white-space:pre">        </span>    StgWord64 ss_pass; </div><div> <span class="" style="white-space:pre">        </span>171<span class="" style="white-space:pre">        </span>    // These are bounded above by STRIDE1, which is less than max 32-bit word. </div>

<div> <span class="" style="white-space:pre">        </span>172<span class="" style="white-space:pre">        </span>    // You must take out the sched_lock to write to these; reads are OK </div><div> <span class="" style="white-space:pre">        </span>173<span class="" style="white-space:pre">        </span>    StgWord ss_tickets, ss_stride, ss_remain; </div>

<div> <span class="" style="white-space:pre">        </span>174<span class="" style="white-space:pre">        </span> </div></div><div><br></div><div style>Schedule.c:</div><div style><br></div><div><div> <span class="" style="white-space:pre">        </span>727<span class="" style="white-space:pre">        </span>        StgTSO *t; </div>

</div><div style><br></div><div style>Rename to &#39;tso&#39; to be descriptive.</div><div style><br></div><div style><div> <span class="" style="white-space:pre">        </span>744<span class="" style="white-space:pre">        </span>        // go through all of the TSOs in the run queue and decide where </div>

<div> <span class="" style="white-space:pre">        </span>745<span class="" style="white-space:pre">        </span>        // they should go </div><div> <span class="" style="white-space:pre">        </span>746<span class="" style="white-space:pre">        </span>        // XXX We can create the new heap more efficiently O(n) by just </div>

<div> <span class="" style="white-space:pre">        </span>747<span class="" style="white-space:pre">        </span>        // blitting them in and then re-heapifying </div><div> <span class="" style="white-space:pre">        </span>748<span class="" style="white-space:pre">        </span>        if (!emptyRunQueue(cap)) { </div>

<div> <span class="" style="white-space:pre">        </span>749<span class="" style="white-space:pre">        </span>            StgWord64 k; </div><div><br></div><div style>Ditto for &#39;k&#39;.</div><div style><br></div><div style>
<div>
 <span class="" style="white-space:pre">        </span>1201<span class="" style="white-space:pre">        </span>scheduleHandleThreadBlocked( Capability *cap, StgTSO *t ) </div><div><br></div><div style>Rename &#39;t&#39; to &#39;tso&#39;.</div>

<div style><br></div><div style>Schedule.h:</div><div style><br></div><div style><div> <span class="" style="white-space:pre">        </span>125<span class="" style="white-space:pre">        </span>// oh no magic constant </div><div> <span class="" style="white-space:pre">        </span>126<span class="" style="white-space:pre">        </span>#define STRIDE1 (1 &lt;&lt; 20) </div>

</div><div style><br></div><div style>Document STRIDE1</div><div style><br></div><div style><div> <span class="" style="white-space:pre">        </span>147<span class="" style="white-space:pre">        </span>EXTERN_INLINE void </div>
<div>
 <span class="" style="white-space:pre">        </span>148<span class="" style="white-space:pre">        </span>annulTSO(StgTSO *tso) { </div><div> <span class="" style="white-space:pre">        </span>149<span class="" style="white-space:pre">        </span>    // hack to make some invariants with regards to block_info and _link work </div>

<div> <span class="" style="white-space:pre">        </span>150<span class="" style="white-space:pre">        </span>    // this is called whereever we would have stepped all over the </div><div> <span class="" style="white-space:pre">        </span>151<span class="" style="white-space:pre">        </span>    // fields in the linked list implementation </div>

<div> <span class="" style="white-space:pre">        </span>152<span class="" style="white-space:pre">        </span>    tso-&gt;_link = END_TSO_QUEUE; </div><div> <span class="" style="white-space:pre">        </span>153<span class="" style="white-space:pre">        </span>    tso-&gt;block_info.closure = (StgClosure*)END_TSO_QUEUE; </div>

<div><br></div><div style>It would be great to have a pointer to the invariants, or the invariant(s) documented.</div></div><div style><br></div><div style><div> <span class="" style="white-space:pre">        </span>213<span class="" style="white-space:pre">        </span>    tso-&gt;ss_pass += tso-&gt;ss_stride; </div>

<div> <span class="" style="white-space:pre">        </span>214<span class="" style="white-space:pre">        </span>    StgWord64 r; </div><div> <span class="" style="white-space:pre">        </span>215<span class="" style="white-space:pre">        </span>    if (tso-&gt;ss_pass &lt;= cap-&gt;ss_pass) { </div>

<div> <span class="" style="white-space:pre">        </span>216<span class="" style="white-space:pre">        </span>        // Thread is behind, it will get scheduled in front with no </div><div> <span class="" style="white-space:pre">        </span>217<span class="" style="white-space:pre">        </span>        // intervention (note that cap-&gt;ss_pass is probably nonsense, </div>

<div> <span class="" style="white-space:pre">        </span>218<span class="" style="white-space:pre">        </span>        // since it doesn&#39;t include *this* thread.) </div><div> <span class="" style="white-space:pre">        </span>219<span class="" style="white-space:pre">        </span>        r = tso-&gt;ss_pass; </div>

<div> <span class="" style="white-space:pre">        </span>220<span class="" style="white-space:pre">        </span>    } else if (tso-&gt;ss_pass - tso-&gt;ss_pass &lt;= cap-&gt;ss_pass) { </div><div><br></div><div style>This expression looks weird/magic, tso-&gt;ss_pass - tso-&gt;ss_pass is 0.</div>

<div><br></div><div> <span class="" style="white-space:pre">        </span>221<span class="" style="white-space:pre">        </span>        // Thread is in good standing, schedule it in front </div><div> <span class="" style="white-space:pre">        </span>222<span class="" style="white-space:pre">        </span>        // (next iteration, they will not be in good standing if </div>

<div> <span class="" style="white-space:pre">        </span>223<span class="" style="white-space:pre">        </span>        // the global pass doesn&#39;t advance by much; that is, this </div><div> <span class="" style="white-space:pre">        </span>224<span class="" style="white-space:pre">        </span>        // thread managed to cut in front of other threads which </div>

<div> <span class="" style="white-space:pre">        </span>225<span class="" style="white-space:pre">        </span>        // are running behind.) </div><div> <span class="" style="white-space:pre">        </span>226<span class="" style="white-space:pre">        </span>        r = cap-&gt;ss_pass; </div>

<div> <span class="" style="white-space:pre">        </span>227<span class="" style="white-space:pre">        </span>    } else { </div><div> <span class="" style="white-space:pre">        </span>228<span class="" style="white-space:pre">        </span>        // Thread is not in good standing, schedule it later. </div>

<div> <span class="" style="white-space:pre">        </span>229<span class="" style="white-space:pre">        </span>        // Eventually, global pass will advance enough that the </div><div> <span class="" style="white-space:pre">        </span>230<span class="" style="white-space:pre">        </span>        // thread will be in good standing again, and can cut </div>

<div> <span class="" style="white-space:pre">        </span>231<span class="" style="white-space:pre">        </span>        // to the front. </div><div> <span class="" style="white-space:pre">        </span>232<span class="" style="white-space:pre">        </span>        r = tso-&gt;ss_pass; </div>

<div><br></div></div><div style><br></div><div style>Threads.c:</div><div style><br></div><div style><div> <span class="" style="white-space:pre">        </span>361<span class="" style="white-space:pre">        </span>    if (migrating) { </div>

<div> <span class="" style="white-space:pre">        </span>362<span class="" style="white-space:pre">        </span>        joinRunQueue(cap,tso); </div><div> <span class="" style="white-space:pre">        </span>363<span class="" style="white-space:pre">        </span>    } else { </div>

<div> <span class="" style="white-space:pre">        </span>364<span class="" style="white-space:pre">        </span>        appendToRunQueue(cap,tso); </div><div> <span class="" style="white-space:pre">        </span>365<span class="" style="white-space:pre">        </span>    } </div>

<div style><br></div><div style>Space after &#39;,&#39;</div><div style><br></div><div style>Select.c: and rts/win32/AsyncIO.c</div><div style><br></div><div style><div> <span class="" style="white-space:pre">        </span>309<span class="" style="white-space:pre">        </span>                  tso-&gt;ss_remain = 0; </div>

<div> <span class="" style="white-space:pre">        </span>310<span class="" style="white-space:pre">        </span>                  joinRunQueue(&amp;MainCapability,tso); </div><div><br></div><div style>Should this be an abstraction in itself?</div>

<div style><br></div><div style><br></div><div style>rts/Capability.h:</div><div style><br></div><div style><div> <span class="" style="white-space:pre">        </span>59<span class="" style="white-space:pre">        </span>    PQueue *run_pqueue; </div>

<div> <span class="" style="white-space:pre">        </span>60<span class="" style="white-space:pre">        </span> </div><div> <span class="" style="white-space:pre">        </span>61<span class="" style="white-space:pre">        </span>    // [SSS] Stride scheduling extensions.  The Task with this </div>

<div> <span class="" style="white-space:pre">        </span>62<span class="" style="white-space:pre">        </span>    // Capability has exclusive access to this variable. </div><div> <span class="" style="white-space:pre">        </span>63<span class="" style="white-space:pre">        </span>    nat ss_pass; </div>

<div><br></div><div style>Document ss_pass.  For example how it relates to capPassUpdate and pushOnRunQueue (the weird/magic I commented on above) and invariants.</div><div style><br></div><div style><br></div><div style>

Alexander</div><div style><br></div><div style><br></div></div></div></div></div></div></div>