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

[Gunnar Farneback]

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. Turning off USE_HASHTABLE_NG
causes crashes (remainder by zero) after this patch.

> 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.

But well, leave this for now. It's much more exciting to see how the
new cache performs when fully enabled.

> -  gprintf("%o (%d)", hashval_ng);
> +  gprintf("%o (%d)", hashdata_to_string(&hashdata));

%d should be changed to %s.

> +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.

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?

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.

Inge wrote:
> Is there anything else that needs to be reverted or cleaned now? If
> not, I will continue to integrate the new cache into gnugo.

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 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.

/Gunnar