Arg assignement [Fwd: how can we sync]

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

Arg assignement [Fwd: how can we sync]

Stéphane Ducasse


Begin forwarded message:

From: Eliot Miranda <[hidden email]>
Date: April 2, 2009 11:09:14 PM CEDT
To: Lukas Renggli <[hidden email]>
Cc: Stéphane Ducasse <[hidden email]>, Marcus Denker <[hidden email]>, Michael Rueger <[hidden email]>, Adrian Lienhard <[hidden email]>, John McIntosh <[hidden email]>
Subject: Re: how can we sync



On Thu, Mar 26, 2009 at 1:24 AM, Lukas Renggli <[hidden email]> wrote:
Something related ...

In noticed that there are numerous places in the Pharo code that cannot be re-compiled anymore, because they assign to block parameters. There are two cases:

1. Things along the following example were a bug in the first place anyway, and can be easily fixed:

  aCollection inject: 0 into: [ :s :e | s := s + e ]

2. Then there are also various places where code seems to nil temps to avoid garbage collection issues, such as in WeakArray>>#finalizationProcess.

FinalizationDependents do:
       [:weakDependent |
       weakDependent ifNotNil:
               [weakDependent finalizeValues.
                       "***Following statement is required to keep weakDependent
                       from holding onto its value as garbage.***"
                       weakDependent := nil]]

I think this is no longer necessary and can be fixed by simply removing that assignment as well, however I wanted to get a confirmation by Eliot before submitting a batch of fixes in various places of Pharo.

First of all there is a preference for backward-compatibility (see preferences category compiler).  Turn on allowBlockArgumentAssignment and the old code can still be compiled (and I believe wrks as it did (yuck)).

The bootstrap enables this preference while it is bootstrapping and then turns it off once done so that one is discouraged from assigning to block arguments in new code. At least that's the intent.  But since we tend to load more code than we write I guedss the intent isn't that helpful :)

Second, yes you can get rid of a number of redundant assignments that nil arguments to prevent retention.  Find attached a change set (Closure Usage Fixes) that includes a few from my image.  These either move temporaries local to a block inside the block (useful for performance) or delete unnecessary assignments.  But beware; these changes should be compared carefully against the Pharo image as they apply to the current state of the Qwaq code base which could easily be incompatible.  Best to compare carefully, so to this end...

Third, I wrote a tool to automatically edit methods with method-level temps that are only used at block level.  There's a rather stupid blog post on developing this.  Stupid, because it tries to illustrate programming in the debugger at the same time and I think it ends up beng confused.  Anyway, find the tool attached.  Unsupervised usage is "TempScopeEditor edit".  There is an instance side method editNoCompile that you can use to look at the output without applying the change, but you'll have to rte tool support to make this useful.  I have a hack for side-by-side method comparisons based on MethodReference if you want it but I suspect you have more familiar tools to hand.

Best regards
Eliot





Cheers,
Lukas

--
Lukas Renggli
http://www.lukas-renggli.ch






_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

Closure Usage Fixes.1.cs (69K) Download Attachment
TempScopeEditor2.st (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Arg assignement [Fwd: how can we sync]

Nicolas Cellier
I was just reviewing
http://www.mirandabanda.org/files/Cog/Closures0811/Bootstrap/changesets/
...and focusing on Closure_Usage_Fix.1.cs which has not been loaded in Pharo.

At first glance, I did not see changes about not writing blocks own arguments...
Only changes moving some temporaries inside blocks or removing some
useless ones, so as to get clean blocks.
(Clean blocks which don't write into outside temps should execute faster)

Is there a more recent version?

Nicolas

2009/4/15 Stéphane Ducasse <[hidden email]>:

>
>
> Begin forwarded message:
>
> From: Eliot Miranda <[hidden email]>
> Date: April 2, 2009 11:09:14 PM CEDT
> To: Lukas Renggli <[hidden email]>
> Cc: Stéphane Ducasse <[hidden email]>, Marcus Denker
> <[hidden email]>, Michael Rueger <[hidden email]>, Adrian Lienhard
> <[hidden email]>, John McIntosh <[hidden email]>
> Subject: Re: how can we sync
>
>
> On Thu, Mar 26, 2009 at 1:24 AM, Lukas Renggli <[hidden email]> wrote:
>>
>> Something related ...
>>
>> In noticed that there are numerous places in the Pharo code that cannot be
>> re-compiled anymore, because they assign to block parameters. There are two
>> cases:
>>
>> 1. Things along the following example were a bug in the first place
>> anyway, and can be easily fixed:
>>
>>   aCollection inject: 0 into: [ :s :e | s := s + e ]
>>
>> 2. Then there are also various places where code seems to nil temps to
>> avoid garbage collection issues, such as in WeakArray>>#finalizationProcess.
>>
>> FinalizationDependents do:
>>        [:weakDependent |
>>        weakDependent ifNotNil:
>>                [weakDependent finalizeValues.
>>                        "***Following statement is required to keep
>> weakDependent
>>                        from holding onto its value as garbage.***"
>>                        weakDependent := nil]]
>>
>> I think this is no longer necessary and can be fixed by simply removing
>> that assignment as well, however I wanted to get a confirmation by Eliot
>> before submitting a batch of fixes in various places of Pharo.
>
> First of all there is a preference for backward-compatibility (see
> preferences category compiler).  Turn on allowBlockArgumentAssignment and
> the old code can still be compiled (and I believe wrks as it did (yuck)).
> The bootstrap enables this preference while it is bootstrapping and then
> turns it off once done so that one is discouraged from assigning to block
> arguments in new code. At least that's the intent.  But since we tend to
> load more code than we write I guedss the intent isn't that helpful :)
> Second, yes you can get rid of a number of redundant assignments that nil
> arguments to prevent retention.  Find attached a change set (Closure Usage
> Fixes) that includes a few from my image.  These either move temporaries
> local to a block inside the block (useful for performance) or delete
> unnecessary assignments.  But beware; these changes should be compared
> carefully against the Pharo image as they apply to the current state of the
> Qwaq code base which could easily be incompatible.  Best to compare
> carefully, so to this end...
> Third, I wrote a tool to automatically edit methods with method-level temps
> that are only used at block level.  There's a rather stupid blog post on
> developing this.  Stupid, because it tries to illustrate programming in the
> debugger at the same time and I think it ends up beng confused.  Anyway,
> find the tool attached.  Unsupervised usage is "TempScopeEditor edit".
>  There is an instance side method editNoCompile that you can use to look at
> the output without applying the change, but you'll have to rte tool support
> to make this useful.  I have a hack for side-by-side method comparisons
> based on MethodReference if you want it but I suspect you have more familiar
> tools to hand.
> Best regards
> Eliot
>
>
>
>>
>> Cheers,
>> Lukas
>>
>> --
>> Lukas Renggli
>> http://www.lukas-renggli.ch
>>
>
>
>
>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: Arg assignement [Fwd: how can we sync]

Nicolas Cellier
Oops, I just miss the attachments :(

2009/4/15 Nicolas Cellier <[hidden email]>:
>
> Is there a more recent version?
>
> Nicolas
>

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: Arg assignement [Fwd: how can we sync]

Nicolas Cellier
I published very trivials cleanups from Eliots abount Sounds package
in PharoInbox
I will continue reviewing tomorrow.
Bye

2009/4/15 Nicolas Cellier <[hidden email]>:
> Oops, I just miss the attachments :(
>
> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>
>> Is there a more recent version?
>>
>> Nicolas
>>
>

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: Arg assignement [Fwd: how can we sync]

Nicolas Cellier
Balloon-nice.17
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
Collections-Arrayed-nice.16
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'

I also cleaned evalStrings a little further
----
Collections-Unordered-nice.13
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
Compression-nice.19
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
Compiler-nice.99
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
Files-nice.ducasse.46
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
System-Change Notification-nice.3
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
System-Object Storage-nice.7
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
System-Changes-nice.3
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
Tools-nice.149
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
Kernel-nice.renggli.290
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
Traits-nice.271
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'
----
Graphics-nice.86
trivial BlockClosure cleanup from Eliot's 'Closure Usage Fixes.1.cs'


Nicolas


2009/4/15 Nicolas Cellier <[hidden email]>:

> I published very trivials cleanups from Eliots abount Sounds package
> in PharoInbox
> I will continue reviewing tomorrow.
> Bye
>
> 2009/4/15 Nicolas Cellier <[hidden email]>:
>> Oops, I just miss the attachments :(
>>
>> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>>
>>> Is there a more recent version?
>>>
>>> Nicolas
>>>
>>
>

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: Arg assignement [Fwd: how can we sync]

Stéphane Ducasse
In reply to this post by Nicolas Cellier
Thanks nicolas
You are talking about
        http://code.google.com/p/pharo/issues/detail?id=742

Stef

On Apr 15, 2009, at 11:10 PM, Nicolas Cellier wrote:

> I published very trivials cleanups from Eliots abount Sounds package
> in PharoInbox
> I will continue reviewing tomorrow.
> Bye
>
> 2009/4/15 Nicolas Cellier <[hidden email]>:
>> Oops, I just miss the attachments :(
>>
>> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>>
>>> Is there a more recent version?
>>>
>>> Nicolas
>>>
>>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>


_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: Arg assignement [Fwd: how can we sync]

Nicolas Cellier
No, after a full review of 'Closure Usage Fixes.1.cs', I verified
this file only fix minor case of blocks writing in outside temps,
not the case of blocks writing their own arguments.

Nicolas

2009/4/16 Stéphane Ducasse <[hidden email]>:

> Thanks nicolas
> You are talking about
>        http://code.google.com/p/pharo/issues/detail?id=742
>
> Stef
>
> On Apr 15, 2009, at 11:10 PM, Nicolas Cellier wrote:
>
>> I published very trivials cleanups from Eliots abount Sounds package
>> in PharoInbox
>> I will continue reviewing tomorrow.
>> Bye
>>
>> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>> Oops, I just miss the attachments :(
>>>
>>> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>>>
>>>> Is there a more recent version?
>>>>
>>>> Nicolas
>>>>
>>>
>>
>> _______________________________________________
>> Pharo-project mailing list
>> [hidden email]
>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>
>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: Arg assignement [Fwd: how can we sync]

Stéphane Ducasse
ok do I understand correctly that I do not have to have a look
at 'closure usage fixes' :)

Stef

On Apr 16, 2009, at 11:51 AM, Nicolas Cellier wrote:

> No, after a full review of 'Closure Usage Fixes.1.cs', I verified
> this file only fix minor case of blocks writing in outside temps,
> not the case of blocks writing their own arguments.
>
> Nicolas
>
> 2009/4/16 Stéphane Ducasse <[hidden email]>:
>> Thanks nicolas
>> You are talking about
>>        http://code.google.com/p/pharo/issues/detail?id=742
>>
>> Stef
>>
>> On Apr 15, 2009, at 11:10 PM, Nicolas Cellier wrote:
>>
>>> I published very trivials cleanups from Eliots abount Sounds package
>>> in PharoInbox
>>> I will continue reviewing tomorrow.
>>> Bye
>>>
>>> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>>> Oops, I just miss the attachments :(
>>>>
>>>> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>>>>
>>>>> Is there a more recent version?
>>>>>
>>>>> Nicolas
>>>>>
>>>>
>>>
>>> _______________________________________________
>>> Pharo-project mailing list
>>> [hidden email]
>>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>>
>>
>>
>> _______________________________________________
>> Pharo-project mailing list
>> [hidden email]
>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>


_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: Arg assignement [Fwd: how can we sync]

Nicolas Cellier
Correct, all the fixes are in PharoInbox.
You choose to integrate them or not, they are minor.
But I expect you can integrate them all in less than 1 hour.

Cheers

Nicolas

2009/4/16 Stéphane Ducasse <[hidden email]>:

> ok do I understand correctly that I do not have to have a look
> at 'closure usage fixes' :)
>
> Stef
>
> On Apr 16, 2009, at 11:51 AM, Nicolas Cellier wrote:
>
>> No, after a full review of 'Closure Usage Fixes.1.cs', I verified
>> this file only fix minor case of blocks writing in outside temps,
>> not the case of blocks writing their own arguments.
>>
>> Nicolas
>>
>> 2009/4/16 Stéphane Ducasse <[hidden email]>:
>>> Thanks nicolas
>>> You are talking about
>>>        http://code.google.com/p/pharo/issues/detail?id=742
>>>
>>> Stef
>>>
>>> On Apr 15, 2009, at 11:10 PM, Nicolas Cellier wrote:
>>>
>>>> I published very trivials cleanups from Eliots abount Sounds package
>>>> in PharoInbox
>>>> I will continue reviewing tomorrow.
>>>> Bye
>>>>
>>>> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>>>> Oops, I just miss the attachments :(
>>>>>
>>>>> 2009/4/15 Nicolas Cellier <[hidden email]>:
>>>>>>
>>>>>> Is there a more recent version?
>>>>>>
>>>>>> Nicolas
>>>>>>
>>>>>
>>>>
>>>> _______________________________________________
>>>> Pharo-project mailing list
>>>> [hidden email]
>>>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>>>
>>>
>>>
>>> _______________________________________________
>>> Pharo-project mailing list
>>> [hidden email]
>>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>>
>>
>> _______________________________________________
>> Pharo-project mailing list
>> [hidden email]
>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>
>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project