The Inbox: Kernel-cmm.830.mcz

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

The Inbox: Kernel-cmm.830.mcz

commits-2
A new version of Kernel was added to project The Inbox:
http://source.squeak.org/inbox/Kernel-cmm.830.mcz

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

Name: Kernel-cmm.830
Author: cmm
Time: 7 January 2014, 9:31:35.206 pm
UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
Ancestors: Kernel-fbs.829

Removing a class should remove its subclasses too.

=============== Diff against Kernel-fbs.829 ===============

Item was changed:
  ----- Method: Class>>removeFromSystem: (in category 'initialize-release') -----
  removeFromSystem: logged
  "Forget the receiver from the Smalltalk global dictionary. Any existing
  instances will refer to an obsolete version of the receiver."
 
  "keep the class name and category for triggering the system change message. If we wait to long, then we get obsolete information which is not what we want."
 
  "tell class to deactivate and unload itself-- two separate events in the module system"
+ self subclassesDo: [ : each | each removeFromSystem: logged ].
  self deactivate; unload.
  self superclass ifNotNil:
  ["If we have no superclass there's nothing to be remembered"
  self superclass addObsoleteSubclass: self].
  self environment forgetClass: self logged: logged.
  self obsolete.!


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Chris Muller-3
Command+X on TestCase in the browser.  Sweet.  :)

On Tue, Jan 7, 2014 at 9:31 PM,  <[hidden email]> wrote:

> A new version of Kernel was added to project The Inbox:
> http://source.squeak.org/inbox/Kernel-cmm.830.mcz
>
> ==================== Summary ====================
>
> Name: Kernel-cmm.830
> Author: cmm
> Time: 7 January 2014, 9:31:35.206 pm
> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
> Ancestors: Kernel-fbs.829
>
> Removing a class should remove its subclasses too.
>
> =============== Diff against Kernel-fbs.829 ===============
>
> Item was changed:
>   ----- Method: Class>>removeFromSystem: (in category 'initialize-release') -----
>   removeFromSystem: logged
>         "Forget the receiver from the Smalltalk global dictionary. Any existing
>         instances will refer to an obsolete version of the receiver."
>
>         "keep the class name and category for triggering the system change message. If we wait to long, then we get obsolete information which is not what we want."
>
>         "tell class to deactivate and unload itself-- two separate events in the module system"
> +       self subclassesDo: [ : each | each removeFromSystem: logged ].
>         self deactivate; unload.
>         self superclass ifNotNil:
>                 ["If we have no superclass there's nothing to be remembered"
>                 self superclass addObsoleteSubclass: self].
>         self environment forgetClass: self logged: logged.
>         self obsolete.!
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Frank Shearar-3
On 08 Jan 2014, at 3:36, Chris Muller <[hidden email]> wrote:

> Command+X on TestCase in the browser.  Sweet.  :)
>
> On Tue, Jan 7, 2014 at 9:31 PM,  <[hidden email]> wrote:
>> A new version of Kernel was added to project The Inbox:
>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Kernel-cmm.830
>> Author: cmm
>> Time: 7 January 2014, 9:31:35.206 pm
>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
>> Ancestors: Kernel-fbs.829
>>
>> Removing a class should remove its subclasses too.
>>
>> =============== Diff against Kernel-fbs.829 ===============
>>
>> Item was changed:
>>  ----- Method: Class>>removeFromSystem: (in category 'initialize-release') -----
>>  removeFromSystem: logged
>>        "Forget the receiver from the Smalltalk global dictionary. Any existing
>>        instances will refer to an obsolete version of the receiver."
>>
>>        "keep the class name and category for triggering the system change message. If we wait to long, then we get obsolete information which is not what we want."
>>
>>        "tell class to deactivate and unload itself-- two separate events in the module system"
>> +       self subclassesDo: [ : each | each removeFromSystem: logged ].

I'd rather see this as a separate selector, maybe
#removeFromSystemRecursively: or #removeFromSystemWithSubclasses or
something:. Imagine you're unloading a package. Often there will be
several 'root' classes in your package: a base domain object, a base
exception, other bits. How can I write a package unloader to unload
your package? Either I have to understand your package structure, or I
end up calling #removeFromSystem: multiple times for non-root classes
(because MCClassDefinition will do this when it unloads).

I'd be happy to see it used via cmd-X, perhaps with certain guards, or
a confirmation saying "this will remove Foo, SubFoo, SubSubFoo and
FooNotification. Are you sure?"

frank

>>        self deactivate; unload.
>>        self superclass ifNotNil:
>>                ["If we have no superclass there's nothing to be remembered"
>>                self superclass addObsoleteSubclass: self].
>>        self environment forgetClass: self logged: logged.
>>        self obsolete.!
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Tobias Pape
In reply to this post by commits-2

On 08.01.2014, at 03:31, [hidden email] wrote:

> A new version of Kernel was added to project The Inbox:
> http://source.squeak.org/inbox/Kernel-cmm.830.mcz
>
> ==================== Summary ====================
>
> Name: Kernel-cmm.830
> Author: cmm
> Time: 7 January 2014, 9:31:35.206 pm
> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
> Ancestors: Kernel-fbs.829
>
> Removing a class should remove its subclasses too.
>
No. See comment

> =============== Diff against Kernel-fbs.829 ===============
>
> Item was changed:
>  ----- Method: Class>>removeFromSystem: (in category 'initialize-release') -----
>  removeFromSystem: logged
>   "Forget the receiver from the Smalltalk global dictionary. Any existing
>   instances will refer to an obsolete version of the receiver."

That means, the subclasses will be subclasses from the obsoleted class.

>   "keep the class name and category for triggering the system change message. If we wait to long, then we get obsolete information which is not what we want."
>
>   "tell class to deactivate and unload itself-- two separate events in the module system"
> + self subclassesDo: [ : each | each removeFromSystem: logged ].
>   self deactivate; unload.
>   self superclass ifNotNil:
>   ["If we have no superclass there's nothing to be remembered"
>   self superclass addObsoleteSubclass: self].
>   self environment forgetClass: self logged: logged.
>   self obsolete.!
>
>



signature.asc (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Chris Muller-3
>> A new version of Kernel was added to project The Inbox:
>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Kernel-cmm.830
>> Author: cmm
>> Time: 7 January 2014, 9:31:35.206 pm
>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
>> Ancestors: Kernel-fbs.829
>>
>> Removing a class should remove its subclasses too.
>>
>
> No.

Why?

> See comment

Ok, so if I remove the comment we're golden..?

Otherwise, I want to understand why I would ever want to have a class
with an ObsoleteSuperclass..?

Because I think the system should cater to regular Smalltalk
development.  "Regular" meaning, all classes have a valid superclass.
 :)

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Tobias Pape

On 08.01.2014, at 16:34, Chris Muller <[hidden email]> wrote:

>>> A new version of Kernel was added to project The Inbox:
>>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz
>>>
>>> ==================== Summary ====================
>>>
>>> Name: Kernel-cmm.830
>>> Author: cmm
>>> Time: 7 January 2014, 9:31:35.206 pm
>>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
>>> Ancestors: Kernel-fbs.829
>>>
>>> Removing a class should remove its subclasses too.
>>>
>>
>> No.
>
> Why?
>
>> See comment
>
> Ok, so if I remove the comment we're golden..?
>
> Otherwise, I want to understand why I would ever want to have a class
> with an ObsoleteSuperclass..?
>
> Because I think the system should cater to regular Smalltalk
> development.  "Regular" meaning, all classes have a valid superclass.
Ok, let me rephrase:
I fear that there is functionality in the system that depends on
being-removed classes not removing their subclasses.
However, I am not sure.

Can one of the older (in the sense of wiser) guys chime in here?

If we know this cannot happen, I’m fine with the change.

Best
        -Tobias




signature.asc (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Frank Shearar-3
On 8 January 2014 15:38, Tobias Pape <[hidden email]> wrote:

>
> On 08.01.2014, at 16:34, Chris Muller <[hidden email]> wrote:
>
>>>> A new version of Kernel was added to project The Inbox:
>>>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz
>>>>
>>>> ==================== Summary ====================
>>>>
>>>> Name: Kernel-cmm.830
>>>> Author: cmm
>>>> Time: 7 January 2014, 9:31:35.206 pm
>>>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
>>>> Ancestors: Kernel-fbs.829
>>>>
>>>> Removing a class should remove its subclasses too.
>>>>
>>>
>>> No.
>>
>> Why?
>>
>>> See comment
>>
>> Ok, so if I remove the comment we're golden..?
>>
>> Otherwise, I want to understand why I would ever want to have a class
>> with an ObsoleteSuperclass..?
>>
>> Because I think the system should cater to regular Smalltalk
>> development.  "Regular" meaning, all classes have a valid superclass.
>
> Ok, let me rephrase:
> I fear that there is functionality in the system that depends on
> being-removed classes not removing their subclasses.
> However, I am not sure.
>
> Can one of the older (in the sense of wiser) guys chime in here?

I'm not older or wiser, but if we remove subclasses when we remove
classes, unloading an MCClassDefinition will result in a bunch of
MCClassDefinition's backing classes disappearing out from under them.
As a package unloads, each subclass will be #unload'd N times, N being
its depth in the inheritance hierarchy (where the root's the argument
to the method).

That's why while I'm happy with the _feature_, I want to see it as a
_new_ feature, leaving #removeFromSystem: removing just a single
class.

frank

> If we know this cannot happen, I’m fine with the change.
>
> Best
>         -Tobias
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Chris Muller-3
On Wed, Jan 8, 2014 at 9:45 AM, Frank Shearar <[hidden email]> wrote:

> On 8 January 2014 15:38, Tobias Pape <[hidden email]> wrote:
>>
>> On 08.01.2014, at 16:34, Chris Muller <[hidden email]> wrote:
>>
>>>>> A new version of Kernel was added to project The Inbox:
>>>>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz
>>>>>
>>>>> ==================== Summary ====================
>>>>>
>>>>> Name: Kernel-cmm.830
>>>>> Author: cmm
>>>>> Time: 7 January 2014, 9:31:35.206 pm
>>>>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
>>>>> Ancestors: Kernel-fbs.829
>>>>>
>>>>> Removing a class should remove its subclasses too.
>>>>>
>>>>
>>>> No.
>>>
>>> Why?
>>>
>>>> See comment
>>>
>>> Ok, so if I remove the comment we're golden..?
>>>
>>> Otherwise, I want to understand why I would ever want to have a class
>>> with an ObsoleteSuperclass..?
>>>
>>> Because I think the system should cater to regular Smalltalk
>>> development.  "Regular" meaning, all classes have a valid superclass.
>>
>> Ok, let me rephrase:
>> I fear that there is functionality in the system that depends on
>> being-removed classes not removing their subclasses.
>> However, I am not sure.
>>
>> Can one of the older (in the sense of wiser) guys chime in here?
>
> I'm not older or wiser, but if we remove subclasses when we remove
> classes, unloading an MCClassDefinition will result in a bunch of
> MCClassDefinition's backing classes disappearing out from under them.
> As a package unloads, each subclass will be #unload'd N times, N being
> its depth in the inheritance hierarchy (where the root's the argument
> to the method).

Browse MCClassDefinition>>#unload and trace it into
SystemDictionary>>#removeClassNamed:.  There's a guard to ensure it's
still there before trying to unload it.

So it's just N lookups in a Dictionary, it won't be unloaded N times.

Nevertheless, I factored the individual remove as
#primRemoveFromSystem:, just for you.  :)  So if some external caller
wants to use that they can (although doing it seems it leaves the
system in a bad state, maybe there's some use case I'm missing).

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Colin Putney-3



On Wed, Jan 8, 2014 at 11:29 AM, Chris Muller <[hidden email]> wrote:
 
Nevertheless, I factored the individual remove as
#primRemoveFromSystem:, just for you.  :)  So if some external caller
wants to use that they can (although doing it seems it leaves the
system in a bad state, maybe there's some use case I'm missing).

All this environments and shrinking work I've been doing lately has made me aware of just how complicated and delicate class removal really is. I've been thinking we should do some work on simplifying it in 4.6.

It may be that this *should* be the default behaviour, but I'm with Tobias, we shouldn't change it lightly. If you want to release 4.5 soon, I don't think there's enough time to test a change this fundamental.

Also, a nit: since it's not really a primitive, #primRemoveFromSystem: should be called #basicRemoveFromSystem:

Colin



Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Frank Shearar-3
In reply to this post by Chris Muller-3
On 8 January 2014 16:29, Chris Muller <[hidden email]> wrote:

> On Wed, Jan 8, 2014 at 9:45 AM, Frank Shearar <[hidden email]> wrote:
>> On 8 January 2014 15:38, Tobias Pape <[hidden email]> wrote:
>>>
>>> On 08.01.2014, at 16:34, Chris Muller <[hidden email]> wrote:
>>>
>>>>>> A new version of Kernel was added to project The Inbox:
>>>>>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz
>>>>>>
>>>>>> ==================== Summary ====================
>>>>>>
>>>>>> Name: Kernel-cmm.830
>>>>>> Author: cmm
>>>>>> Time: 7 January 2014, 9:31:35.206 pm
>>>>>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed
>>>>>> Ancestors: Kernel-fbs.829
>>>>>>
>>>>>> Removing a class should remove its subclasses too.
>>>>>>
>>>>>
>>>>> No.
>>>>
>>>> Why?
>>>>
>>>>> See comment
>>>>
>>>> Ok, so if I remove the comment we're golden..?
>>>>
>>>> Otherwise, I want to understand why I would ever want to have a class
>>>> with an ObsoleteSuperclass..?
>>>>
>>>> Because I think the system should cater to regular Smalltalk
>>>> development.  "Regular" meaning, all classes have a valid superclass.
>>>
>>> Ok, let me rephrase:
>>> I fear that there is functionality in the system that depends on
>>> being-removed classes not removing their subclasses.
>>> However, I am not sure.
>>>
>>> Can one of the older (in the sense of wiser) guys chime in here?
>>
>> I'm not older or wiser, but if we remove subclasses when we remove
>> classes, unloading an MCClassDefinition will result in a bunch of
>> MCClassDefinition's backing classes disappearing out from under them.
>> As a package unloads, each subclass will be #unload'd N times, N being
>> its depth in the inheritance hierarchy (where the root's the argument
>> to the method).

So I should really check ALL my mail before responding.

> Browse MCClassDefinition>>#unload and trace it into
> SystemDictionary>>#removeClassNamed:.  There's a guard to ensure it's
> still there before trying to unload it.

It won't route through SystemDictionary because there are no instances
of SystemDictionary in the base image. But yes, Environment >>
#removeClassNamed: does guard.

> So it's just N lookups in a Dictionary, it won't be unloaded N times.

That doesn't make me any happier :) OK, so in my other mail my worry
of multiple #unload sends won't come to pass, but the N lookups for
the _usual_ code path just smells bad. I don't see why we shouldn't
keep just-this-class as the default behaviour - it's really quite deep
in the bowels of the system - and keep yours & Colin's unload-em-all
as a _user level_ optimisation. It's great for the _user_ to be able
to do this.

> Nevertheless, I factored the individual remove as
> #primRemoveFromSystem:, just for you.  :)  So if some external caller
> wants to use that they can (although doing it seems it leaves the
> system in a bad state, maybe there's some use case I'm missing).

Yeah, as I said I would much rather see it the other way round.

frank

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Colin Putney-3



On Wed, Jan 8, 2014 at 1:35 PM, Frank Shearar <[hidden email]> wrote:
 
That doesn't make me any happier :) OK, so in my other mail my worry
of multiple #unload sends won't come to pass, but the N lookups for
the _usual_ code path just smells bad. I don't see why we shouldn't
keep just-this-class as the default behaviour - it's really quite deep
in the bowels of the system - and keep yours & Colin's unload-em-all
as a _user level_ optimisation. It's great for the _user_ to be able
to do this.

Mine? No... I'm willing to listen to the argument, but so far I haven't heard one. And even given a irrefutable arguments, I'd say wait until 4.6.  


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Chris Muller-3
In reply to this post by Colin Putney-3
>> Nevertheless, I factored the individual remove as
>> #primRemoveFromSystem:, just for you.  :)  So if some external caller
>> wants to use that they can (although doing it seems it leaves the
>> system in a bad state, maybe there's some use case I'm missing).
>
> All this environments and shrinking work I've been doing lately has made me
> aware of just how complicated and delicate class removal really is. I've
> been thinking we should do some work on simplifying it in 4.6.

One thing that's true is, I have no experience with shrinking, so,
that's fine.  There's no hurry on this.  :)

> It may be that this *should* be the default behaviour, but I'm with Tobias,
> we shouldn't change it lightly. If you want to release 4.5 soon, I don't
> think there's enough time to test a change this fundamental.

However, I do think that the image is basically nowhere in terms of
deleting classes.  IOW, no one ever deletes classes with subclasses
except during development.

> Also, a nit: since it's not really a primitive, #primRemoveFromSystem:
> should be called #basicRemoveFromSystem:

Sure, that's fine.  I just wanted it to be something that connotates
being at a "lower-level" than the API.  #basic does that just fine.

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-cmm.830.mcz

Frank Shearar-3
In reply to this post by Colin Putney-3
On 8 January 2014 20:05, Colin Putney <[hidden email]> wrote:

>
>
>
> On Wed, Jan 8, 2014 at 1:35 PM, Frank Shearar <[hidden email]>
> wrote:
>
>>
>> That doesn't make me any happier :) OK, so in my other mail my worry
>> of multiple #unload sends won't come to pass, but the N lookups for
>> the _usual_ code path just smells bad. I don't see why we shouldn't
>> keep just-this-class as the default behaviour - it's really quite deep
>> in the bowels of the system - and keep yours & Colin's unload-em-all
>> as a _user level_ optimisation. It's great for the _user_ to be able
>> to do this.
>
>
> Mine? No... I'm willing to listen to the argument, but so far I haven't
> heard one.

Sure you have: "there is no sensible use case for deleting a class
when it has subclasses: you leave the inheritance hierarchy broken".

> And even given a irrefutable arguments, I'd say wait until 4.6.

Why did I think you'd actually done that in the shrink.st? You don't!
Yeah, ok. But strongly agree re "wait until 4.6".

frank