Re: [gnugo-devel] Patch: new cache part 2

[Inge Wallin]

On Friday 08 August 2003 17:20, Gunnar Farneback wrote:
> Inge wrote:
> > - removed references to read_result in attack() and find_defense()
>
> Isn't it a little premature to remove this code? I'd still like to be
> able to switch between old and new cache. 

Yes, you are probably right.   And besides, it is good to be able to benchmark 
the two approaches against each other.

> > The only thing that I am sure that you will comment on is that I had
> > to separate some macro definitions in hash.h dependent on wether
> > NUM_HASHVALUES is equal to 1 or 2. Take a look and say what you
> > think.
>
> Obviously this still limits MIN_HASHBITS to 64 if long is 32 bits. I
> still don't think it's resolved whether we are satisfied with this.
> Even if a collision is extremely unlikely with 64 bits, it has a
> psychological value to be able to increase it and verify that a weird
> problem indeed is not caused by hash collisions.

I can go along with that, even if it doesn't buy us anything in practice.

> > -  gprintf("%o (%d)", hashval_ng);
> > +  gprintf("%o (%d)", hashdata_to_string(&hashdata));
>
> %d should be changed to %s.

Yes, I must have missed that one.

> > +char *
> > +hashdata_to_string(Hash_data *hashdata)
> > +{
> > +  static char  buffer[17];
> > +
> > +  sprintf(buffer, "%lx", hashdata->hashval[0]);
> > +#if NUM_HASHVALUES == 2
> > +  sprintf(buffer + 8, "%lx", hashdata->hashval[1]);
> > +#endif
> > +
> > +  return buffer;
> >  }
>
> The second sprintf has no (meaningful) effect unless
> hashdata->hashval[0] happens to be big enough to require eight
> hexadecimal digits.

right

> From an earlier patch:
> > +  /* Sanity check. */
> > +  if (remaining_depth < 0)
> > +    remaining_depth = 0;
> > +  if (remaining_depth > HN_MAX_REMAINING_DEPTH)
> > +    remaining_depth = HN_MAX_REMAINING_DEPTH;
>
> I'm uneasy about these. Wouldn't it be better to have assertions here,
> alternatively just return 0?

We could have an assertion against < 0, but why disregard results that are so 
deeply searched that it doesn't even fit into the few bits that we allow?

> Arend wrote:
> > > +  /* Return data.  Only set the result if remaining depth in the table
> > > +   * is big enough to be trusted.  The move can always be used for move
> > > +   * ordering if nothing else.
> > > +   */
> > > +  if ((unsigned) remaining_depth <= hn_get_remaining_depth(*node)) {
> > > +    if (result)
> > > +      *result = hn_get_result1(*node);
> >
> > That's a subtle change in behaviour that also needs discussion
> > (the old one only set the result if "==" in the above equation). I am not
> > against it, but one should be aware that it makes debugging more
> > difficult.
>
> Oh, this is dangerous. I would strongly suggest to continue
> considering the remaining depth as input data and throw it into the
> Zobrist hash. I just tested to change "<=" to "==" above and it does
> at least solve the failure of optics:1201.

No, this is the way that a standard transposition table works in games such as 
chess.  The thinking goes like this:  If we have greater depth left, then the 
answer is likely to have a greater precision, than if it is shallower.  So we 
should always trust deeper searches more.  

I would like to hear the reason why you think that deeper searches should not 
be trusted and why it is dangerous. Did your test also produce any FAILs?  
How many nodes did it cost?  Cost in time always come up when there is a 
patch that strengthens play at the cost of longer run-time, so it should also 
be taken into account when it is the opposite.  

> I still think only the hashing of the actual board should go into
> hash.[ch] and that the hash used as key in the transposition table
> should reside in cache.[ch]. This is also something we can ignore for
> now, however.

I agree.  I will fix that.  It is not a difficult change.

> I have added the patch to CVS except for the changes in reading.c.
> Please go ahead with the integration but leave the possibility to
> switch back to the old one until we have evaluated it fully.

Will do.

        -Inge