<table><tr><td style="">TallFurryMan added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D14942">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
          color: #6B748C;
          font-style: italic;
          margin: 4px 0 12px 0;
          padding: 8px 12px;
          background-color: #F8F9FC;">
<div style="font-style: normal;
          padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D14942#317187" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D14942#317187</a>, <a href="https://phabricator.kde.org/p/wreissenberger/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@wreissenberger</a> wrote:</div>
<div style="margin: 0;
          padding: 0;
          border: 0;
          color: rgb(107, 116, 140);"><p>The fix is quite simple. Here is the patch:<a href="https://phabricator.kde.org/F6223164" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">F6223164: looping.patch</a></p></div>
</blockquote>

<p>When I look at your patch, I don't readily understand how, at the point you target, <tt style="background: #ebebeb; font-size: 13px;">newFramesCount</tt> could end up already assigned with a value that is different that what we are going to assign.<br />
So let's analyze.</p>

<p>Map <tt style="background: #ebebeb; font-size: 13px;">newFramesCount</tt> is empty at the beginning of the function.<br />
That map receives a value keyed by a signature when...</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">...the map cache <tt style="background: #ebebeb; font-size: 13px;">capturedFramesCount</tt> does not contain that signature as a key,</li>
<li class="remarkup-list-item">...or for JOB_IDLE and JOB_EVALUATION states, for which we don't trust <tt style="background: #ebebeb; font-size: 13px;">capturedFramesCount</tt> and want to recount.</li>
</ul>

<p>In those two situations, the algorithm calls <tt style="background: #ebebeb; font-size: 13px;">getCompletedFiles</tt> to obtain the storage contents.</p>

<p>Your patch essentially means it is possible for <tt style="background: #ebebeb; font-size: 13px;">capturedFramesCount</tt> to have two different values for the same signature during a single run.<br />
That I agree on, but in order to fix the real issue, the question must be "<em>How is it possible for <tt style="background: #ebebeb; font-size: 13px;">capturedFramesCount</tt> to end up with two different values during the run?</em>".</p>

<p>The reason is, actually, that this function is relying on a cache: <tt style="background: #ebebeb; font-size: 13px;">capturedFramesCount</tt> does not represent the current state of the storage, but is used to avoid the relatively high cost of <tt style="background: #ebebeb; font-size: 13px;">getCompletedFiles</tt>.<br />
When jobs are evaluated, and one job triggers a recount because it is complete, <tt style="background: #ebebeb; font-size: 13px;">capturedFramesCount</tt> will be refreshed with <tt style="background: #ebebeb; font-size: 13px;">getCompletedFiles</tt>, and <em>that will introduce a discrepancy</em> for other jobs that were evaluated before.<br />
Depending on the evaluation order, sometimes we might see that discrepancy, sometimes not.</p>

<p>I believe this is the root cause to fix, and I think your patch does not address that. What do you think?</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R321 KStars</div></div></div><br /><div><strong>BRANCH</strong><div><div>improve__scheduler_image_update (branched from master)</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14942">https://phabricator.kde.org/D14942</a></div></div><br /><div><strong>To: </strong>TallFurryMan, mutlaqja, wreissenberger<br /><strong>Cc: </strong>kde-edu, narvaez, apol<br /></div>