[squeak-dev] [Bug][Fix] In ReferenceStream, code review request

Juan Vuletich juan at jvuletich.org
Tue Dec 6 11:18:43 UTC 2011


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:
> 1) it doesn't return all non-weak references
> 2) 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

>
>
> Levente
>
>>
>> Dave
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1164-ReferenceStreamFix-jmv.1.cs.zip
Type: application/zip
Size: 2973 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20111206/7079090d/1164-ReferenceStreamFix-jmv.1.cs.zip


More information about the Squeak-dev mailing list