Reject time losses mid-run, take 2#1571
Open
dubslow wants to merge 2 commits intoofficial-stockfish:masterfrom
Open
Reject time losses mid-run, take 2#1571dubslow wants to merge 2 commits intoofficial-stockfish:masterfrom
dubslow wants to merge 2 commits intoofficial-stockfish:masterfrom
Conversation
dubslow
force-pushed
the
rejectTimeLosses2
branch
6 times, most recently
from
dab0923 to
0a38d45
Compare
Factor out task_mark_failed Factor out task_mark_bad and task_mark_excessive_residual Cleanups, change behavior of handle_crash_or_time to automatically mark high residual Reorder update_residuals and get_bad_workers Cosmetic: rename bad tasks as purged tasks Slightly tighten allowable timelosses bound
Bad history: 3 of 8 recent tasks are bad I'm not sure about the correctness of this commit, especially regarding locks and concurrency.
dubslow
force-pushed
the
rejectTimeLosses2
branch
from
0a38d45 to
a1d76db
Compare
Contributor
Author
|
I've stored the original, more detailed commit history here: master...dubslow:fishtest:rejectTimeLosses2Orig and instead have forcepushed a simplified history more suitable for merging. |
Contributor
Author
|
I think it's probably a benefit to have "this run was purged of n tasks" as an event log, in parallel with the time or crashes log event, but I'll put that idea off until this one is complete in some form. |
Contributor
Author
|
@vdbergh btw I would very much appreciate your review/feedback of this patch, since among other reasons you're the most familiar with the code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Considerably more sophisticated than my prior attempt some months ago. There's a lot going on here.
The first
6commitsare very nearly non functional, being solely cleanups and refactorings which were necessary for me to even hope to be able to understand what I was modifying (and even then I'm only like 80% sure). Even if the functionality of this PR is rejected, the first6commitsof cleanups and refactoring should be considered/merged independently. (Thethirdcommit is slightly functional, but only the slightest bit, I believe only cosmetically.)The last
twocommitsare the meat of the proposed new functionality. Instead of waiting until the run is "over" to autopurgecrash_or_timetasks (if autopurge is even set, which it usually is not), we purge them immediately, mid-run, if the worker has a recent history of spewing garbage.My usual caveat applies: this is completely untested, and even more than that, I'm not even entirely sure that the last
twocommitsare correct, in the sense of doing what I intended. In any case, I believe it should be easier to understand each commit individually than the grand sum diff. All kinds of review/feedback is requested and appreciated.(Also, I think I found two other potential bugs, not directly related; rather than change them, I merely marked them with comments, to be reconsidered later.)
(Also also, does the rename "bad" --> "purged" imply a db upgrade is required?)