Smalltalk › Gnu

Better compiled method copying

‹ Previous Topic Next Topic ›
 
classic Classic list List threaded Threaded
♦
♦
 5 messages Options
Gwenaël Casaccio
Reply | Threaded
Open this post in threaded view
♦
♦
|

Better compiled method copying

Gwenaël Casaccio
Hi,

Add new Tests package and better support for method copying.

When a compiled method is copied some literals (block and closures)
need to be fixed: they are pointing to the bad method. Also the debug
information need to be patched to point to the new literals array.
And a new test package is added with compiled method testing.

Cheers,
Gwen


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

0001-Add-new-Tests-package-and-better-support-for-method-.patch (13K) Download Attachment
Holger Freyther
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Better compiled method copying

Holger Freyther
On Tue, Jun 11, 2013 at 11:30:33PM +0200, Gwenaël Casaccio wrote:

Good Evening,

here some quick comments on the code and commit message.


> When a compiled method is copied some literals (block and closures)
> need to be fixed: they are pointing to the bad method. Also the debug
> information need to be patched to point to the new literals array.

"useless"/"bad". These words carry judgement but there is no poin in
judging. I would very much prefer if you could use a more neutral tone
in your commit messages.

Could you elaborate on how you stumbled across this? When did you copy
the CompiledMethod? What was the usecase?


> +GST_PACKAGE_ENABLE([Tests], [tests])

"Tests" is very generic. What about "SystemTests"?  I understand that
using SUnit is nicer than the GNU autotest framework and personally I
can understand that.


> +    method: aCompiledCode [
> +        <category: 'accessing'>
> +
> +        block method: aCompiledCode
> +    ]


Sounds more like a private method to me, than 'accessing'.

> +    deepCopy [
           ^super deepCopy
> +            fixBlockInformation;
> +            fixDebugInformation: self;
> +            makeLiteralsReadOnly;
               yourself

why didn't this work? Otherwise you will need to adjust your test
case to also test for classes where isPointers evaluates to true.


> +            (literals at: i) class == BlockClosure ifTrue: [
> +                | new_block |
> +                new_block := (literals at: i) deepCopy.
> +                new_block block: new_block block copy.
> +                new_block method: self.
> +                literals at: i put: new_block ]. ]

can you please elaborate on these lines? First youtake a deep copy
and then you take a copy of the deep copied block? Why is that needed?

> +    postCopy [
> +        "Private - Make a deep copy of the descriptor and literals.
> +         Don't need to replace the method header and bytecodes, since they
> +         are integers."
> +
> +        <category: 'private-copying'>
> +
> +        super postCopy.
> +        descriptor := descriptor copy.
> +        literals := literals copy.
> +        self fixBlockInformation.
> +        self makeLiteralsReadOnly.
> +        "literals := literals deepCopy.
> +         self makeLiteralsReadOnly"

time to remove the commented out code as you are doing this now? Did you
do the archology to see if these two lines have ever been enabled in the
last couple of years?


> +    method: aCompiledMethod [
> + <category: 'accessing'>

it is not really accessing when you modify a class. :)

> +TestCase subclass: TestCompiledMethod [
> +
> +    setUp [
> +        <category: 'setup'>
> +
> +        Object subclass: #Bar.
> +        Object subclass: #Foo.

a tearDown should remove this class too.


> +    testCopy [

...

> +        self assert: old_method ~~ new_method.
> +        self assert: old_method literals ~~ new_method literals.
> +        self assert: old_method getHeader == new_method getHeader.
> +        self assert: old_method descriptor ~~ new_method descriptor.
> +        self assert: old_method descriptor debugInformation ~~ new_method descriptor debugInformation.

matching bytecodes could be added?


> +    testDeepCopy [
> +        <category: 'testing'>

can some code from the above be re-used and also with the below.


> +    ]
> +
> +    testWithNewMethodClass [
> +        <category: 'testing'>


thanks for the patch!


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk
Gwenaël Casaccio
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Better compiled method copying

Gwenaël Casaccio
On 23/06/2013 19:45, Holger Hans Peter Freyther wrote:

> On Tue, Jun 11, 2013 at 11:30:33PM +0200, Gwenaël Casaccio wrote:
>
> Good Evening,
>
> here some quick comments on the code and commit message.
>
>
>> When a compiled method is copied some literals (block and closures)
>> need to be fixed: they are pointing to the bad method. Also the debug
>> information need to be patched to point to the new literals array.
> "useless"/"bad". These words carry judgement but there is no poin in
> judging. I would very much prefer if you could use a more neutral tone
> in your commit messages.
>
> Could you elaborate on how you stumbled across this? When did you copy
> the CompiledMethod? What was the usecase?
When a compiled method is copied the debug informations attached
to them still referenced the old compiled method, the patch fix the
references to the new compiled method. Moreover the compiled block
or block closure should also reference the new compiled method.

>
>> +GST_PACKAGE_ENABLE([Tests], [tests])
> "Tests" is very generic. What about "SystemTests"?  I understand that
> using SUnit is nicer than the GNU autotest framework and personally I
> can understand that.
SystemTests is a better name :)

>
>> +    method: aCompiledCode [
>> +        <category: 'accessing'>
>> +
>> +        block method: aCompiledCode
>> +    ]
>
> Sounds more like a private method to me, than 'accessing'.
>
>> +    deepCopy [
>   ^super deepCopy
>> +            fixBlockInformation;
>> +            fixDebugInformation: self;
>> +            makeLiteralsReadOnly;
>       yourself
>
> why didn't this work? Otherwise you will need to adjust your test
> case to also test for classes where isPointers evaluates to true.
>
>
>> +            (literals at: i) class == BlockClosure ifTrue: [
>> +                | new_block |
>> +                new_block := (literals at: i) deepCopy.
>> +                new_block block: new_block block copy.
>> +                new_block method: self.
>> +                literals at: i put: new_block ]. ]
> can you please elaborate on these lines? First youtake a deep copy
> and then you take a copy of the deep copied block? Why is that needed?

I fix the method reference of the block to the new compiled method
otherwise it will reference the older method.

>> +    postCopy [
>> +        "Private - Make a deep copy of the descriptor and literals.
>> +         Don't need to replace the method header and bytecodes, since they
>> +         are integers."
>> +
>> +        <category: 'private-copying'>
>> +
>> +        super postCopy.
>> +        descriptor := descriptor copy.
>> +        literals := literals copy.
>> +        self fixBlockInformation.
>> +        self makeLiteralsReadOnly.
>> +        "literals := literals deepCopy.
>> +         self makeLiteralsReadOnly"
> time to remove the commented out code as you are doing this now? Did you
> do the archology to see if these two lines have ever been enabled in the
> last couple of years?
>
Yes

>> +    method: aCompiledMethod [
>> + <category: 'accessing'>
> it is not really accessing when you modify a class. :)
>
>> +TestCase subclass: TestCompiledMethod [
>> +
>> +    setUp [
>> +        <category: 'setup'>
>> +
>> +        Object subclass: #Bar.
>> +        Object subclass: #Foo.
> a tearDown should remove this class too.
Yes but I prefer a way to create anonymous classes

>
>> +    testCopy [
> ...
>
>> +        self assert: old_method ~~ new_method.
>> +        self assert: old_method literals ~~ new_method literals.
>> +        self assert: old_method getHeader == new_method getHeader.
>> +        self assert: old_method descriptor ~~ new_method descriptor.
>> +        self assert: old_method descriptor debugInformation ~~ new_method descriptor debugInformation.
> matching bytecodes could be added?
Yes
>
>> +    testDeepCopy [
>> +        <category: 'testing'>
> can some code from the above be re-used and also with the below.
Yes it will be refactored
>
>> +    ]
>> +
>> +    testWithNewMethodClass [
>> +        <category: 'testing'>
>
> thanks for the patch!
>

Gwen


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk
Paolo Bonzini-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Better compiled method copying

Paolo Bonzini-2
In reply to this post by Holger Freyther
Il 23/06/2013 19:45, Holger Hans Peter Freyther ha scritto:

> On Tue, Jun 11, 2013 at 11:30:33PM +0200, Gwenaël Casaccio wrote:
>
> Good Evening,
>
> here some quick comments on the code and commit message.
>
>
>> When a compiled method is copied some literals (block and closures)
>> need to be fixed: they are pointing to the bad method. Also the debug
>> information need to be patched to point to the new literals array.
>
> "useless"/"bad". These words carry judgement but there is no poin in
> judging. I would very much prefer if you could use a more neutral tone
> in your commit messages.
>
> Could you elaborate on how you stumbled across this? When did you copy
> the CompiledMethod? What was the usecase?
>
>
>> +GST_PACKAGE_ENABLE([Tests], [tests])
>
> "Tests" is very generic. What about "SystemTests"?  I understand that
> using SUnit is nicer than the GNU autotest framework and personally I
> can understand that.

Or KernelTests.  It's a pity that Kernel is not a regular package. :(

>
>> +    method: aCompiledCode [
>> +        <category: 'accessing'>
>> +
>> +        block method: aCompiledCode
>> +    ]
>
>
> Sounds more like a private method to me, than 'accessing'.

Agreed.

>> +    deepCopy [
>   ^super deepCopy
>> +            fixBlockInformation;
>> +            fixDebugInformation: self;
>> +            makeLiteralsReadOnly;
>       yourself
>
> why didn't this work? Otherwise you will need to adjust your test
> case to also test for classes where isPointers evaluates to true.
>
>
>> +            (literals at: i) class == BlockClosure ifTrue: [
>> +                | new_block |
>> +                new_block := (literals at: i) deepCopy.

No underscores in variable names.

>> +                new_block block: new_block block copy.
>> +                new_block method: self.
>> +                literals at: i put: new_block ]. ]
>
> can you please elaborate on these lines? First youtake a deep copy
> and then you take a copy of the deep copied block? Why is that needed?
>
>> +    postCopy [
>> +        "Private - Make a deep copy of the descriptor and literals.
>> +         Don't need to replace the method header and bytecodes, since they
>> +         are integers."
>> +
>> +        <category: 'private-copying'>
>> +
>> +        super postCopy.
>> +        descriptor := descriptor copy.
>> +        literals := literals copy.
>> +        self fixBlockInformation.
>> +        self makeLiteralsReadOnly.
>> +        "literals := literals deepCopy.
>> +         self makeLiteralsReadOnly"
>
> time to remove the commented out code as you are doing this now? Did you
> do the archology to see if these two lines have ever been enabled in the
> last couple of years?
>
>
>> +    method: aCompiledMethod [
>> + <category: 'accessing'>
>
> it is not really accessing when you modify a class. :)
>
>> +TestCase subclass: TestCompiledMethod [
>> +
>> +    setUp [
>> +        <category: 'setup'>
>> +
>> +        Object subclass: #Bar.
>> +        Object subclass: #Foo.
>
> a tearDown should remove this class too.

I think it's better to create the classes unconditionally.
setUp/tearDown can create and remove the methods, though.

Paolo

>
>> +    testCopy [
>
> ...
>
>> +        self assert: old_method ~~ new_method.
>> +        self assert: old_method literals ~~ new_method literals.
>> +        self assert: old_method getHeader == new_method getHeader.
>> +        self assert: old_method descriptor ~~ new_method descriptor.
>> +        self assert: old_method descriptor debugInformation ~~ new_method descriptor debugInformation.
>
> matching bytecodes could be added?
>
>
>> +    testDeepCopy [
>> +        <category: 'testing'>
>
> can some code from the above be re-used and also with the below.
>
>
>> +    ]
>> +
>> +    testWithNewMethodClass [
>> +        <category: 'testing'>
>
>
> thanks for the patch!
>
>
> _______________________________________________
> help-smalltalk mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/help-smalltalk
>


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk
Gwenaël Casaccio
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: Better compiled method copying

Gwenaël Casaccio
On 27/06/2013 10:21, Paolo Bonzini wrote:

> Il 23/06/2013 19:45, Holger Hans Peter Freyther ha scritto:
>> On Tue, Jun 11, 2013 at 11:30:33PM +0200, Gwenaël Casaccio wrote:
>>
>> Good Evening,
>>
>> here some quick comments on the code and commit message.
>>
>>
>>> When a compiled method is copied some literals (block and closures)
>>> need to be fixed: they are pointing to the bad method. Also the debug
>>> information need to be patched to point to the new literals array.
>> "useless"/"bad". These words carry judgement but there is no poin in
>> judging. I would very much prefer if you could use a more neutral tone
>> in your commit messages.
>>
>> Could you elaborate on how you stumbled across this? When did you copy
>> the CompiledMethod? What was the usecase?
>>
>>
>>> +GST_PACKAGE_ENABLE([Tests], [tests])
>> "Tests" is very generic. What about "SystemTests"?  I understand that
>> using SUnit is nicer than the GNU autotest framework and personally I
>> can understand that.
> Or KernelTests.  It's a pity that Kernel is not a regular package. :(
>
>>> +    method: aCompiledCode [
>>> +        <category: 'accessing'>
>>> +
>>> +        block method: aCompiledCode
>>> +    ]
>>
>> Sounds more like a private method to me, than 'accessing'.
> Agreed.
>
>>> +    deepCopy [
>>   ^super deepCopy
>>> +            fixBlockInformation;
>>> +            fixDebugInformation: self;
>>> +            makeLiteralsReadOnly;
>>       yourself
>>
>> why didn't this work? Otherwise you will need to adjust your test
>> case to also test for classes where isPointers evaluates to true.
>>
>>
>>> +            (literals at: i) class == BlockClosure ifTrue: [
>>> +                | new_block |
>>> +                new_block := (literals at: i) deepCopy.
> No underscores in variable names.
>
>>> +                new_block block: new_block block copy.
>>> +                new_block method: self.
>>> +                literals at: i put: new_block ]. ]
>> can you please elaborate on these lines? First youtake a deep copy
>> and then you take a copy of the deep copied block? Why is that needed?
>>
>>> +    postCopy [
>>> +        "Private - Make a deep copy of the descriptor and literals.
>>> +         Don't need to replace the method header and bytecodes, since they
>>> +         are integers."
>>> +
>>> +        <category: 'private-copying'>
>>> +
>>> +        super postCopy.
>>> +        descriptor := descriptor copy.
>>> +        literals := literals copy.
>>> +        self fixBlockInformation.
>>> +        self makeLiteralsReadOnly.
>>> +        "literals := literals deepCopy.
>>> +         self makeLiteralsReadOnly"
>> time to remove the commented out code as you are doing this now? Did you
>> do the archology to see if these two lines have ever been enabled in the
>> last couple of years?
>>
>>
>>> +    method: aCompiledMethod [
>>> + <category: 'accessing'>
>> it is not really accessing when you modify a class. :)
>>
>>> +TestCase subclass: TestCompiledMethod [
>>> +
>>> +    setUp [
>>> +        <category: 'setup'>
>>> +
>>> +        Object subclass: #Bar.
>>> +        Object subclass: #Foo.
>> a tearDown should remove this class too.
> I think it's better to create the classes unconditionally.
> setUp/tearDown can create and remove the methods, though.
>
> Paolo
>
>>> +    testCopy [
>> ...
>>
>>> +        self assert: old_method ~~ new_method.
>>> +        self assert: old_method literals ~~ new_method literals.
>>> +        self assert: old_method getHeader == new_method getHeader.
>>> +        self assert: old_method descriptor ~~ new_method descriptor.
>>> +        self assert: old_method descriptor debugInformation ~~ new_method descriptor debugInformation.
>> matching bytecodes could be added?
>>
>>
>>> +    testDeepCopy [
>>> +        <category: 'testing'>
>> can some code from the above be re-used and also with the below.
>>
>>
>>> +    ]
>>> +
>>> +    testWithNewMethodClass [
>>> +        <category: 'testing'>
>>
>> thanks for the patch!
>>
>>
>> _______________________________________________
>> help-smalltalk mailing list
>> [hidden email]
>> https://lists.gnu.org/mailman/listinfo/help-smalltalk
>>
All the points should be fixed in the patch

Gwen


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

0001-Add-a-new-Kernel-Tests-package-and-a-better-support-.patch (13K) Download Attachment
Free forum by Nabble Edit this page