Re: The Trunk: Kernel-eem.847.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Kernel-eem.847.mcz

Nicolas Cellier

2014-05-03 2:46 GMT+02:00 <[hidden email]>:
Eliot Miranda uploaded a new version of Kernel to project The Trunk:
http://source.squeak.org/trunk/Kernel-eem.847.mcz

==================== Summary ====================

Name: Kernel-eem.847
Author: eem
Time: 2 May 2014, 5:45:56.76 pm
UUID: 61cf122b-e7e3-4c10-9d8b-17df1509d656
Ancestors: Kernel-eem.846

Fix class reshaping losing the correct methodClassAssociation
in instance side methods.

=============== Diff against Kernel-eem.846 ===============

Item was changed:
  ----- Method: Behavior>>compileAllFrom: (in category 'compiling') -----
  compileAllFrom: oldClass
        "Compile all the methods in the receiver's method dictionary.
        This validates sourceCode and variable references and forces
        all methods to use the current bytecode set"
+       | envBinding binding |
-       | binding |
        "ar 7/10/1999: Use oldClass selectors not self selectors"
        oldClass selectorsDo: [:sel | self recompile: sel from: oldClass].

        "Ensure that we share a common binding after recompilation.
+        This is so that ClassBuilder reshapes avoid creating new bindings
+        for every method when recompiling a large class hierarchy.
+        eem 5/2/2014 17:43: Further, if we're not yet in the environment
+        (because we're about to be mutated into oldClass), use oldClass's
+        binding, so as not to end up with the wrong binding post recompile."
-       This is so that ClassBuilder reshapes avoid creating new bindings
-       for every method when recompiling a large class hierarchy."
        binding := self binding.
+       (self name = oldClass name
+        and: [(envBinding := self environment bindingOf: self name) ~= binding
+        and: [envBinding = oldClass binding]]) ifTrue:
+               [binding := envBinding].

Hi Eliot, this is breaking Traits as shown by PureBehaviorTests>>testReshapeClass

We must look into ClassBuilder>>newSubclassOf: newSuper type: type instanceVariables: instVars from: oldClass,
After this instruction: [newClass compileAllFrom: oldClass]
newClass methodDictionary has bindings that points to oldClass instead of newClass (the bindings are #C2 -> oldClass)

Then what happens in newClass setTraitComposition: -> uses: -> installTraitsFrom: -> assembleTraitMethodsFrom: ???
The methods are not recognized as local methods (isLocalMethod:) because the binding points to the oldClass, not the newClass...
As a consequence, the local methods #(foo bar) are not re-installed in newClass, but lost.

What do you think?
Should we revert this?
Or hack isLocalMethod: to rely on name rather than identity?
 
        self methodsDo:[:m|
                m methodClassAssociation == binding
                        ifFalse:[m methodClassAssociation: binding]].
  !





Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Kernel-eem.847.mcz

Eliot Miranda-2
Hi Nicolas,


On Mon, May 26, 2014 at 5:01 PM, Nicolas Cellier <[hidden email]> wrote:

2014-05-03 2:46 GMT+02:00 <[hidden email]>:
Eliot Miranda uploaded a new version of Kernel to project The Trunk:
http://source.squeak.org/trunk/Kernel-eem.847.mcz

==================== Summary ====================

Name: Kernel-eem.847
Author: eem
Time: 2 May 2014, 5:45:56.76 pm
UUID: 61cf122b-e7e3-4c10-9d8b-17df1509d656
Ancestors: Kernel-eem.846

Fix class reshaping losing the correct methodClassAssociation
in instance side methods.

=============== Diff against Kernel-eem.846 ===============

Item was changed:
  ----- Method: Behavior>>compileAllFrom: (in category 'compiling') -----
  compileAllFrom: oldClass
        "Compile all the methods in the receiver's method dictionary.
        This validates sourceCode and variable references and forces
        all methods to use the current bytecode set"
+       | envBinding binding |
-       | binding |
        "ar 7/10/1999: Use oldClass selectors not self selectors"
        oldClass selectorsDo: [:sel | self recompile: sel from: oldClass].

        "Ensure that we share a common binding after recompilation.
+        This is so that ClassBuilder reshapes avoid creating new bindings
+        for every method when recompiling a large class hierarchy.
+        eem 5/2/2014 17:43: Further, if we're not yet in the environment
+        (because we're about to be mutated into oldClass), use oldClass's
+        binding, so as not to end up with the wrong binding post recompile."
-       This is so that ClassBuilder reshapes avoid creating new bindings
-       for every method when recompiling a large class hierarchy."
        binding := self binding.
+       (self name = oldClass name
+        and: [(envBinding := self environment bindingOf: self name) ~= binding
+        and: [envBinding = oldClass binding]]) ifTrue:
+               [binding := envBinding].

Hi Eliot, this is breaking Traits as shown by PureBehaviorTests>>testReshapeClass

We must look into ClassBuilder>>newSubclassOf: newSuper type: type instanceVariables: instVars from: oldClass,
After this instruction: [newClass compileAllFrom: oldClass]
newClass methodDictionary has bindings that points to oldClass instead of newClass (the bindings are #C2 -> oldClass)

Then what happens in newClass setTraitComposition: -> uses: -> installTraitsFrom: -> assembleTraitMethodsFrom: ???
The methods are not recognized as local methods (isLocalMethod:) because the binding points to the oldClass, not the newClass...
As a consequence, the local methods #(foo bar) are not re-installed in newClass, but lost.

What do you think?
Should we revert this?
Or hack isLocalMethod: to rely on name rather than identity?
 
        self methodsDo:[:m|
                m methodClassAssociation == binding
                        ifFalse:[m methodClassAssociation: binding]].
  !

Well, I think the ClassBuilder should just change the bindings in the methods once the replacement class is installed.  I've committed a fix.  I should have worked out that ClassBuilder>>update:to: was the lace to put the code in the first place.  Sorry for the mess.

-- 
best,
Eliot