<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:x="urn:schemas-microsoft-com:office:excel" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 12 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Verdana;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Verdana","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-GB" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Verdana","sans-serif";color:#1F497D">Thanks for doing a code review .. v helpful.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Verdana","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Verdana","sans-serif";color:#1F497D">Simon<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Verdana","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> ghc-devs-bounces@haskell.org [mailto:ghc-devs-bounces@haskell.org]
<b>On Behalf Of </b>Alexander Kjeldaas<br>
<b>Sent:</b> 31 January 2013 09:31<br>
<b>To:</b> Edward Z. Yang; ghc-devs<br>
<b>Subject:</b> Code review, new scheduler:<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Hi Edward, thanks for your work on the new scheduler!<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I have a done a super light-weight review. I think documenting the code a little more would help readability.<o:p></o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><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><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">TSO.h:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">(Please don't point to a paper as primary documentation.)<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> // 64-bit to prevent overflows; only ever accessed by the task which owns TSO. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 170 StgWord64 ss_pass; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 171 // These are bounded above by STRIDE1, which is less than max 32-bit word. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 172 // You must take out the sched_lock to write to these; reads are OK <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 173 StgWord ss_tickets, ss_stride, ss_remain; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 174 <o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Schedule.c:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 727 StgTSO *t; <o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Rename to 'tso' to be descriptive.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 744 // go through all of the TSOs in the run queue and decide where <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 745 // they should go <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 746 // XXX We can create the new heap more efficiently O(n) by just <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 747 // blitting them in and then re-heapifying <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 748 if (!emptyRunQueue(cap)) { <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 749 StgWord64 k; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Ditto for 'k'.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 1201 scheduleHandleThreadBlocked( Capability *cap, StgTSO *t ) <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Rename 't' to 'tso'.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Schedule.h:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 125 // oh no magic constant <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 126 #define STRIDE1 (1 << 20) <o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Document STRIDE1<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 147 EXTERN_INLINE void <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 148 annulTSO(StgTSO *tso) { <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 149 // hack to make some invariants with regards to block_info and _link work <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 150 // this is called whereever we would have stepped all over the <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 151 // fields in the linked list implementation <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 152 tso->_link = END_TSO_QUEUE; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 153 tso->block_info.closure = (StgClosure*)END_TSO_QUEUE; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">It would be great to have a pointer to the invariants, or the invariant(s) documented.<o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 213 tso->ss_pass += tso->ss_stride; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 214 StgWord64 r; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 215 if (tso->ss_pass <= cap->ss_pass) { <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 216 // Thread is behind, it will get scheduled in front with no <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 217 // intervention (note that cap->ss_pass is probably nonsense, <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 218 // since it doesn't include *this* thread.) <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 219 r = tso->ss_pass; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 220 } else if (tso->ss_pass - tso->ss_pass <= cap->ss_pass) { <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">This expression looks weird/magic, tso->ss_pass - tso->ss_pass is 0.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"> 221 // Thread is in good standing, schedule it in front <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 222 // (next iteration, they will not be in good standing if <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 223 // the global pass doesn't advance by much; that is, this <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 224 // thread managed to cut in front of other threads which <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 225 // are running behind.) <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 226 r = cap->ss_pass; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 227 } else { <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 228 // Thread is not in good standing, schedule it later. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 229 // Eventually, global pass will advance enough that the <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 230 // thread will be in good standing again, and can cut <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 231 // to the front. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 232 r = tso->ss_pass; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Threads.c:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 361 if (migrating) { <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 362 joinRunQueue(cap,tso); <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 363 } else { <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 364 appendToRunQueue(cap,tso); <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 365 } <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Space after ','<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Select.c: and rts/win32/AsyncIO.c<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 309 tso->ss_remain = 0; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 310 joinRunQueue(&MainCapability,tso); <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Should this be an abstraction in itself?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">rts/Capability.h:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> 59 PQueue *run_pqueue; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 60 <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 61 // [SSS] Stride scheduling extensions. The Task with this <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 62 // Capability has exclusive access to this variable. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> 63 nat ss_pass; <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Document ss_pass. For example how it relates to capPassUpdate and pushOnRunQueue (the weird/magic I commented on above) and invariants.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Alexander<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>