Simplify pondering time management #1899
Conversation
|
meanwhile the last bit of data, still looking OK: Score of ponderSimp vs master: 2616 - 2528 - 8630 [0.503] 13774 |
|
This is a difficult one, the subject is very tricky. Sorry for the delay in the feedback, but I need to find some time where I can calmly focus on this one. For this topic (multi thread, race conditions, etc.), test results can cover until a certain point, and you really need some good code review by human eyes, possibly more than 2. |
|
thanks, sure, this needs two pair of eyes. This should actually reduce the risk of races (stopOnPonderHit is now only managed by mainThread). |
|
You move ponder hit evaluation under main thread, but you leave Threads.ponder as a shared variable. Why this? It seems half baked concept to me, eventually try to move all time checking under main thread and just leave the stop as a shared signal. As a side note, while there I noted that in Thread::search() there is a big chunk of time management code that is executed only by mainThread. It is suboptimal to me to leave it there. Eventually we should find an elegant way to move it under MainThread, this would help to clarify that only main thread handles the time management. I see we already have;
Eventually we could add a new
and move time management at root there. This of course would be a separate patch. |
|
@mcostalba agree with both points, and had them on my mental todo list. I will prepare the change to ponder and make it part of this commit if you think this one is otherwise OK. |
|
@mcostalba change made. |
|
Ok thanks. Because I guess we may want to retest anyhow to check for some odd blunder, I would suggest to add also the refactoring in the same PR but on a separated commit, if there's no regression both commits could be merged keeping them splitted (a novelty :-) ), so to avoid a double consuming local test. I'd suggest to write the refactoring and wait for me before to rerun the rest, so that first patch is approved and test is just a safety check |
|
@mcostalba your comment above refers to the refactoring towards a 'check_time_root', I assume? Since it is also conceptually unrelated, and it can be somewhat meaningfully tested on fishtest, I'd rather keep it separate. I did do a quick test on the latest commit in this PR, but since it is essentially a trivial renaming stopped it after a few games that looked just fine. |
|
Ok thanks. I am on my way now, I will check it later today. |
|
Ok, it seems good to me. Thanks. |
stopOnPonderhit is used to stop search quickly on a ponderhit. It is set by mainThread as part of its time management. However, master employs it as a signal between mainThread and the UCI thread. This is not necessary, it is sufficient for the UCI thread to signal that pondering finished, and mainThread should do its usual time-keeping job, and in this case stop immediately. This patch implements this, removing stopOnPonderHit as an atomic variable from the ThreadPool, and moving it as a normal variable to mainThread, reducing its scope. In MainThread::check_time() the search is stopped immediately if ponder switches to false, and the variable stopOnPonderHit is set. Furthermore, ponder has been moved to mainThread, as the variable is only used to exchange signals between the UCI thread and mainThread. The version has been tested locally (as fishtest doesn't support ponder): Score of ponderSimp vs master: 2616 - 2528 - 8630 [0.503] 13774 Elo difference: 2.22 +/- 3.54 which indicates no regression. No functional change.
|
I've squashed all commits into a single one, on top of current master. Anything else holding back a merge? |
|
@vondele thanks! merged. |
|
Do this patch increase overall strength when ponder is enabled? |
|
@Nordlandia no there is no difference from a user point of view. |
3 participants
stopOnPonderhit is used to stop search quickly on a ponderhit. It is set by mainThread as part of its time management. However, master employs it as a signal between mainThread and the UCI thread. This is not necessary, it is sufficient for the UCI thread to signal that pondering finished, and mainThread should do its usual time-keeping job, and in this case stop immediately.
This patch implements this, removing stopOnPonderHit as an atomic variable from the ThreadPool,
and moving it as a normal variable to mainThread, reducing its scope. In MainThread::check_time() the search is stopped immediately if ponder switches to false, and the variable stopOnPonderHit is set.
Local testing with pondering on (pondering on not support on fishtest) at 10+0.1 shows nothing unexpected:
Score of ponderSimp vs master: 660 - 660 - 2140 [0.500] 3460
Elo difference: 0.00 +/- 7.14
No functional change.