On Tue, 6 Dec 2011, David T. Lewis wrote:
On Tue, Dec 06, 2011 at 08:18:43AM -0300, Juan Vuletich wrote:
Hi Folks,
Levente Uzonyi wrote:
On Mon, 5 Dec 2011, David T. Lewis wrote:
On Mon, Dec 05, 2011 at 05:02:57PM +0100, Levente Uzonyi wrote:
On Wed, 30 Nov 2011, David T. Lewis wrote:
Given apparent agreement that this is the correct approach, and considering this as a bug fix (not new feature) for ReferenceStream, I have moved Juan's changes to trunk.
After these changes we have five new errors when running all tests:
BitmapStreamTests>>#testMatrixTransform2x3WithImageSegment BitmapStreamTests>>#testShortIntegerArrayWithImageSegment BitmapStreamTests>>#testShortPointArrayWithImageSegment BitmapStreamTests>>#testShortRunArrayWithImageSegment BitmapStreamTests>>#testWordArrayWithImageSegment
The problem seems to be with DiskProxy's serialization, because the cause of the error is that it's not found in the structures dictionary during materialization.
Thanks for catching this.
The problem is certainly in the serialization, because #testMatrixTransform2x3WithImageSegment should be writing a file called 'bitmapStreamTest.extSeg' of size 1549 bytes, but after the ReferenceStream changes the file is only 1380 bytes in size, indicating that something that should have been written to the file is now missing. I'm not sure of the cause though.
I found that the new implementation of ReferenceStream >> #references causes the problems. There are two different issues with it:
- it doesn't return all non-weak references
- it does return a copy
Yes. You're right.
DiskProxy isn't serialized, because it's added to the references as DiskProxy -> #none, so it won't be returned by #references (1), therefore SmartRefStream >> #instVarInfo: won't add it to the structures.
Other classes are not serialized, because (e.g.) ImageSegment >> #storeDataOn: tries to modify aDataStream's references. If that method returns a copy (2), then the modification won't have any effect.
What can we do?
To solve (1) we have to make sure that all non-weak references are returned. It seems to me that weak references have an OrderedCollection as their value in the dictionary, but I don't know what's the reason for that. Anyway, using [ :value | value class ~~ OrderedCollection ] seems to be a possible solution. Another solution is to add a method that returns all references and use it from places where all of them is needed instead of #references.
My first implementation of #references didn't include the special #none symbol, and while it fixed ReferenceStream, it broke SmartRefStream. The attach includes a new one that does include #none. It also includes this hopefully useful comment: "Values can be: - integer: regular reference. Value is position in the output stream. - #none: special value for DiskProxy. Value is just a marker. - A collection of (still) weak only references. They are turned into regular references when the first regular reference to the key is dumped. See #tryToPutReference:typeID:" "Warning: Methods that need to deal with the internals of dumping weak references, or methods that need to actually modify the references dictionary CAN NOT use this method!" "Do not include provisory references created in #nextPutWeak that never became normal references, because the referenced object was never added from a call to #nextPut:"
To solve (2), the best thing to do is to add new methods to ReferenceStream, allowing indirect manipulation of the contents of it's references variable and use these methods from ImageSegment. This would be more explicit than using a method that returns the references dictionary.
Yes. I had already done this in #addSpecialReference: .
What is really needed here is to track all users of the 'references' ivar and all senders of #references, and for each one, decide if it needs (1) the copy without the weak refs, or (2) the full dictionary, possibly to modify it. I did this for Cuis, and this is in my change set, but in Squeak there are many more users of the ivar and senders of the method, so more work is needed.
I also included better tests, that catch the DiskProxy bug I introduced.
Thanks for finding the bug, and for working on this.
Cheers, Juan Vuletich
Thanks Juan and Levente,
I tried adding these changes and it looks like there are a few issues still to be sorted out. I suspect we may want to just back these updates out of trunk until after the Squeak 4.3 release is complete, then put them back and finish any remaining work after the release is done.
comments?
I tried to fix it last night, but ImageSegment breaks the unwritten contract of ReferenceStream, so I'll probably just restore the previous behavior today and we'll fix this issue after the release of 4.3.
Levente
Dave