Fix cache discrepancy when a job is complete.
ClosedPublic

Authored by TallFurryMan on Sep 3 2018, 7:24 AM.

Details

Summary

When a job completes, capture storage cache is incoherent if job is re-evaluated later than others.

Does NOT fix the regression on sequences embedding duplicated frame types, which cause counts to be incorrect when transferred to Capture.

Test Plan

Test duplicated jobs. When a job finishes, subsequent jobs must not cause the finishing job to loop indefinitely.

Diff Detail

Repository
R321 KStars
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
TallFurryMan created this revision.Sep 3 2018, 7:24 AM
Restricted Application added a project: KDE Edu. · View Herald TranscriptSep 3 2018, 7:24 AM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
TallFurryMan requested review of this revision.Sep 3 2018, 7:24 AM
wreissenberger accepted this revision.Sep 3 2018, 11:18 AM

Looks good, my test cases with the duplicated schedule are running now. Two minor things that I found, but they are not critical:

  • Having two jobs with the same signature both with FINISH_REPEATand the second to run has less cycles than the first one, the second is started and finishes after one iteration. But I think this not a behavior introduced with this fix. Just to be mentioned...
  • When creating newFramesCountthere is no check whether it already contains the signature to be evaluated. This leads to duplicated calls to getCompletedFiles(). Functionally this is OK, but it makes the check slower.
This revision is now accepted and ready to land.Sep 3 2018, 11:18 AM
TallFurryMan planned changes to this revision.Sep 4 2018, 5:41 AM

Looks good, my test cases with the duplicated schedule are running now. Two minor things that I found, but they are not critical:

  • Having two jobs with the same signature both with FINISH_REPEATand the second to run has less cycles than the first one, the second is started and finishes after one iteration. But I think this not a behavior introduced with this fix. Just to be mentioned...

Ah, yes. Probably a regression or a side-effect of an older change. This use case is a bit weird (but possible of course), perhaps we should order duplicates per repeat count?
Also, I'm weighting the possibility to reorder the jobs in the list per startup order, so that it's clearer for the end-user which one will start before the other.

  • When creating newFramesCountthere is no check whether it already contains the signature to be evaluated. This leads to duplicated calls to getCompletedFiles(). Functionally this is OK, but it makes the check slower.

Agreed. Perhaps I can add a check right now.

  • Avoided counting captures for signatures already counted, now that cache is more coherent.
This revision is now accepted and ready to land.Sep 4 2018, 6:21 AM

Looks good, my test cases with the duplicated schedule are running now. Two minor things that I found, but they are not critical:

  • Having two jobs with the same signature both with FINISH_REPEATand the second to run has less cycles than the first one, the second is started and finishes after one iteration. But I think this not a behavior introduced with this fix. Just to be mentioned...

Ah, yes. Probably a regression or a side-effect of an older change. This use case is a bit weird (but possible of course), perhaps we should order duplicates per repeat count?
Also, I'm weighting the possibility to reorder the jobs in the list per startup order, so that it's clearer for the end-user which one will start before the other.

This happens also if you have two identical cycle numbers. The second one starts once and detects after one cycle that it is completed. It seems like there is no check that jobs once scheduled might get completed without a single run.

  • When creating newFramesCountthere is no check whether it already contains the signature to be evaluated. This leads to duplicated calls to getCompletedFiles(). Functionally this is OK, but it makes the check slower.

Agreed. Perhaps I can add a check right now.

OK, checked, working.

Thanks! Now I need to revisit another differential I made to fix the capture storage, because it was trying to work around an issue with the wrong fix.

Issue is apparent when testing with vector "duplicated_scheduler_jobs_duplicated_sequence_jobs_no_twilight".
The count of captured frames is wrong for the second R sequence job, and the third scheduler job, supposed to be run twice, is not running.

But hey wait that's my fault, I left D14928 without answer!! I'm so sorry about this, I was certain this differential was merged. Let me test this as soon as possible.

No sorry, I'm mixing up. I've been hit this night by what D14928 fixes as I was certain FINISH_LOOP would be handled properly, but that is unrelated to the issue I'm describing in my previous comment.
In any case, that's not a reason for me to leave D14928 behind like this!

@mutlaqja Could we merge this soon?

mutlaqja accepted this revision.Sep 11 2018, 2:23 PM
This revision was automatically updated to reflect the committed changes.