Show the real value in the code, not hide it
behind a variable name, especially when there
is only once occurence.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Extend for an extra half-ply in case the node is (probably)
going to fail high. In this case the added overhead is limited.
A novelity is the way this patch has been tested: Always in
self-play but with a much longer TC to allow the singular
extension to fully kick in and also (my impression) to have
less noisy results.
Ater 1015 games on my QUAD at 60"+0.05
Mod vs Orig 173 - 150 - 692 ELO +8
Handle also the SMP case. This has been quite tricky, not
trivial to enforce the node limit in SMP case becuase
with "helpful master" concept we can have recursive split
points and we cannot lock them all at once so there is the
risk of counting the same nodes more than once.
Anyhow this patch should be race free and counted nodes are
correct.
No functional change.
As long as isPvMove (renamed to pvMove) is set after
legality check, we can postpone legality even in PV case.
Patch aligns the PV case with the common non-pv one.
No functional change.
Currently we exit the loop when
abs(bestValue) >= VALUE_KNOWN_WIN
but there is no logical reason for this. It seems more
natural to re-search again with full open window.
This has practically no impact in most cases, we have a
'no functional change' running 'bench' command.
Before the search we setup the starting position doing all the
moves (sent by GUI) from start position to the position just
before to start searching.
To do this we use a set of StateInfo records used by each
do_move() call. These records shall be kept valid during all
the search because repetition draw detection uses them to back
track all the earlier positions keys. The problem is that, while
searching, the GUI could send another 'position' command, this
calls set_position() that clears the states! Of course a crash
follows shortly.
Before searching all the relevant parameters are copied in
start_searching() just for this reason: to fully detach data
accessed during the search from the UCI protocol handling.
So the natural solution would be to copy also the setup states.
Unfortunatly this approach does not work because StateInfo
contains a pointer to the previous record, so naively copying and
then freeing the original memory leads to a crash.
That's why we use two std::auto_ptr (one belonging to UCI and another
to Search) to safely transfer ownership of the StateInfo records to
the search, after we have setup the root position.
As a nice side-effect all the possible memory leaks are magically
sorted out for us by std::auto_ptr semantic.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
To mimics C++11 std::mutex and std::condition_variable,
also rename locks and condition variables to be more
uniform across the classes.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Check we are the last slave of the split point
before to wake up the master. This should avoid
spurious wakes up.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
We can detect the split point master also from within idle_loop,
so we can call the function without parameters and remove an
overloaded member hack in Thread class.
Note that we don't need to take a lock around curSplitPoint
when entering idle_loop() because if we are the master then
curSplitPoint cannot change under our feet (because is_searching
is set and so we cannot be reallocated), if we are a slave
we enter idle_loop() only upon Thread creation and in that case
is always splitPointsCnt == 0. This is true even in the very rare
case that curSplitPoint != NULL, if we have been already allocated
even before entering idle_loop().
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Templetize MovePicker::next_move() member function instead. It
is easier and we also avoid the forwarding of MovePicker() c'tor
arguments in the common case.
Suggested by Rein Halbersma.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
And group there all the formatting functions but
uci_pv() that requires access to search.cpp variables.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Use the helpers to format the PV info but without
writing to output stream (file or cout). Message
formatting and sending are two logically different
task.
Incidentaly reintroduce the pretty_pv() name,
from Glaurung memories :-)
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Simplifies the code and seems more natural.
We have a very small fucntional change becuase now
at PV nodes castles are extended one ply anyhow.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
It seems more accurate: lsb is clear while 'first
bit' depends from where you look at the bitboard.
And fix compile in case of 64 bits platforms that
do not use BSFQ intrinsics.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Although a (little) functional change, we have no ELO change
but formula it is now more clear.
After 13019 games at 30"+0.05
Mod vs Orig 2075 - 2088 - 8856 ELO 0 (+- 3.4)
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
After extensive testing (I was off line this week and let
the test go on) it seems this change is useless:
After 33968 games on a QUAD (4 threads) at 15"+0.05
Mod vs Orig 5425 - 5550 - 22993 ELO -1 (+-2.1)
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
It seems ADL lookup is broken with the STLPort library. Peter says:
The compiler is gcc 4.4.3, but I don't know how many patches they
have applied to it. I think gcc has had support for Koenig lookup
a long time. I think the problem is the type of the vector iterator.
For example, line 272 in search.cpp:
if (bookMove && count(RootMoves.begin(), RootMoves.end(), bookMove))
gives the error:
jni/stockfish/search.cpp:272: error: 'count' was not declared in this scope
Here RootMoves is:
std::vector<RootMove> RootMoves;
If std::vector<T>::iterator is implemented as T*, then Koenig lookup
would fail because RootMove* is not in namespace std.
I compile with the stlport implementation of STL, which in its vector
class has:
typedef value_type* iterator;
I'm not sure if this is allowed by the C++ standard. I did not find
anything that says the iterator type must belong to namespace std.
The consensus in this thread
http://compgroups.net/comp.lang.c++.moderated/argument-dependent-lookup/433395
is that the stlport iterator type is allowed.
Report and patch by Peter Osterlund.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Hex representation doesn't add any value in those cases.
Preserve hex representation where more self-documenting
for instance for binary masks values.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
It seems to increase SMP performances. To note that
this patch goes in the opposite direction of "Active
reparenting" where we try to reparent an idle slave
as soon as possible. Instead here we prefer to keep
it idle instead of splitting on a shallow / near the
leaves node.
After 11550 games on a QUAD (4 threads) at 15"+0.05
Mod vs Orig 1972 - 1752 - 7826 ELO +6 (+-3.6)
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
A PvNode that givesCheck has been already granted
an extension of ONE_PLY in previous condition.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
After 6K games at 60" + 0.1 on QUAD with 4 threads
this implementation fails to show a measurable increase,
result is well within error bar.
Perhaps with 8 or more threads resut is better but we
don't have the hardware to test. So retire for now and
in case re-add in the future if it proves good on big
machines.
The only good news is that we don't have a regression and
implementation is stable and bug-free, so could be reused
somewhere in the future.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
The check for detecting when a split point has all the
slaves still running is done with:
slavesMask == allSlavesMask
When a thread reparents, slavesMask is increased, then, if
the same thread finishes, because there are no more moves,
slavesMask returns to original value and the above condition
returns to be true. So that the just finished thread immediately
reparents again with the same split point, then starts and
then immediately exits in a tight loop that ends only when a
second slave finishes, so that slavesMask decrements and the
condition becomes false. This gives a spurious and anomaly
high number of faked reparents.
With this patch, that rewrites the logic to avoid this pitfall,
the reparenting success rate drops to a more realistical 5-10%
for 4 threads case.
As a side effect note that now there is no more the limit of
maxThreadsPerSplitPoint when reparenting.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Check for a cutoff occurred also high in
the tree and not only at current split
point.
This avoids some more wasted reparenting.
No functional chnage.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
It is more correct given what the function does. In
particular single_bit() returns true also in case of
empty bitboards.
Of course also the usual renaming while there :-)
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Instead of reparenting to oldest split point, try to reparent
to latest. The nice thing here is that we can use the YBWC
helpful master condition to allow the reparenting of a split
point master as long as is reparented to one of its slaves.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
And update master->splitPointsCnt under lock
protection. Not stricly necessary because
single_bit() condition takes care of false
positives anyhow, but it is a bit tricky and
moving under lock is the most natural thing
to do to avoid races with "reparenting".
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
In Young Brothers Wait Concept (YBWC) available slaves are
booked by the split point master, then start to search below
the assigned split point and, once finished, return in idle
state waiting to be booked by another master.
This patch introduces "Active Reparenting" so that when a
slave finishes its job on the assigned split point, instead
of passively waiting to be booked, searches a suitable active
split point and reprents itselfs to that split point. Then
immediately starts to search below the split point in exactly
the same way of the others split point's slaves. This reduces
to zero the time waiting in idle loop and should increase
scalability especially whit many (8 or more) cores.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
It is still enabled during fixed limit search so to
use it during fixed depth/nodes/time matches.
Bug reported by Daylen.
No functional changes.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Shrink dimensions of the biggest stack consumers arrays.
In particular movesSearched[] can be safely shrinked
without any impact on strenght or risk of crashing.
Also MAX_PLY can be reverted to 100 with almost no impact
so to limit search recursion and hence stack allocation.
A different case is for MAX_MOVES (used by Movepicker's
moves[]), because we know that do exsist some artificial
position with about 220 legal moves, so in those cases SF
will crash. Anyhow these cases are never found in games.
An open risk remains perft, especially run above handcrafted
positions.
This patch originates from a report by Daylen that found
SF crashing on his Mac OS X 10.7.3 while in deep analysys
on the following position:
8/3Q1pk1/5p2/4r3/5K2/8/8/8 w - - 0 1
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Unfortunatly accessing thread local variable
is much slower than object data (see previous
patch log msg), so we have to revert to old code
to avoid speed regression.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
Much faster then pthread_getspecific() but still a
speed regression against the original code.
Following are the nps on a bench:
Position
454165
454838
455433
tls
441046
442767
442767
ms (Win)
450521
447510
451105
ms (pthread)
422115
422115
424276
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
After we release the SplitPoint lock the master, suppose
is main thread, can safely return and if a "quit" command
is pending, main thread exits and associated Thread object
is freed. So when we access master->is_searching a crash
occurs.
I have never found such a race that is of course very rare
becuase assumes that from lock releasing we go to sleep for
a time long enough for the main thread to end the search and
return. But you can never know, and anyhow a race is a race.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
But use the newly introduced local storage
for this. A good code semplification and also
the correct way to go.
No functional change.
Signed-off-by: Marco Costalba <mcostalba@gmail.com>
