Hi Folks,
I found a bug in Cuis that made weak references to be improperly dumped in SmartRefStreams due to a bug in ReferenceStream. When I tried to reproduce the bug and fix in Squeak, I found out that in Squeak weak references are always dumped (and not just the more subtle bug in Cuis). The attach includes a test and fix. I'm confident about the fix in the context of Cuis, but I only played a few minutes with this in Squeak.
Can anybody check this for Squeak and consider it for inclusion?
Thanks, Juan Vuletich
'From Squeak4.2 of 20 November 2011 [latest update: #11796] on 27 November 2011 at 2:56:53 pm'! TestCase subclass: #ReferenceStreamTest instanceVariableNames: '' classVariableNames: '' poolDictionaries: '' category: 'System-Object Storage-Tests'!
!Object methodsFor: 'objects from disk' stamp: 'jmv 11/27/2011 14:56'! storeDataOn: aDataStream "Store myself on a DataStream. Answer self. This is a low-level DataStream/ReferenceStream method. See also objectToStoreOnDataStream. NOTE: This method must send 'aDataStream beginInstance:size:' and then (nextPut:/nextPutWeak:) its subobjects. readDataFrom:size: reads back what we write here." | cntInstVars cntIndexedVars |
cntInstVars := self class instSize. cntIndexedVars := self basicSize. aDataStream beginInstance: self class size: cntInstVars + cntIndexedVars. 1 to: cntInstVars do: [:i | aDataStream nextPut: (self instVarAt: i)].
"Write fields of a variable length object. When writing to a dummy stream, don't bother to write the bytes" ((aDataStream byteStream class == DummyStream) and: [self class isBits]) ifFalse: [ self class isWeak ifTrue: [ "For weak classes (for example DependentsArray) write the referenced object only if referenced from elsewhere in the dumped object graph. This means, for instance that if we only dump a model, no dependents are stored, but if we store a view (i.e. a Morph), it is properly handled as a dependent after the object graph is revived." 1 to: cntIndexedVars do: [ :i | aDataStream nextPutWeak: (self basicAt: i)]] ifFalse: [ 1 to: cntIndexedVars do: [ :i | aDataStream nextPut: (self basicAt: i)]]]! !
!DiskProxy methodsFor: 'i/o' stamp: 'jmv 11/25/2011 13:20'! storeDataOn: aReferenceStream "Besides just storing, get me inserted into references, so structures will know about class DiskProxy."
super storeDataOn: aReferenceStream.
"just so instVarInfo: will find it and put it into structures" " aReferenceStream references at: self put: #none." aReferenceStream addSpecialReference: self! !
!ReferenceStream methodsFor: 'statistics' stamp: 'jmv 11/25/2011 13:21'! statisticsOfRefs "Analyze the information in references, the objects being written out"
| parents ownerBags tallies n nm owners normalReferences | normalReferences := self references. "Exclude unrealized weaks" parents := IdentityDictionary new: normalReferences size * 2. n := 0. 'Finding Owners...' displayProgressFrom: 0 to: normalReferences size during: [:bar | normalReferences keysDo: [:parent | | kids | bar value: (n := n+1). kids := parent class isFixed ifTrue: [(1 to: parent class instSize) collect: [:i | parent instVarAt: i]] ifFalse: [parent class isBits ifTrue: [Array new] ifFalse: [(1 to: parent basicSize) collect: [:i | parent basicAt: i]]]. (kids select: [:x | normalReferences includesKey: x]) do: [:child | parents at: child put: parent]]]. ownerBags := Dictionary new. tallies := Bag new. n := 0. 'Tallying Owners...' displayProgressFrom: 0 to: normalReferences size during: [:bar | normalReferences keysDo: "For each class of obj, tally a bag of owner classes" [:obj | | objParent | bar value: (n := n+1). nm := obj class name. tallies add: nm. owners := ownerBags at: nm ifAbsent: [ownerBags at: nm put: Bag new]. (objParent := parents at: obj ifAbsent: [nil]) == nil ifFalse: [owners add: objParent class name]]]. ^ String streamContents: [:strm | tallies sortedCounts do: [:assn | n := assn key. nm := assn value. owners := ownerBags at: nm. strm cr; nextPutAll: nm; space; print: n. owners size > 0 ifTrue: [strm cr; tab; print: owners sortedCounts]]]! !
!ReferenceStream methodsFor: 'writing' stamp: 'jmv 11/25/2011 13:20'! addSpecialReference: aDiskProxy "See senders. Added to avoid breaking encapsulation (assuming that #references would answer the actual collection)" references at: aDiskProxy put: #none! !
!ReferenceStream methodsFor: 'writing' stamp: 'jmv 11/27/2011 14:56'! references "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:" ^ references select: [ :value | value isNumber ]! !
!ReferenceStreamTest methodsFor: 'testing' stamp: 'jmv 11/27/2011 14:56'! testWeakDumps "Test that if we serialize a model with weak references to views, only the model is serialized and not the views. Note: The bug became apparent only when dumping a model to a SmartRefStream, that calls #references, and the serialized stream was later materialized in an image where the view classes had been deleted. In such rare cases, materialization would fail when trying to reference these absent classes. If serializing to a ReferenceStream, the bug didn't become apparent (views were never serialized). If serializing to a SmartRefStream, but view classes still existed, the bug didn't really become apparent (because views were not actually deserialized), the only effect was a larger file. ReferenceStreamTest new testWeakDumps " | oldInstance refStream | oldInstance :=StringHolder new contents: 'This is a text'. oldInstance addDependent: Morph new. refStream := ReferenceStream on: (DummyStream on: nil). refStream nextPut: oldInstance. self deny: (refStream references keys anySatisfy: [ :dumpedObject | dumpedObject isKindOf: Morph ])! !
On Sun, Nov 27, 2011 at 02:28:23PM -0300, Juan Vuletich wrote:
Hi Folks,
I found a bug in Cuis that made weak references to be improperly dumped in SmartRefStreams due to a bug in ReferenceStream. When I tried to reproduce the bug and fix in Squeak, I found out that in Squeak weak references are always dumped (and not just the more subtle bug in Cuis). The attach includes a test and fix. I'm confident about the fix in the context of Cuis, but I only played a few minutes with this in Squeak.
Can anybody check this for Squeak and consider it for inclusion?
Thanks, Juan Vuletich
This is in the inbox as Kernel-dtl.655.mcz, System-dtl.461.mcz, and Tests-dtl.136.mcz.
I cleaned up a few <cr><lf> issues in the change set, and also moved the unit test into category 'Tests-System-Object Storage' because ReferenceStream is in category: 'System-Object Storage'.
The changes look good to me. We are in feature freeze for Squeak 4.3, so is it OK to move this to trunk?
Dave
Hi. I am not sure if I understand which is the bug and it is difficult to do a clear diff. You mean that weak references should NOT be serialized ? While thinking about this in Fuel I asked in the mailing listhttp://forum.world.st/Should-a-serializer-do-something-in-particular-with-weak-references-td3827593.htmland the conclusion was that we shouldn't do anything special in Fuel for weak references. In this case, if I understand, you are saying the opposite: do something different for weak references, basically to consider them "transient". Can you please explain me why it is needed a specific behavior for weak references?
Thanks in advance!
On Sun, Nov 27, 2011 at 6:28 PM, Juan Vuletich juan@jvuletich.org wrote:
Hi Folks,
I found a bug in Cuis that made weak references to be improperly dumped in SmartRefStreams due to a bug in ReferenceStream. When I tried to reproduce the bug and fix in Squeak, I found out that in Squeak weak references are always dumped (and not just the more subtle bug in Cuis). The attach includes a test and fix. I'm confident about the fix in the context of Cuis, but I only played a few minutes with this in Squeak.
Can anybody check this for Squeak and consider it for inclusion?
Thanks, Juan Vuletich
'From Squeak4.2 of 20 November 2011 [latest update: #11796] on 27 November 2011 at 2:56:53 pm'! TestCase subclass: #ReferenceStreamTest instanceVariableNames: '' classVariableNames: '' poolDictionaries: '' category: 'System-Object Storage-Tests'!
!Object methodsFor: 'objects from disk' stamp: 'jmv 11/27/2011 14:56'! storeDataOn: aDataStream "Store myself on a DataStream. Answer self. This is a low-level DataStream/ReferenceStream method. See also objectToStoreOnDataStream. NOTE: This method must send 'aDataStream beginInstance:size:' and then (nextPut:/nextPutWeak:) its subobjects. readDataFrom:size: reads back what we write here." | cntInstVars cntIndexedVars |
cntInstVars := self class instSize. cntIndexedVars := self basicSize. aDataStream beginInstance: self class size: cntInstVars + cntIndexedVars. 1 to: cntInstVars do: [:i | aDataStream nextPut: (self instVarAt: i)]. "Write fields of a variable length object. When writing to a dummy stream, don't bother to write the bytes" ((aDataStream byteStream class == DummyStream) and: [self class
isBits]) ifFalse: [ self class isWeak ifTrue: [ "For weak classes (for example DependentsArray) write the referenced object only if referenced from elsewhere in the dumped object graph. This means, for instance that if we only dump a model, no dependents are stored, but if we store a view (i.e. a Morph), it is properly handled as a dependent after the object graph is revived." 1 to: cntIndexedVars do: [ :i | aDataStream nextPutWeak: (self basicAt: i)]] ifFalse: [ 1 to: cntIndexedVars do: [ :i | aDataStream nextPut: (self basicAt: i)]]]! !
!DiskProxy methodsFor: 'i/o' stamp: 'jmv 11/25/2011 13:20'! storeDataOn: aReferenceStream "Besides just storing, get me inserted into references, so structures will know about class DiskProxy."
super storeDataOn: aReferenceStream. "just so instVarInfo: will find it and put it into structures"
" aReferenceStream references at: self put: #none." aReferenceStream addSpecialReference: self! !
!ReferenceStream methodsFor: 'statistics' stamp: 'jmv 11/25/2011 13:21'! statisticsOfRefs "Analyze the information in references, the objects being written out"
| parents ownerBags tallies n nm owners normalReferences | normalReferences := self references. "Exclude unrealized weaks" parents := IdentityDictionary new: normalReferences size * 2. n := 0. 'Finding Owners...' displayProgressFrom: 0 to: normalReferences size during: [:bar | normalReferences keysDo: [:parent | | kids | bar value: (n := n+1). kids := parent class isFixed ifTrue: [(1 to: parent class instSize) collect: [:i
| parent instVarAt: i]] ifFalse: [parent class isBits ifTrue: [Array new] ifFalse: [(1 to: parent basicSize) collect: [:i | parent basicAt: i]]]. (kids select: [:x | normalReferences includesKey: x]) do: [:child | parents at: child put: parent]]]. ownerBags := Dictionary new. tallies := Bag new. n := 0. 'Tallying Owners...' displayProgressFrom: 0 to: normalReferences size during: [:bar | normalReferences keysDo: "For each class of obj, tally a bag of owner classes" [:obj | | objParent | bar value: (n := n+1). nm := obj class name. tallies add: nm. owners := ownerBags at: nm ifAbsent: [ownerBags at: nm put: Bag new]. (objParent := parents at: obj ifAbsent: [nil]) == nil ifFalse: [owners add: objParent class name]]]. ^ String streamContents: [:strm | tallies sortedCounts do: [:assn | n := assn key. nm := assn value. owners := ownerBags at: nm. strm cr; nextPutAll: nm; space; print: n. owners size > 0 ifTrue: [strm cr; tab; print: owners sortedCounts]]]! !
!ReferenceStream methodsFor: 'writing' stamp: 'jmv 11/25/2011 13:20'! addSpecialReference: aDiskProxy "See senders. Added to avoid breaking encapsulation (assuming that #references would answer the actual collection)" references at: aDiskProxy put: #none! !
!ReferenceStream methodsFor: 'writing' stamp: 'jmv 11/27/2011 14:56'! references "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:" ^ references select: [ :value | value isNumber ]! !
!ReferenceStreamTest methodsFor: 'testing' stamp: 'jmv 11/27/2011 14:56'! testWeakDumps "Test that if we serialize a model with weak references to views, only the model is serialized and not the views.
Note: The bug became apparent only when dumping a model to a
SmartRefStream, that calls #references, and the serialized stream was later materialized in an image where the view classes had been deleted. In such rare cases, materialization would fail when trying to reference these absent classes. If serializing to a ReferenceStream, the bug didn't become apparent (views were never serialized). If serializing to a SmartRefStream, but view classes still existed, the bug didn't really become apparent (because views were not actually deserialized), the only effect was a larger file.
ReferenceStreamTest new testWeakDumps " | oldInstance refStream | oldInstance :=StringHolder new contents: 'This is a text'. oldInstance addDependent: Morph new. refStream := ReferenceStream on: (DummyStream on: nil). refStream nextPut: oldInstance. self deny: (refStream references keys anySatisfy: [ :dumpedObject |
dumpedObject isKindOf: Morph ])! !
Hi Mariano,
Mariano Martinez Peck wrote:
Hi. I am not sure if I understand which is the bug and it is difficult to do a clear diff. You mean that weak references should NOT be serialized ?
The test below documents the correct behavior. See implementors and senders of #nextPutWeak: in Cuis. Those methods have _very_ good comments. If you only dump a model, any views should not get dumped. If you dump a view, both view and model must be dumped. Basic View / Model separation: the View knows the Model, the Model doesn't know about the View. In the case of Morph, the owner of a morph dumped by itself should not be included; but the owner / submorphs relationship must be correct for all morphs that ultimately get dumped. The subtler bug in Cuis was in #references, and is also caught by this test.
While thinking about this in Fuel I asked in the mailing list http://forum.world.st/Should-a-serializer-do-something-in-particular-with-weak-references-td3827593.html and the conclusion was that we shouldn't do anything special in Fuel for weak references.
Wrong conclusion. The correct answer was Levente's: http://forum.world.st/Should-a-serializer-do-something-in-particular-with-we... ..
In this case, if I understand, you are saying the opposite: do something different for weak references, basically to consider them "transient". Can you please explain me why it is needed a specific behavior for weak references?
Including objects that are only weakly referenced from within the dumped object graph (but strongly referenced from somewhere else) is useless: very shortly after materialization they will go away. You're wasting serialized file size storing garbage, and requesting that these objects can be materialized (their class or some appropriate replacement must be present, etc). But a serialized model should be materializable in, for instance, an UI-less server, or some client with a very different UI framework, etc. This would enable (among many other useful things) sharing serialized objects between Cuis, Squeak and Pharo.
Thanks in advance!
Cheers, Juan Vuletich
On Sun, Nov 27, 2011 at 6:28 PM, Juan Vuletich <juan@jvuletich.org mailto:juan@jvuletich.org> wrote:
Hi Folks, I found a bug in Cuis that made weak references to be improperly dumped in SmartRefStreams due to a bug in ReferenceStream. When I tried to reproduce the bug and fix in Squeak, I found out that in Squeak weak references are always dumped (and not just the more subtle bug in Cuis). The attach includes a test and fix. I'm confident about the fix in the context of Cuis, but I only played a few minutes with this in Squeak. Can anybody check this for Squeak and consider it for inclusion? Thanks, Juan Vuletich ... !ReferenceStreamTest methodsFor: 'testing' stamp: 'jmv 11/27/2011 14:56'! testWeakDumps "Test that if we serialize a model with weak references to views, only the model is serialized and not the views. Note: The bug became apparent only when dumping a model to a SmartRefStream, that calls #references, and the serialized stream was later materialized in an image where the view classes had been deleted. In such rare cases, materialization would fail when trying to reference these absent classes. If serializing to a ReferenceStream, the bug didn't become apparent (views were never serialized). If serializing to a SmartRefStream, but view classes still existed, the bug didn't really become apparent (because views were not actually deserialized), the only effect was a larger file. ReferenceStreamTest new testWeakDumps " | oldInstance refStream | oldInstance :=StringHolder new contents: 'This is a text'. oldInstance addDependent: Morph new. refStream := ReferenceStream on: (DummyStream on: nil). refStream nextPut: oldInstance. self deny: (refStream references keys anySatisfy: [ :dumpedObject | dumpedObject isKindOf: Morph ])! !
-- Mariano http://marianopeck.wordpress.com
Hello
On Tue, Nov 29, 2011 at 7:41 PM, Juan Vuletich juan@jvuletich.org wrote:
Hi Mariano,
Mariano Martinez Peck wrote:
Hi. I am not sure if I understand which is the bug and it is difficult to do a clear diff. You mean that weak references should NOT be serialized ?
The test below documents the correct behavior. See implementors and senders of #nextPutWeak: in Cuis. Those methods have _very_ good comments. If you only dump a model, any views should not get dumped. If you dump a view, both view and model must be dumped. Basic View / Model separation: the View knows the Model, the Model doesn't know about the View. In the case of Morph, the owner of a morph dumped by itself should not be included; but the owner / submorphs relationship must be correct for all morphs that ultimately get dumped. The subtler bug in Cuis was in #references, and is also caught by this test.
Thnaks, now I understand Levente's answer. I figure out how to implement it on Fuel, it should be easy.
But a serialized model should be materializable in, for instance, an UI-less server, or some client with a very different UI framework, etc. This would enable (among many other useful things) sharing serialized objects between Cuis, Squeak and Pharo.
Sorry, let me see if I understood: You mean that as the model has references to UI objects but they are just weak references, then "Serializer serialize: model" should not include UI objects and thus you can transport the model with UI-framework independence. Am I right?
Cheers, Martín
Thanks in advance!
Cheers, Juan Vuletich
On Sun, Nov 27, 2011 at 6:28 PM, Juan Vuletich <juan@jvuletich.org<mailto:
juan@jvuletich.org>> wrote:
Hi Folks,
I found a bug in Cuis that made weak references to be improperly dumped in SmartRefStreams due to a bug in ReferenceStream. When I tried to reproduce the bug and fix in Squeak, I found out that in Squeak weak references are always dumped (and not just the more subtle bug in Cuis). The attach includes a test and fix. I'm confident about the fix in the context of Cuis, but I only played a few minutes with this in Squeak.
Can anybody check this for Squeak and consider it for inclusion?
Thanks, Juan Vuletich
...
!ReferenceStreamTest methodsFor: 'testing' stamp: 'jmv 11/27/2011 14:56'! testWeakDumps "Test that if we serialize a model with weak references to views, only the model is serialized and not the views.
Note: The bug became apparent only when dumping a model to
a SmartRefStream, that calls #references, and the serialized stream was later materialized in an image where the view classes had been deleted. In such rare cases, materialization would fail when trying to reference these absent classes. If serializing to a ReferenceStream, the bug didn't become apparent (views were never serialized). If serializing to a SmartRefStream, but view classes still existed, the bug didn't really become apparent (because views were not actually deserialized), the only effect was a larger file.
ReferenceStreamTest new testWeakDumps " | oldInstance refStream | oldInstance :=StringHolder new contents: 'This is a text'. oldInstance addDependent: Morph new. refStream := ReferenceStream on: (DummyStream on: nil). refStream nextPut: oldInstance. self deny: (refStream references keys anySatisfy: [
:dumpedObject | dumpedObject isKindOf: Morph ])! !
-- Mariano http://marianopeck.wordpress.**com http://marianopeck.wordpress.com
On Wed, Nov 30, 2011 at 03:17:52AM -0300, Martin Dias wrote:
Hello
On Tue, Nov 29, 2011 at 7:41 PM, Juan Vuletich juan@jvuletich.org wrote:
Hi Mariano,
Mariano Martinez Peck wrote:
Hi. I am not sure if I understand which is the bug and it is difficult to do a clear diff. You mean that weak references should NOT be serialized ?
The test below documents the correct behavior. See implementors and senders of #nextPutWeak: in Cuis. Those methods have _very_ good comments. If you only dump a model, any views should not get dumped. If you dump a view, both view and model must be dumped. Basic View / Model separation: the View knows the Model, the Model doesn't know about the View. In the case of Morph, the owner of a morph dumped by itself should not be included; but the owner / submorphs relationship must be correct for all morphs that ultimately get dumped. The subtler bug in Cuis was in #references, and is also caught by this test.
Thnaks, now I understand Levente's answer. I figure out how to implement it on Fuel, it should be easy.
Thanks all,
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.
Dave
On Wed, 30 Nov 2011, David T. Lewis wrote:
On Wed, Nov 30, 2011 at 03:17:52AM -0300, Martin Dias wrote:
Hello
On Tue, Nov 29, 2011 at 7:41 PM, Juan Vuletich juan@jvuletich.org wrote:
Hi Mariano,
Mariano Martinez Peck wrote:
Hi. I am not sure if I understand which is the bug and it is difficult to do a clear diff. You mean that weak references should NOT be serialized ?
The test below documents the correct behavior. See implementors and senders of #nextPutWeak: in Cuis. Those methods have _very_ good comments. If you only dump a model, any views should not get dumped. If you dump a view, both view and model must be dumped. Basic View / Model separation: the View knows the Model, the Model doesn't know about the View. In the case of Morph, the owner of a morph dumped by itself should not be included; but the owner / submorphs relationship must be correct for all morphs that ultimately get dumped. The subtler bug in Cuis was in #references, and is also caught by this test.
Thnaks, now I understand Levente's answer. I figure out how to implement it on Fuel, it should be easy.
Thanks all,
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.
Levente
Dave
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.
Dave
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
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.
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.
Levente
Dave
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
Levente
Dave
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?
Dave
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
On Wed, Dec 07, 2011 at 02:06:09PM +0100, Levente Uzonyi wrote:
On Tue, 6 Dec 2011, David T. Lewis wrote:
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.
Thanks Levente, I think that's the right thing to do.
Dave
On Wed, Dec 07, 2011 at 10:04:46AM -0500, David T. Lewis wrote:
On Wed, Dec 07, 2011 at 02:06:09PM +0100, Levente Uzonyi wrote:
On Tue, 6 Dec 2011, David T. Lewis wrote:
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.
Thanks Levente, I think that's the right thing to do.
I removed the first set of changes from trunk so as not to interfere with the 4.3 release process. Meanwhile we can post updates to inbox as needed.
Dave
On Wed, 7 Dec 2011, David T. Lewis wrote:
On Wed, Dec 07, 2011 at 10:04:46AM -0500, David T. Lewis wrote:
On Wed, Dec 07, 2011 at 02:06:09PM +0100, Levente Uzonyi wrote:
On Tue, 6 Dec 2011, David T. Lewis wrote:
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.
Thanks Levente, I think that's the right thing to do.
I removed the first set of changes from trunk so as not to interfere with the 4.3 release process. Meanwhile we can post updates to inbox as needed.
I was only about to revert ReferenceStream >> #references to the previous version, but this is ok too.
Levente
Dave
But a serialized model should be materializable in, for instance, an UI-less server, or some client with a very different UI framework, etc. This would enable (among many other useful things) sharing serialized objects between Cuis, Squeak and Pharo.
Sorry, let me see if I understood: You mean that as the model has references to UI objects but they are just weak references, then "Serializer serialize: model" should not include UI objects and thus you can transport the model with UI-framework independence. Am I right?
Ouch, I didn't pay attention to the test! now it's clrear for me. Thanks
Martin Dias wrote:
But a serialized model should be materializable in, for instance, an UI-less server, or some client with a very different UI framework, etc. This would enable (among many other useful things) sharing serialized objects between Cuis, Squeak and Pharo. Sorry, let me see if I understood: You mean that as the model has references to UI objects but they are just weak references, then "Serializer serialize: model" should not include UI objects and thus you can transport the model with UI-framework independence. Am I right?
Ouch, I didn't pay attention to the test! now it's clrear for me. Thanks
:)
Cheers, Juan Vuletich
On Tue, Nov 29, 2011 at 11:41 PM, Juan Vuletich juan@jvuletich.org wrote:
Hi Mariano,
Mariano Martinez Peck wrote:
Hi. I am not sure if I understand which is the bug and it is difficult to do a clear diff. You mean that weak references should NOT be serialized ?
The test below documents the correct behavior. See implementors and senders of #nextPutWeak: in Cuis. Those methods have _very_ good comments. If you only dump a model, any views should not get dumped. If you dump a view, both view and model must be dumped. Basic View / Model separation: the View knows the Model, the Model doesn't know about the View. In the case of Morph, the owner of a morph dumped by itself should not be included; but the owner / submorphs relationship must be correct for all morphs that ultimately get dumped. The subtler bug in Cuis was in #references, and is also caught by this test.
Thanks, it is much clear now.
While thinking about this in Fuel I asked in the mailing list <
http://forum.world.st/Should-**a-serializer-do-something-in-** particular-with-weak-**references-td3827593.htmlhttp://forum.world.st/Should-a-serializer-do-something-in-particular-with-weak-references-td3827593.html> and the conclusion was that we shouldn't do anything special in Fuel for weak references.
Wrong conclusion. The correct answer was Levente's: http://forum.world.st/Should-**a-serializer-do-something-in-** particular-with-weak-**references-td3827593.html#**a3828288http://forum.world.st/Should-a-serializer-do-something-in-particular-with-weak-references-td3827593.html#a3828288..
Well, it is just "another" conclusion, but I don't think it is totally wrong (more below)
In this case, if I understand, you are saying the opposite: do something
different for weak references, basically to consider them "transient". Can you please explain me why it is needed a specific behavior for weak references?
Including objects that are only weakly referenced from within the dumped object graph (but strongly referenced from somewhere else) is useless: very shortly after materialization they will go away. You're wasting serialized file size storing garbage, and requesting that these objects can be materialized (their class or some appropriate replacement must be present, etc).
100% agree. That's why I was asking, because from your first answer it looks like not doing that was WRONG or that there could be a real problem.
From my point of view, it is valid, and it is a nice behavior, but it is an
optimization. So I see it as an "optimized solution" :) We are probably going to do the same in Fuel.
But a serialized model should be materializable in, for instance, an UI-less server, or some client with a very different UI framework, etc. This would enable (among many other useful things) sharing serialized objects between Cuis, Squeak and Pharo.
I understand what you mean but I think this concrete example is wrong. Since the model doesn't know the view (hence the view is not serialized), wouldn't it always be able to be materialized in a UI-less server?
Thanks!
Thanks in advance!
Cheers, Juan Vuletich
On Sun, Nov 27, 2011 at 6:28 PM, Juan Vuletich <juan@jvuletich.org<mailto:
juan@jvuletich.org>> wrote:
Hi Folks,
I found a bug in Cuis that made weak references to be improperly dumped in SmartRefStreams due to a bug in ReferenceStream. When I tried to reproduce the bug and fix in Squeak, I found out that in Squeak weak references are always dumped (and not just the more subtle bug in Cuis). The attach includes a test and fix. I'm confident about the fix in the context of Cuis, but I only played a few minutes with this in Squeak.
Can anybody check this for Squeak and consider it for inclusion?
Thanks, Juan Vuletich
...
!ReferenceStreamTest methodsFor: 'testing' stamp: 'jmv 11/27/2011 14:56'! testWeakDumps "Test that if we serialize a model with weak references to views, only the model is serialized and not the views.
Note: The bug became apparent only when dumping a model to
a SmartRefStream, that calls #references, and the serialized stream was later materialized in an image where the view classes had been deleted. In such rare cases, materialization would fail when trying to reference these absent classes. If serializing to a ReferenceStream, the bug didn't become apparent (views were never serialized). If serializing to a SmartRefStream, but view classes still existed, the bug didn't really become apparent (because views were not actually deserialized), the only effect was a larger file.
ReferenceStreamTest new testWeakDumps " | oldInstance refStream | oldInstance :=StringHolder new contents: 'This is a text'. oldInstance addDependent: Morph new. refStream := ReferenceStream on: (DummyStream on: nil). refStream nextPut: oldInstance. self deny: (refStream references keys anySatisfy: [
:dumpedObject | dumpedObject isKindOf: Morph ])! !
-- Mariano http://marianopeck.wordpress.**com http://marianopeck.wordpress.com
Mariano Martinez Peck wrote:
...
But a serialized model should be materializable in, for instance, an UI-less server, or some client with a very different UI framework, etc. This would enable (among many other useful things) sharing serialized objects between Cuis, Squeak and Pharo.
I understand what you mean but I think this concrete example is wrong. Since the model doesn't know the view (hence the view is not serialized), wouldn't it always be able to be materialized in a UI-less server?
The view is weakly referenced. That's the whole point.
The bug is that the view class is included in the serialized stream, and is required to exist on materialization. Hence, the fix.
Cheers, Juan Vuletich
Ps. I copied the test again below. Please take a look at it (that's what it is for).
Thanks!
!ReferenceStreamTest methodsFor: 'testing' stamp: 'jmv 11/27/2011 14:56'! testWeakDumps "Test that if we serialize a model with weak references to views, only the model is serialized and not the views. Note: The bug became apparent only when dumping a model to a SmartRefStream, that calls #references, and the serialized stream was later materialized in an image where the view classes had been deleted. In such rare cases, materialization would fail when trying to reference these absent classes. If serializing to a ReferenceStream, the bug didn't become apparent (views were never serialized). If serializing to a SmartRefStream, but view classes still existed, the bug didn't really become apparent (because views were not actually deserialized), the only effect was a larger file. ReferenceStreamTest new testWeakDumps " | oldInstance refStream | oldInstance :=StringHolder new contents: 'This is a text'. oldInstance addDependent: Morph new. refStream := ReferenceStream on: (DummyStream on: nil). refStream nextPut: oldInstance. self deny: (refStream references keys anySatisfy: [ :dumpedObject | dumpedObject isKindOf: Morph ])! !
-- Mariano http://marianopeck.wordpress.com
squeak-dev@lists.squeakfoundation.org