calling for a fixer :)

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

calling for a fixer :)

Stéphane Ducasse
does anyone want to fix

        SystemNavigationTest>>testIsUnsentMessage



Stef

_______________________________________________
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: calling for a fixer :)

Johan Brichau
This fixes it.
It seems the last 2 literals in each method are selector and  
classbinding.

So now I will try to submit it as a pharo-process-compatible patch too  
(I should learn someday anyway... :-)

----
CompiledMethod>>refersToLiteral: aLiteral
        "Answer true if any literal in this method is literal, even if  
embedded in array structure or within its pragmas."
        "only iterate to numLiterals - 1 , as the last has the classBinding  
and method selector"
        2
                to: self numLiterals - 1
                do:
                        [ :index |
                        | literal |
                        literal := self objectAt: index.
                        literal == aLiteral ifTrue: [ ^ true ].
                        (literal refersToLiteral: aLiteral) ifTrue: [ ^ true ] ].
        ^ false


On 07 Oct 2009, at 12:17, Stéphane Ducasse wrote:

> does anyone want to fix
>
> SystemNavigationTest>>testIsUnsentMessage
>
>
>
> Stef
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

----------------------------
Johan Brichau
[hidden email]





_______________________________________________
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: calling for a fixer :)

Igor Stasenko
2009/10/7 Johan Brichau <[hidden email]>:

> This fixes it.
> It seems the last 2 literals in each method are selector and
> classbinding.
>
> So now I will try to submit it as a pharo-process-compatible patch too
> (I should learn someday anyway... :-)
>
> ----
> CompiledMethod>>refersToLiteral: aLiteral
>        "Answer true if any literal in this method is literal, even if
> embedded in array structure or within its pragmas."
>        "only iterate to numLiterals - 1 , as the last has the classBinding
> and method selector"
>        2
>                to: self numLiterals - 1
>                do:
>                        [ :index |
>                        | literal |
>                        literal := self objectAt: index.
>                        literal == aLiteral ifTrue: [ ^ true ].
>                        (literal refersToLiteral: aLiteral) ifTrue: [ ^ true ] ].
>        ^ false
>

Maybe its worth to change #literalsDo: method to not iterate over
class and method selector?
And then use it in #refersToLiteral: ?

--
Best regards,
Igor Stasenko AKA sig.

_______________________________________________
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: calling for a fixer :)

Johan Brichau

On 07 Oct 2009, at 15:19, Igor Stasenko wrote:
>
> Maybe its worth to change #literalsDo: method to not iterate over
> class and method selector?
> And then use it in #refersToLiteral: ?

Actually, I just noticed after publishing the fix that the last-but-
one position can contain either the method selector -or- the method's  
pragma (which in turn will hold the method's selector).

So, since the #refersToLiteral: method is supposed to return also  
references to literals from the pragma's (see somment) I had to adapt  
the fix to take the last-but-one position as a special case... rather  
than simply excluding it.

----------------------------
Johan Brichau
[hidden email]





_______________________________________________
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: calling for a fixer :)

Igor Stasenko
2009/10/7 Johan Brichau <[hidden email]>:

>
> On 07 Oct 2009, at 15:19, Igor Stasenko wrote:
>>
>> Maybe its worth to change #literalsDo: method to not iterate over
>> class and method selector?
>> And then use it in #refersToLiteral: ?
>
> Actually, I just noticed after publishing the fix that the last-but-
> one position can contain either the method selector -or- the method's
> pragma (which in turn will hold the method's selector).
>
> So, since the #refersToLiteral: method is supposed to return also
> references to literals from the pragma's (see somment) I had to adapt
> the fix to take the last-but-one position as a special case... rather
> than simply excluding it.
>

Okay, if you need to visit pragmas as well, then why don't do it:

CompiledMethod>>refersToLiteral: aLiteral

  self literalsDo: [ blablah ].
  self pragmas do: [ blah blah ]

       ^ false

which makes this method immune from any future changes in CompiledMethod format.
So, we don't need to think about 'fixing' it again and again, each
time we need to hack things.

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



--
Best regards,
Igor Stasenko AKA sig.

_______________________________________________
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: calling for a fixer :)

Lukas Renggli
> Okay, if you need to visit pragmas as well, then why don't do it:

Yes, this is important. Otherwise you can't browser for references to
a Pragma anymore.

> CompiledMethod>>refersToLiteral: aLiteral
>
>  self literalsDo: [ blablah ].
>  self pragmas do: [ blah blah ]
>
>       ^ false
>
> which makes this method immune from any future changes in CompiledMethod format.
> So, we don't need to think about 'fixing' it again and again, each
> time we need to hack things.

This would introduce a major performance bottleneck for commonly used
Smalltalk features like senders/implementors/references.

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
Reply | Threaded
Open this post in threaded view
|

Re: calling for a fixer :)

Igor Stasenko
2009/10/7 Lukas Renggli <[hidden email]>:

>> Okay, if you need to visit pragmas as well, then why don't do it:
>
> Yes, this is important. Otherwise you can't browser for references to
> a Pragma anymore.
>
>> CompiledMethod>>refersToLiteral: aLiteral
>>
>>  self literalsDo: [ blablah ].
>>  self pragmas do: [ blah blah ]
>>
>>       ^ false
>>
>> which makes this method immune from any future changes in CompiledMethod format.
>> So, we don't need to think about 'fixing' it again and again, each
>> time we need to hack things.
>
> This would introduce a major performance bottleneck for commonly used
> Smalltalk features like senders/implementors/references.
>

Well, before stating that, we should measure it.
My point, that we should keep a minimum methods, which is aware of a
deep internals of method's format.
Then it would be easier to maintain it.

> 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
>



--
Best regards,
Igor Stasenko AKA sig.

_______________________________________________
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: calling for a fixer :)

Stéphane Ducasse
I want benchs :)

Stef

On Oct 7, 2009, at 9:24 PM, Igor Stasenko wrote:

> 2009/10/7 Lukas Renggli <[hidden email]>:
>>> Okay, if you need to visit pragmas as well, then why don't do it:
>>
>> Yes, this is important. Otherwise you can't browser for references to
>> a Pragma anymore.
>>
>>> CompiledMethod>>refersToLiteral: aLiteral
>>>
>>>  self literalsDo: [ blablah ].
>>>  self pragmas do: [ blah blah ]
>>>
>>>       ^ false
>>>
>>> which makes this method immune from any future changes in  
>>> CompiledMethod format.
>>> So, we don't need to think about 'fixing' it again and again, each
>>> time we need to hack things.
>>
>> This would introduce a major performance bottleneck for commonly used
>> Smalltalk features like senders/implementors/references.
>>
>
> Well, before stating that, we should measure it.
> My point, that we should keep a minimum methods, which is aware of a
> deep internals of method's format.
> Then it would be easier to maintain it.
>
>> 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
>>
>
>
>
> --
> Best regards,
> Igor Stasenko AKA sig.
>
> _______________________________________________
> 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: calling for a fixer :)

Stéphane Ducasse
In reply to this post by Johan Brichau
Thanks johan!!!
did you open a ticket?

Stef

On Oct 7, 2009, at 3:23 PM, Johan Brichau wrote:

>
> On 07 Oct 2009, at 15:19, Igor Stasenko wrote:
>>
>> Maybe its worth to change #literalsDo: method to not iterate over
>> class and method selector?
>> And then use it in #refersToLiteral: ?
>
> Actually, I just noticed after publishing the fix that the last-but-
> one position can contain either the method selector -or- the method's
> pragma (which in turn will hold the method's selector).
>
> So, since the #refersToLiteral: method is supposed to return also
> references to literals from the pragma's (see somment) I had to adapt
> the fix to take the last-but-one position as a special case... rather
> than simply excluding it.
>
> ----------------------------
> Johan Brichau
> [hidden email]
>
>
>
>
>
> _______________________________________________
> 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: calling for a fixer :)

Johan Brichau
In reply to this post by Stéphane Ducasse

On 07 Oct 2009, at 21:38, Stéphane Ducasse wrote:

> I want benchs :)
>
> Stef

Here they are:

Time millisecondsToRun: [1 to: 10 do:[:i | SystemNavigation new  
allCallsOn: #add: ]].
"current: 2234 2240 2261"
"refactored: 2806 2799 2816"

Time millisecondsToRun: [1 to: 10 do:[:i | SystemNavigation new  
allCallsOn: #foobar ]].
"current: 2166 2216 2159"
"refactored: 2826 2790 2933"

So, an overhead of 20-30%.
However, making that refactoring will probably also trigger a lot of  
changes elsewhere. The current #literalsDo: method iterates over  
header, method properties and class binding too and it is sent by  
external methods. In addition, there is the #hasLiteral:,  
#hasLiteralThorough: and #sendsSelector: methods on CompiledMethod  
that all perform roughly identical tasks but it is far from clear to  
me who is expected to do what, especially since the comments are  
sometimes not in sync with what the method actually does.
It seems like this part of the code is another candidate for cleanup.

----------------------------
Johan Brichau
[hidden email]





_______________________________________________
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: calling for a fixer :)

Johan Brichau
In reply to this post by Stéphane Ducasse

On 07 Oct 2009, at 21:55, Stéphane Ducasse wrote:

> did you open a ticket?


Yes, I did: 1288
but I will probably send in a cleaner fix today (instead of the two  
previous ones... sorry about that!)

----------------------------
Johan Brichau
[hidden email]





_______________________________________________
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: calling for a fixer :)

Stéphane Ducasse
be my guest!
I like this kind of charming song :)

Stef

On Oct 8, 2009, at 10:01 AM, Johan Brichau wrote:

>
> On 07 Oct 2009, at 21:55, Stéphane Ducasse wrote:
>
>> did you open a ticket?
>
>
> Yes, I did: 1288
> but I will probably send in a cleaner fix today (instead of the two
> previous ones... sorry about that!)
>
> ----------------------------
> Johan Brichau
> [hidden email]
>
>
>
>
>
> _______________________________________________
> 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