0001591: Chess game crashes squeak

(0002080 - 95 - 95 - 95 - 95 - 95 - 95)
laza
08-02-05 06:49

Seems to be same what happens in Squeak3.8-current-win-full, so this ain't Squeakland specific.

(0002709 - 396 - 580 - 580 - 580 - 580 - 580)
njb
09-25-05 04:27

also observed in 3.9alpha.

In method:

   ChessMoveGenerator::findAllPossibleMovesFor: player

I substituted ByteString for String in the expression:

  String
    findFirstInString: myPieces
    inSet: EmptyPieceMap
    startingAt: square + 1

It seemed to play chess ok after that.
Perhaps this is related to the multibyte string changes?
How does one go about actually 'making the fix'?

(0002710 - 396 - 580 - 580 - 580 - 580 - 580)
njb
09-25-05 04:27

also observed in 3.9alpha.

In method:

   ChessMoveGenerator::findAllPossibleMovesFor: player

I substituted ByteString for String in the expression:

  String
    findFirstInString: myPieces
    inSet: EmptyPieceMap
    startingAt: square + 1

It seemed to play chess ok after that.
Perhaps this is related to the multibyte string changes?
How does one go about actually 'making the fix'?

(0002714 - 53 - 53 - 53 - 53 - 53 - 53)
user291
09-26-05 22:45

Post a changeset and someone here will comment on it.

(0002715 - 120 - 120 - 120 - 234 - 234 - 234)
njb
09-27-05 07:03

Here is a ChangeSet that stops the error notification, and seems to allow normal play to continue. per my comment 0002709.

(0002716 - 46 - 46 - 46 - 46 - 46 - 46)
njb
09-27-05 07:15

oh sorry - I ran the tests in 3.8 and 3.9Alpha

(0002717 - 730 - 742 - 742 - 742 - 742 - 742)
user291
09-27-05 16:11

Thanks. Tested in 3.8 and it worked fine. 3.9a(6693) did not include Chess and I'm not sure if the the version on SM is appropriate to load into 3.9 (maybe that's the one you used, though?).

The original code was a hack, treating a ByteArray as a String, used to improve performance. I think that you're correct that the problems began with refactoring of the String class. String's version of findFirstInString:inSet:startingAt: no longer uses the primitive so it probably wasn't getting the performance benifits desired by the original author. The change to ByteString makes use of the primitive and fixes the bug. Another solution would be to drop this optimization altogether but I don't think that this is necessary.

(0002720 - 256 - 256 - 256 - 256 - 256 - 256)
njb
09-28-05 07:33

Yes I'd loaded 'Games 30-11-2004' from SqueakMap. It did complain when loading it, which I had interpreted as the package not having been tested and 'certified' for 3.9 - is that the correct interpretation? is there an appropriate version of Games for 3.9?

(0002724 - 484 - 494 - 494 - 494 - 494 - 494)
andreas
09-29-05 11:49

The original fix is not so good since the chess move generator effectively abuses a particular primitive for efficiency. Since there is no guarantuee that the prim will stay (or even stay in the same class) it seems better to isolate it inside the move generator, providing proper (and WAY more efficient) fallback code to running the primitive equivalent "just because". The attached CS fixes the problem by doing exactly that, e.g., isolate the prim in CMG and have proper fallback.

(0002725 - 390 - 524 - 524 - 524 - 524 - 524)
njb
09-29-05 15:40

neat.
just for my education; the ByteString method has a construct like:
    <primitive: 'primitiveFindFirstInString' module: 'MiscPrimitivePlugin'>
    self var: #aString declareC: 'unsigned char *aString'.
    self var: #inclusionMap declareC: 'char *inclusionMap'.
but you only used:
    <primitive: 'primitiveFindFirstInString' module: 'MiscPrimitivePlugin'>
why the difference?

(0002727 - 116 - 116 - 116 - 116 - 116 - 116)
andreas
09-29-05 16:10

nbj: #var:declareC: is used only for translating the primitive to C code. It has no effect whatsoever when executed.

(0002737 - 84 - 84 - 84 - 84 - 84 - 84)
ScottWallace
09-30-05 09:36

Andreas's fix has now been published to SqueakAlpha as update 0486chessBugFix-ar.cs.

Notes
(0002080 - 95 - 95 - 95 - 95 - 95 - 95) laza 08-02-05 06:49	Seems to be same what happens in Squeak3.8-current-win-full, so this ain't Squeakland specific.

(0002709 - 396 - 580 - 580 - 580 - 580 - 580) njb 09-25-05 04:27	also observed in 3.9alpha. In method: ChessMoveGenerator::findAllPossibleMovesFor: player I substituted ByteString for String in the expression: String findFirstInString: myPieces inSet: EmptyPieceMap startingAt: square + 1 It seemed to play chess ok after that. Perhaps this is related to the multibyte string changes? How does one go about actually 'making the fix'?

(0002710 - 396 - 580 - 580 - 580 - 580 - 580) njb 09-25-05 04:27	also observed in 3.9alpha. In method: ChessMoveGenerator::findAllPossibleMovesFor: player I substituted ByteString for String in the expression: String findFirstInString: myPieces inSet: EmptyPieceMap startingAt: square + 1 It seemed to play chess ok after that. Perhaps this is related to the multibyte string changes? How does one go about actually 'making the fix'?

(0002714 - 53 - 53 - 53 - 53 - 53 - 53) user291 09-26-05 22:45	Post a changeset and someone here will comment on it.

(0002715 - 120 - 120 - 120 - 234 - 234 - 234) njb 09-27-05 07:03	Here is a ChangeSet that stops the error notification, and seems to allow normal play to continue. per my comment 0002709.

(0002716 - 46 - 46 - 46 - 46 - 46 - 46) njb 09-27-05 07:15	oh sorry - I ran the tests in 3.8 and 3.9Alpha

(0002717 - 730 - 742 - 742 - 742 - 742 - 742) user291 09-27-05 16:11	Thanks. Tested in 3.8 and it worked fine. 3.9a(6693) did not include Chess and I'm not sure if the the version on SM is appropriate to load into 3.9 (maybe that's the one you used, though?). The original code was a hack, treating a ByteArray as a String, used to improve performance. I think that you're correct that the problems began with refactoring of the String class. String's version of findFirstInString:inSet:startingAt: no longer uses the primitive so it probably wasn't getting the performance benifits desired by the original author. The change to ByteString makes use of the primitive and fixes the bug. Another solution would be to drop this optimization altogether but I don't think that this is necessary.

(0002720 - 256 - 256 - 256 - 256 - 256 - 256) njb 09-28-05 07:33	Yes I'd loaded 'Games 30-11-2004' from SqueakMap. It did complain when loading it, which I had interpreted as the package not having been tested and 'certified' for 3.9 - is that the correct interpretation? is there an appropriate version of Games for 3.9?

(0002724 - 484 - 494 - 494 - 494 - 494 - 494) andreas 09-29-05 11:49	The original fix is not so good since the chess move generator effectively abuses a particular primitive for efficiency. Since there is no guarantuee that the prim will stay (or even stay in the same class) it seems better to isolate it inside the move generator, providing proper (and WAY more efficient) fallback code to running the primitive equivalent "just because". The attached CS fixes the problem by doing exactly that, e.g., isolate the prim in CMG and have proper fallback.

(0002725 - 390 - 524 - 524 - 524 - 524 - 524) njb 09-29-05 15:40	neat. just for my education; the ByteString method has a construct like: <primitive: 'primitiveFindFirstInString' module: 'MiscPrimitivePlugin'> self var: #aString declareC: 'unsigned char aString'. self var: #inclusionMap declareC: 'char inclusionMap'. but you only used: <primitive: 'primitiveFindFirstInString' module: 'MiscPrimitivePlugin'> why the difference?

(0002727 - 116 - 116 - 116 - 116 - 116 - 116) andreas 09-29-05 16:10	nbj: #var:declareC: is used only for translating the primitive to C code. It has no effect whatsoever when executed.

(0002737 - 84 - 84 - 84 - 84 - 84 - 84) ScottWallace 09-30-05 09:36	Andreas's fix has now been published to SqueakAlpha as update 0486chessBugFix-ar.cs.

Issue History
Date Modified	Username	Field	Change
08-02-05 06:23	laza	New Issue
08-02-05 06:49	laza	Note Added: 0002080
09-25-05 04:27	njb	Note Added: 0002709
09-25-05 04:27	njb	Note Added: 0002710
09-26-05 22:45	user291	Note Added: 0002714
09-27-05 06:55	njb	File Added: ChessMoveGeneratorFix.1.cs
09-27-05 07:03	njb	Note Added: 0002715
09-27-05 07:15	njb	Note Added: 0002716
09-27-05 16:11	user291	Note Added: 0002717
09-28-05 07:33	njb	Note Added: 0002720
09-29-05 11:46	andreas	File Added: ChessBugFix.1.cs
09-29-05 11:49	andreas	Note Added: 0002724
09-29-05 15:40	njb	Note Added: 0002725
09-29-05 16:10	andreas	Note Added: 0002727
09-30-05 09:36	ScottWallace	Note Added: 0002737
09-30-05 09:36	ScottWallace	Status	new => resolved
09-30-05 09:36	ScottWallace	Resolution	open => fixed
10-08-05 12:28	andreas	Relationship added	related to 0001879