I've been engaged (in my spare time) for about a year in refactoring associations so that they and their subclasses work a bit more rationally. And I almost have a change set that works!
The tricky bit was to get a change set that files in. This I now have, but it took a lot of hand work, because there are various checks to make sure that Association is NOT recompiled.
OK, so it files in. And most things seem to work. But the WeakIdentityKeyDictionary "ObsoleteSubclasses" (a class variable of Behavior) is messed up after the file in. It should, I think, hold a mapping between "Association" (that is, the class) and an array of its now obsolete subclasses #(WeakKeyAssocation). It actually holds a mapping between a weak "#(Association)" and that array. Very strange.
I think that it's my fault. WeakKeyAssociations used to be implemented by putting a weak one-element array in the key slot of an association. With my refactoring, they are now implemented by using the weak slot of a new subclass, HalfWeakAssociation, for the key and the strong slot for the value, which means that the relative positions of the slots have been switched.
Now, here is my question. Does something in the VM or the GC know about WeakKeyAssociations? That is, does something (think) that it knows about this hidden one-element array. Is that how the one element array is appearing in ObsoleteSubclasses?
I've done the obvious stuff: the only value that Behavior uses as a key in ObsoleteSubclasses is "self". If it is not something in the VM or GC magic, I can't see where this broken key is coming from.
I'm attaching my changeset, and my tests for Association. (This requires the dictionary = fix that I just mailed out.) Inspect ObsoleteSubclasses before and after you file AssociationRefact.Hand in.
Andrew
Andrew,
To understand the effect have a look at the implementations of WeakKeyAssociation>>key and WeakKeyAssociation>>key: - this should make it clear that WeakKeyAssociations store their key in form of a weak array so that it can be garbage collected. Thus there isn't anything broken, except from the interpretation of the key iVar.
Cheers, - Andreas
----- Original Message ----- From: "Andrew P. Black" black@cse.ogi.edu To: "Squeak Mailing List" squeak-dev@lists.squeakfoundation.org Sent: Monday, July 12, 2004 4:02 PM Subject: Refactoring Associations
I've been engaged (in my spare time) for about a year in refactoring associations so that they and their subclasses work a bit more rationally. And I almost have a change set that works!
The tricky bit was to get a change set that files in. This I now have, but it took a lot of hand work, because there are various checks to make sure that Association is NOT recompiled.
OK, so it files in. And most things seem to work. But the WeakIdentityKeyDictionary "ObsoleteSubclasses" (a class variable of Behavior) is messed up after the file in. It should, I think, hold a mapping between "Association" (that is, the class) and an array of its now obsolete subclasses #(WeakKeyAssocation). It actually holds a mapping between a weak "#(Association)" and that array. Very strange.
I think that it's my fault. WeakKeyAssociations used to be implemented by putting a weak one-element array in the key slot of an association. With my refactoring, they are now implemented by using the weak slot of a new subclass, HalfWeakAssociation, for the key and the strong slot for the value, which means that the relative positions of the slots have been switched.
Now, here is my question. Does something in the VM or the GC know about WeakKeyAssociations? That is, does something (think) that it knows about this hidden one-element array. Is that how the one element array is appearing in ObsoleteSubclasses?
I've done the obvious stuff: the only value that Behavior uses as a key in ObsoleteSubclasses is "self". If it is not something in the VM or GC magic, I can't see where this broken key is coming from.
I'm attaching my changeset, and my tests for Association. (This requires the dictionary = fix that I just mailed out.) Inspect ObsoleteSubclasses before and after you file AssociationRefact.Hand in.
Andrew
---------------------------------------------------------------------------- ----
Andreas Raab" andreas.raab@gmx.de writes:
To understand the effect have a look at the implementations of WeakKeyAssociation>>key and WeakKeyAssociation>>key: - this should make it clear that WeakKeyAssociations store their key in form of a weak array so that it can be garbage collected. Thus there isn't anything broken, except from the interpretation of the key iVar.
I figured out what was going wrong - one last WeakKeyAssociation object of the old kind (with the array) was being created by the act of patching in the new kind of WeakKeyAssociation. (Changing the class hierarchy created an obsolete subclass, which had to be weakly remembered...)
However, by the time that this errant object was created, the method dictionary of its class had been zapped (Behavior>>canZapMethodDictionary always returns true; perhaps it should check to see if there are still any instances or the potential to create any). Thus, this object didn't behave correctly, because its key method was the one inherited from LookUpKey.
By doing a double renaming I at last have a change set for this refactoring that can be loaded, at least, it loads fine into a clean 3.7beta #5969.
This changeset passes the tests that I posted a couple of weeks ago.
Andrew
Tested with 3.9a#6506 including execution of the sunit tests Andrew provided in a separated email. Reviewed all code for quality and conflicts. The refactoring is not crucial but it definitely makes these core classes cleaner. The work on cleaning up the code, creating a usfule test suite for this important set of classes and figuring out how to install the change definitely merits inclusion in 3.9a. (There is no particular need to add it to 3.8b, although I did some of my orignal testing there.) Background from preamble: " tlk:Can you tell me something about the motivation for the original Association Refactoring cs ? ab: Essentially, the problem was that several methods (like printOn: !) were missing from some subclasses of Association, like ReadOnlyVariableBinding. Also, WeakKeyAssociations were a really ugly hack. The solution seemed to be to refactor the class hierarchy. This was not hard, but coming up with a changeset that would file in without breaking the compiler half way through was a challenge! This changeset was produced by hand. The order in which the various classes are defined or redefined is critical."
AssociationRefact is the prereq for the other two cs's. It is Andrew's original cs with a change to the preamble to explain the motivation. AssociationRefactTest is tests the first cs and is a prereq for the final cs. Andrew sent me this cs, all I did was rename it, add a preamble and add a class comment for the test class. (I know, anal). AssociationRefactCleanUp is my work. It includes class comments for the association classes and a postscript to remove a now redundant test class. You could accept the first cs, or the first two cs and everything would be okay. But I think all three together make the better package.
Pay attention this is a test to see if we can get experts looking at changes. So can someone with knowledge on association and compiler have a look at this refactoring. I know that andrew spent a lot of time on it and I would like to avoid to see it lost.
Stef
squeak-dev@lists.squeakfoundation.org
-
Andreas Raab -
Andrew P. Black -
ducasse＠iam.unibe.ch -
tomkoenig＠mindspring.com