Board undo

[lemonsqueeze]

The optimization i was talking about: play() + undo() on the same board gives a nice boost compared to board_copy() + board_play() (~10% on x64, 20% on x86 iirc). Middle ladder reader especially benefits a lot, i've see x6 / x12 speedups depending on hardware. There's a with_move() construct to combine play() + undo(). Syntax is nice and it helps ensure that undo() does get called. There are runtime checks also one can enable (BOARD_UNDO_CHECKS) to ensure board isn't left in an inconsistent state, but in my experience it's not necessary: code asserts out eventually anyway even without it. Since quick_play() doesn't maintain all board fields (hashes, pattern ...) i also put some countermeasures in place to disable them in all tactics code. Lastly i'd rather not consider the possibility of a bug in the undo layer so there's a fairly thorough stress test in t-unit.

[pasky]

1lib.c: In function ‘can_countercapture’: 1lib.c:58:7: error: ‘struct board’ has no member named ‘clen_field_not_supported_for_quick_boards’

[pasky]

I didn't quite understand yet, why do we have to have a separate board implementation for that, can't we have it in a single board impl.? It seems like hard to maintain to have two baord implementations.

[pasky]

Any idea how to make github show the current diff, i.e. not including the test updates?

[lemonsqueeze]

Yes, looks like i need to rebase it on current master now that moggy_test has been merged. Should be better now ...

[lemonsqueeze]

Compile error happens right now because i wanted to keep board_undo and tactics_undo branches separate: board_undo adds generic safety checks for the tactics code but tactics code only gets fixed later in tactics_undo branch.

I could move first commit of tactics_undo here, that'd fix it.

[pasky]

I see. No big deal, we can merge the next PR simultaneously.

But I still feel uncomfortable about having two copies of the board routines and don't understand if that's necessary?

[pasky]

We also need to document what QUICK_BOARD_CODE means.

[pasky]

I also think it might be problematic to have to decide in the whole file which board implementation are you using...

[lemonsqueeze]

I didn't quite understand yet, why do we have to have a separate board implementation for that, can't we have it in a single board impl.? It seems like hard to maintain to have two baord implementations.

I'm not terribly fond of having parts of board.c duplicated either. Basically board_quick_play() is just board_play() without hashes, patterns, traits etc (plus it saves state for undo later). The idea was to avoid computing features that are not needed for the tactics code. So right now it can't just use board_play() because then board_undo() doesn't know how to restore hashes, patterns etc.

We could have only one code path catering to both board_play() and board_quick_play() but then either board_play() or board_undo() is going to get hurt (for example we'd need to if (quick_mode) all the hashes/patterns/etc parts in board_play()).

I think the right way to do it would be an oo approach here: board class derives from quick_board and reuses its code, adding the extra features. But then we'll have casts all over the place ...

[pasky]

I agree having the casts is not great. But I'd like to dig into the notion that having a runtime check is harmful - do you think it really is? I'd say there is so much work to do that the single if() making a big difference would certainly surprise me. What do you think?

I think that this would be a massive simplification of things compared to now, is that notion rigth?

[lemonsqueeze]

Could try, yes. That would certainly make board_undo.c a lot smaller. One thing i like about current implementation: since it's separate it's easy to test / prove it doesn't introduce bugs in board.c. Should be fine now though.

[pasky]

I agree that this isn't that important a consideration anymore. So if this wouldn't be too much hassle for you, I'd really appreciate that change as it should greatly reduce the diff and code duplication.

[lemonsqueeze]

Here it is, tested with tactics_undo rebased on this, speed is almost the same. board.c isn't getting any prettier though, could use some modularity ...

[pasky]

Awesome, thanks! I think this is quite better.

I agree that board.c could use some splitting up... ;-)