Slang question (was: Re: [squeak-dev] The Trunk: Collections-ul.685.mcz)

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

Slang question (was: Re: [squeak-dev] The Trunk: Collections-ul.685.mcz)

Levente Uzonyi
On Sun, 3 Apr 2016, [hidden email] wrote:

> Levente Uzonyi uploaded a new version of Collections to project The Trunk:
> http://source.squeak.org/trunk/Collections-ul.685.mcz
>
> ==================== Summary ====================
>
> Name: Collections-ul.685
> Author: ul
> Time: 3 April 2016, 3:21:51.035808 am
> UUID: db8fd391-2306-4ccc-87d5-b5dee96a78ab
> Ancestors: Collections-eem.684
>
> Use Spur's new character comparison abilities in some String methods.
>
> =============== Diff against Collections-eem.684 ===============
>
> Item was changed:
>  ----- Method: ByteString>>findSubstring:in:startingAt:matchTable: (in category 'comparing') -----
>  findSubstring: key in: body startingAt: start matchTable: matchTable
>   "Answer the index in the string body at which the substring key first occurs, at or beyond start.  The match is determined using matchTable, which can be used to effect, eg, case-insensitive matches.  If no match is found, zero will be returned.
>
>   The algorithm below is not optimum -- it is intended to be translated to C which will go so fast that it wont matter."
>   | index |
>   <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
>   <var: #key declareC: 'unsigned char *key'>
>   <var: #body declareC: 'unsigned char *body'>
>   <var: #matchTable declareC: 'unsigned char *matchTable'>
>
>   key size = 0 ifTrue: [^ 0].
> + matchTable ifNil: [
> + start to: body size - key size + 1 do: [ :startIndex |
> + index := 0.
> + [ (body at: startIndex + index) == (key at: (index := index + 1)) ] whileTrue: [
> + index = key size ifTrue: [ ^startIndex ] ] ].
> + ^0 ].

I wonder if the #ifNil: check is correct in Slang. The generated code is

...
     unsigned char *matchTable;
...
         matchTable = arrayValueOf(stackValue(0));
         matchTable -= 1;
...
         if (!(matchTable)) {
...

So, will the value of matchTable actually be NULL or not, when nil is
passed to the primitive. That -= 1 makes me think that it won't be NULL.

Another thing is that the loop condition, while it's order of evaluation
is well defined in Smalltalk, will compile to C code containing undefined
behavior:

                         while ((body[startIndex + index]) == (key[index += 1])) {

Since in C the evaluation of the left and right side of == can happen in
any order, the generated machine code may or may not be correct.
Is this is a difference between Smalltalk and Slang, or is it just a slip
in the code generator?

Levente

>   (start max: 1) to: body size - key size + 1 do:
>   [:startIndex |
>   index := 1.
>   [(matchTable at: (body at: startIndex+index-1) asciiValue + 1)
>   = (matchTable at: (key at: index) asciiValue + 1)]
>   whileTrue:
>   [index = key size ifTrue: [^ startIndex].
>   index := index+1]].
>   ^ 0
>  "
>  ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 1 matchTable: CaseSensitiveOrder 1
>  ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 2 matchTable: CaseSensitiveOrder 7
>  ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 8 matchTable: CaseSensitiveOrder 0
>  ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable: CaseSensitiveOrder 0
>  ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable: CaseInsensitiveOrder 7
>  "!

Reply | Threaded
Open this post in threaded view
|

Re: Slang question (was: Re: [squeak-dev] The Trunk: Collections-ul.685.mcz)

David T. Lewis
Many people will not be familiar with this optimization, so a few words of
explanation are in order for background:

There is a mechanism for translating certain methods in the image directly
into primitives for performance. This is normally not noticed, but you will
find occasional refernces such at this one in the image:

  findSubstring: key in: body startingAt: start matchTable: matchTable
       <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>

This is a method that first tries to call a version of itself that has
been translated to C. If the primitive fails, the normal Smalltalk method
is executed. The hidden tricky bit is that MiscPrimitivePlugin, which
lives in the VMMaker package, arranges for the #findSubstring:in:startingAt:matchTable:
method in the image to be translated to C and installed as a primitive.
This means that the generated VM is actually governed partly my methods
in the main image (not in the VMMaker package).

It also means that if one of these methods is changed in the image, it
might accidentally result in unexpected (and possibly wrong) code being
generated for the plugin in the VM. That is the reason that Levente is
asking about the slang code generation in this case.

Dave


On Sun, Apr 03, 2016 at 07:45:13PM +0200, Levente Uzonyi wrote:

> On Sun, 3 Apr 2016, [hidden email] wrote:
>
> >Levente Uzonyi uploaded a new version of Collections to project The Trunk:
> >http://source.squeak.org/trunk/Collections-ul.685.mcz
> >
> >==================== Summary ====================
> >
> >Name: Collections-ul.685
> >Author: ul
> >Time: 3 April 2016, 3:21:51.035808 am
> >UUID: db8fd391-2306-4ccc-87d5-b5dee96a78ab
> >Ancestors: Collections-eem.684
> >
> >Use Spur's new character comparison abilities in some String methods.
> >
> >=============== Diff against Collections-eem.684 ===============
> >
> >Item was changed:
> > ----- Method: ByteString>>findSubstring:in:startingAt:matchTable: (in
> > category 'comparing') -----
> > findSubstring: key in: body startingAt: start matchTable: matchTable
> > "Answer the index in the string body at which the substring key
> > first occurs, at or beyond start.  The match is determined using
> > matchTable, which can be used to effect, eg, case-insensitive
> > matches.  If no match is found, zero will be returned.
> >
> > The algorithm below is not optimum -- it is intended to be
> > translated to C which will go so fast that it wont matter."
> > | index |
> > <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
> > <var: #key declareC: 'unsigned char *key'>
> > <var: #body declareC: 'unsigned char *body'>
> > <var: #matchTable declareC: 'unsigned char *matchTable'>
> >
> > key size = 0 ifTrue: [^ 0].
> >+ matchTable ifNil: [
> >+ start to: body size - key size + 1 do: [ :startIndex |
> >+ index := 0.
> >+ [ (body at: startIndex + index) == (key at: (index
> >:= index + 1)) ] whileTrue: [
> >+ index = key size ifTrue: [ ^startIndex ] ] ].
> >+ ^0 ].
>
> I wonder if the #ifNil: check is correct in Slang. The generated code is
>
> ...
>     unsigned char *matchTable;
> ...
>         matchTable = arrayValueOf(stackValue(0));
>         matchTable -= 1;
> ...
>         if (!(matchTable)) {
> ...
>
> So, will the value of matchTable actually be NULL or not, when nil is
> passed to the primitive. That -= 1 makes me think that it won't be NULL.
>
> Another thing is that the loop condition, while it's order of evaluation
> is well defined in Smalltalk, will compile to C code containing undefined
> behavior:
>
>                         while ((body[startIndex + index]) == (key[index +=
>                         1])) {
>
> Since in C the evaluation of the left and right side of == can happen in
> any order, the generated machine code may or may not be correct.
> Is this is a difference between Smalltalk and Slang, or is it just a slip
> in the code generator?
>
> Levente
>
> > (start max: 1) to: body size - key size + 1 do:
> > [:startIndex |
> > index := 1.
> > [(matchTable at: (body at: startIndex+index-1)
> > asciiValue + 1)
> > = (matchTable at: (key at: index) asciiValue
> > + 1)]
> > whileTrue:
> > [index = key size ifTrue: [^ startIndex].
> > index := index+1]].
> > ^ 0
> > "
> > ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 1 matchTable:
> > CaseSensitiveOrder 1
> > ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 2 matchTable:
> > CaseSensitiveOrder 7
> > ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 8 matchTable:
> > CaseSensitiveOrder 0
> > ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable:
> > CaseSensitiveOrder 0
> > ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable:
> > CaseInsensitiveOrder 7
> > "!

Reply | Threaded
Open this post in threaded view
|

Re: Slang question (was: Re: [squeak-dev] The Trunk: Collections-ul.685.mcz)

David T. Lewis
I just got around to checking this for code generation for the interpreter VM
(VMMaker, not VMMaker.oscog).  The #ifNil: is not translated at all, so I replaced
it with this:

        matchTable = nil ifTrue:

This produces the expected result, and can be compiled.

There are other issues in the resulting VM, but they do not seem to be
related to Collections-ul.685 (I reverted to Collections-ul.684 with the
same results). I don't know the cause of those issues, but I will follow
up in a separate thread as needed.

With respect to Collections-ul.685, replacing the ifNil: test as above
seems like the right thing to do.

Dave


On Sun, Apr 03, 2016 at 06:00:13PM -0400, David T. Lewis wrote:

> Many people will not be familiar with this optimization, so a few words of
> explanation are in order for background:
>
> There is a mechanism for translating certain methods in the image directly
> into primitives for performance. This is normally not noticed, but you will
> find occasional refernces such at this one in the image:
>
>   findSubstring: key in: body startingAt: start matchTable: matchTable
>        <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
>
> This is a method that first tries to call a version of itself that has
> been translated to C. If the primitive fails, the normal Smalltalk method
> is executed. The hidden tricky bit is that MiscPrimitivePlugin, which
> lives in the VMMaker package, arranges for the #findSubstring:in:startingAt:matchTable:
> method in the image to be translated to C and installed as a primitive.
> This means that the generated VM is actually governed partly my methods
> in the main image (not in the VMMaker package).
>
> It also means that if one of these methods is changed in the image, it
> might accidentally result in unexpected (and possibly wrong) code being
> generated for the plugin in the VM. That is the reason that Levente is
> asking about the slang code generation in this case.
>
> Dave
>
>
> On Sun, Apr 03, 2016 at 07:45:13PM +0200, Levente Uzonyi wrote:
> > On Sun, 3 Apr 2016, [hidden email] wrote:
> >
> > >Levente Uzonyi uploaded a new version of Collections to project The Trunk:
> > >http://source.squeak.org/trunk/Collections-ul.685.mcz
> > >
> > >==================== Summary ====================
> > >
> > >Name: Collections-ul.685
> > >Author: ul
> > >Time: 3 April 2016, 3:21:51.035808 am
> > >UUID: db8fd391-2306-4ccc-87d5-b5dee96a78ab
> > >Ancestors: Collections-eem.684
> > >
> > >Use Spur's new character comparison abilities in some String methods.
> > >
> > >=============== Diff against Collections-eem.684 ===============
> > >
> > >Item was changed:
> > > ----- Method: ByteString>>findSubstring:in:startingAt:matchTable: (in
> > > category 'comparing') -----
> > > findSubstring: key in: body startingAt: start matchTable: matchTable
> > > "Answer the index in the string body at which the substring key
> > > first occurs, at or beyond start.  The match is determined using
> > > matchTable, which can be used to effect, eg, case-insensitive
> > > matches.  If no match is found, zero will be returned.
> > >
> > > The algorithm below is not optimum -- it is intended to be
> > > translated to C which will go so fast that it wont matter."
> > > | index |
> > > <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
> > > <var: #key declareC: 'unsigned char *key'>
> > > <var: #body declareC: 'unsigned char *body'>
> > > <var: #matchTable declareC: 'unsigned char *matchTable'>
> > >
> > > key size = 0 ifTrue: [^ 0].
> > >+ matchTable ifNil: [
> > >+ start to: body size - key size + 1 do: [ :startIndex |
> > >+ index := 0.
> > >+ [ (body at: startIndex + index) == (key at: (index
> > >:= index + 1)) ] whileTrue: [
> > >+ index = key size ifTrue: [ ^startIndex ] ] ].
> > >+ ^0 ].
> >
> > I wonder if the #ifNil: check is correct in Slang. The generated code is
> >
> > ...
> >     unsigned char *matchTable;
> > ...
> >         matchTable = arrayValueOf(stackValue(0));
> >         matchTable -= 1;
> > ...
> >         if (!(matchTable)) {
> > ...
> >
> > So, will the value of matchTable actually be NULL or not, when nil is
> > passed to the primitive. That -= 1 makes me think that it won't be NULL.
> >
> > Another thing is that the loop condition, while it's order of evaluation
> > is well defined in Smalltalk, will compile to C code containing undefined
> > behavior:
> >
> >                         while ((body[startIndex + index]) == (key[index +=
> >                         1])) {
> >
> > Since in C the evaluation of the left and right side of == can happen in
> > any order, the generated machine code may or may not be correct.
> > Is this is a difference between Smalltalk and Slang, or is it just a slip
> > in the code generator?
> >
> > Levente
> >
> > > (start max: 1) to: body size - key size + 1 do:
> > > [:startIndex |
> > > index := 1.
> > > [(matchTable at: (body at: startIndex+index-1)
> > > asciiValue + 1)
> > > = (matchTable at: (key at: index) asciiValue
> > > + 1)]
> > > whileTrue:
> > > [index = key size ifTrue: [^ startIndex].
> > > index := index+1]].
> > > ^ 0
> > > "
> > > ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 1 matchTable:
> > > CaseSensitiveOrder 1
> > > ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 2 matchTable:
> > > CaseSensitiveOrder 7
> > > ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 8 matchTable:
> > > CaseSensitiveOrder 0
> > > ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable:
> > > CaseSensitiveOrder 0
> > > ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable:
> > > CaseInsensitiveOrder 7
> > > "!

Reply | Threaded
Open this post in threaded view
|

Re: Slang question (was: Re: [squeak-dev] The Trunk: Collections-ul.685.mcz)

Levente Uzonyi
Hi Dave,

On Tue, 5 Apr 2016, David T. Lewis wrote:

> I just got around to checking this for code generation for the interpreter VM
> (VMMaker, not VMMaker.oscog).  The #ifNil: is not translated at all, so I replaced
> it with this:

That's pretty interesting. The Cog branch behaves differently

  matchTable ifNil: [

will become

  if (!matchTable) {

while

  matchTable = nil ifTrue: [
and
  matchTable == nil ifTrue: [

will both compile to

  if (matchTable == null) {

just like in the main branch.
All of these are correct, because null is just a macro to 0, but it's a
bit surprising that "ifNil:" and "== nil ifTrue:" are not the same in
Slang, while they are in Squeak.

My main concern is how matchTable is initialized. The generated code is

         matchTable = arrayValueOf(stackValue(0));
         matchTable -= 1;
         if (failed()) {
                 return null;
         }

which makes me think that matchTable will not be the C null pointer at
all when the argument nil is passed to the primitive, because

  matchTable -= 1;

would not yield an unsigned char* with 0 value unless arrayValueOf
returns a pointer with the value 1 (assuming that sizeof(unsigned char) is
1).
I haven't found where arrayValueOf was defined, so I can't tell if it has
a chance to work at all.

Levente

P.S.: Since we're talking about C here, I CC'd the vm-dev list.

>
> matchTable = nil ifTrue:
>
> This produces the expected result, and can be compiled.
>
> There are other issues in the resulting VM, but they do not seem to be
> related to Collections-ul.685 (I reverted to Collections-ul.684 with the
> same results). I don't know the cause of those issues, but I will follow
> up in a separate thread as needed.
>
> With respect to Collections-ul.685, replacing the ifNil: test as above
> seems like the right thing to do.
>
> Dave
>
>
> On Sun, Apr 03, 2016 at 06:00:13PM -0400, David T. Lewis wrote:
>> Many people will not be familiar with this optimization, so a few words of
>> explanation are in order for background:
>>
>> There is a mechanism for translating certain methods in the image directly
>> into primitives for performance. This is normally not noticed, but you will
>> find occasional refernces such at this one in the image:
>>
>>   findSubstring: key in: body startingAt: start matchTable: matchTable
>>        <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
>>
>> This is a method that first tries to call a version of itself that has
>> been translated to C. If the primitive fails, the normal Smalltalk method
>> is executed. The hidden tricky bit is that MiscPrimitivePlugin, which
>> lives in the VMMaker package, arranges for the #findSubstring:in:startingAt:matchTable:
>> method in the image to be translated to C and installed as a primitive.
>> This means that the generated VM is actually governed partly my methods
>> in the main image (not in the VMMaker package).
>>
>> It also means that if one of these methods is changed in the image, it
>> might accidentally result in unexpected (and possibly wrong) code being
>> generated for the plugin in the VM. That is the reason that Levente is
>> asking about the slang code generation in this case.
>>
>> Dave
>>
>>
>> On Sun, Apr 03, 2016 at 07:45:13PM +0200, Levente Uzonyi wrote:
>>> On Sun, 3 Apr 2016, [hidden email] wrote:
>>>
>>>> Levente Uzonyi uploaded a new version of Collections to project The Trunk:
>>>> http://source.squeak.org/trunk/Collections-ul.685.mcz
>>>>
>>>> ==================== Summary ====================
>>>>
>>>> Name: Collections-ul.685
>>>> Author: ul
>>>> Time: 3 April 2016, 3:21:51.035808 am
>>>> UUID: db8fd391-2306-4ccc-87d5-b5dee96a78ab
>>>> Ancestors: Collections-eem.684
>>>>
>>>> Use Spur's new character comparison abilities in some String methods.
>>>>
>>>> =============== Diff against Collections-eem.684 ===============
>>>>
>>>> Item was changed:
>>>> ----- Method: ByteString>>findSubstring:in:startingAt:matchTable: (in
>>>> category 'comparing') -----
>>>> findSubstring: key in: body startingAt: start matchTable: matchTable
>>>> "Answer the index in the string body at which the substring key
>>>> first occurs, at or beyond start.  The match is determined using
>>>> matchTable, which can be used to effect, eg, case-insensitive
>>>> matches.  If no match is found, zero will be returned.
>>>>
>>>> The algorithm below is not optimum -- it is intended to be
>>>> translated to C which will go so fast that it wont matter."
>>>> | index |
>>>> <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
>>>> <var: #key declareC: 'unsigned char *key'>
>>>> <var: #body declareC: 'unsigned char *body'>
>>>> <var: #matchTable declareC: 'unsigned char *matchTable'>
>>>>
>>>> key size = 0 ifTrue: [^ 0].
>>>> + matchTable ifNil: [
>>>> + start to: body size - key size + 1 do: [ :startIndex |
>>>> + index := 0.
>>>> + [ (body at: startIndex + index) == (key at: (index
>>>> := index + 1)) ] whileTrue: [
>>>> + index = key size ifTrue: [ ^startIndex ] ] ].
>>>> + ^0 ].
>>>
>>> I wonder if the #ifNil: check is correct in Slang. The generated code is
>>>
>>> ...
>>>     unsigned char *matchTable;
>>> ...
>>>         matchTable = arrayValueOf(stackValue(0));
>>>         matchTable -= 1;
>>> ...
>>>         if (!(matchTable)) {
>>> ...
>>>
>>> So, will the value of matchTable actually be NULL or not, when nil is
>>> passed to the primitive. That -= 1 makes me think that it won't be NULL.
>>>
>>> Another thing is that the loop condition, while it's order of evaluation
>>> is well defined in Smalltalk, will compile to C code containing undefined
>>> behavior:
>>>
>>>                         while ((body[startIndex + index]) == (key[index +=
>>>                         1])) {
>>>
>>> Since in C the evaluation of the left and right side of == can happen in
>>> any order, the generated machine code may or may not be correct.
>>> Is this is a difference between Smalltalk and Slang, or is it just a slip
>>> in the code generator?
>>>
>>> Levente
>>>
>>>> (start max: 1) to: body size - key size + 1 do:
>>>> [:startIndex |
>>>> index := 1.
>>>> [(matchTable at: (body at: startIndex+index-1)
>>>> asciiValue + 1)
>>>> = (matchTable at: (key at: index) asciiValue
>>>> + 1)]
>>>> whileTrue:
>>>> [index = key size ifTrue: [^ startIndex].
>>>> index := index+1]].
>>>> ^ 0
>>>> "
>>>> ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 1 matchTable:
>>>> CaseSensitiveOrder 1
>>>> ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 2 matchTable:
>>>> CaseSensitiveOrder 7
>>>> ' ' findSubstring: 'abc' in: 'abcdefabcd' startingAt: 8 matchTable:
>>>> CaseSensitiveOrder 0
>>>> ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable:
>>>> CaseSensitiveOrder 0
>>>> ' ' findSubstring: 'abc' in: 'abcdefABcd' startingAt: 2 matchTable:
>>>> CaseInsensitiveOrder 7
>>>> "!
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Slang question (was: Re: [squeak-dev] The Trunk: Collections-ul.685.mcz)

Bert Freudenberg
In reply to this post by Levente Uzonyi

> On 03.04.2016, at 19:45, Levente Uzonyi <[hidden email]> wrote:
>
> On Sun, 3 Apr 2016, [hidden email] wrote:
>
>> Levente Uzonyi uploaded a new version of Collections to project The Trunk:
>> http://source.squeak.org/trunk/Collections-ul.685.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Collections-ul.685
>> Author: ul
>> Time: 3 April 2016, 3:21:51.035808 am
>> UUID: db8fd391-2306-4ccc-87d5-b5dee96a78ab
>> Ancestors: Collections-eem.684
>>
>> Use Spur's new character comparison abilities in some String methods.
>>
>> =============== Diff against Collections-eem.684 ===============
>>
>> Item was changed:
>> ----- Method: ByteString>>findSubstring:in:startingAt:matchTable: (in category 'comparing') -----
>> findSubstring: key in: body startingAt: start matchTable: matchTable
>> "Answer the index in the string body at which the substring key first occurs, at or beyond start.  The match is determined using matchTable, which can be used to effect, eg, case-insensitive matches.  If no match is found, zero will be returned.
>>
>> The algorithm below is not optimum -- it is intended to be translated to C which will go so fast that it wont matter."
>> | index |
>> <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
>> <var: #key declareC: 'unsigned char *key'>
>> <var: #body declareC: 'unsigned char *body'>
>> <var: #matchTable declareC: 'unsigned char *matchTable'>
>>
>> key size = 0 ifTrue: [^ 0].
>> + matchTable ifNil: [
>> + start to: body size - key size + 1 do: [ :startIndex |
>> + index := 0.
>> + [ (body at: startIndex + index) == (key at: (index := index + 1)) ] whileTrue: [
>> + index = key size ifTrue: [ ^startIndex ] ] ].
>> + ^0 ].
>
> I wonder if the #ifNil: check is correct in Slang.
It’s not correct. Arguably the translation for MiscPrims should deal with it but then there would be special semantics for that, which is probably not a good idea. The way to check arguments in Slang is to see if they have the number of pointer slots you’re expecting.

Besides, for ByteStrings this primitive is always called with a proper matchTable. No check needed.

- Bert -



- Bert -






smime.p7s (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

VM included methods (was: Slang question (was: Re: [squeak-dev] The Trunk: Collections-ul.685.mcz))

David T. Lewis
On Fri, Apr 08, 2016 at 12:21:52PM +0200, Bert Freudenberg wrote:
>
> > On 03.04.2016, at 19:45, Levente Uzonyi <[hidden email]> wrote:

<snip>

> >
> > I wonder if the #ifNil: check is correct in Slang.
>
> It???s not correct. Arguably the translation for MiscPrims should deal with it but then there would be special semantics for that, which is probably not a good idea. The way to check arguments in Slang is to see if they have the number of pointer slots you???re expecting.
>
> Besides, for ByteStrings this primitive is always called with a proper matchTable. No check needed.
>

I think we're going to need to take a closer look at the various methods in
trunk that are translated for the VM. It may be that e.g. image changes for
immediate characters are changing the generated plugins in ways that are
going unnoticed.

It also may be that some of the primitives are simply not needed for Cog/Spur,
so some performance checks may be in order there. Given that the jit is
already optimizing the methods, does translating those methods to primitives
still provide any additional benefit?

The VM plugins that make use of methods translated from the image are
MiscPrimitivePlugin, SoundGeneratorPlugin, and ADPCMCodecPlug.

Dave


Reply | Threaded
Open this post in threaded view
|

Re: Slang question (was: Re: [squeak-dev] The Trunk: Collections-ul.685.mcz)

Levente Uzonyi
In reply to this post by Bert Freudenberg
On Fri, 8 Apr 2016, Bert Freudenberg wrote:

>
>> On 03.04.2016, at 19:45, Levente Uzonyi <[hidden email]> wrote:
>>
>> On Sun, 3 Apr 2016, [hidden email] wrote:
>>
>>> Levente Uzonyi uploaded a new version of Collections to project The Trunk:
>>> http://source.squeak.org/trunk/Collections-ul.685.mcz
>>>
>>> ==================== Summary ====================
>>>
>>> Name: Collections-ul.685
>>> Author: ul
>>> Time: 3 April 2016, 3:21:51.035808 am
>>> UUID: db8fd391-2306-4ccc-87d5-b5dee96a78ab
>>> Ancestors: Collections-eem.684
>>>
>>> Use Spur's new character comparison abilities in some String methods.
>>>
>>> =============== Diff against Collections-eem.684 ===============
>>>
>>> Item was changed:
>>> ----- Method: ByteString>>findSubstring:in:startingAt:matchTable: (in category 'comparing') -----
>>> findSubstring: key in: body startingAt: start matchTable: matchTable
>>> "Answer the index in the string body at which the substring key first occurs, at or beyond start.  The match is determined using matchTable, which can be used to effect, eg, case-insensitive matches.  If no match is found, zero will be returned.
>>>
>>> The algorithm below is not optimum -- it is intended to be translated to C which will go so fast that it wont matter."
>>> | index |
>>> <primitive: 'primitiveFindSubstring' module: 'MiscPrimitivePlugin'>
>>> <var: #key declareC: 'unsigned char *key'>
>>> <var: #body declareC: 'unsigned char *body'>
>>> <var: #matchTable declareC: 'unsigned char *matchTable'>
>>>
>>> key size = 0 ifTrue: [^ 0].
>>> + matchTable ifNil: [
>>> + start to: body size - key size + 1 do: [ :startIndex |
>>> + index := 0.
>>> + [ (body at: startIndex + index) == (key at: (index := index + 1)) ] whileTrue: [
>>> + index = key size ifTrue: [ ^startIndex ] ] ].
>>> + ^0 ].
>>
>> I wonder if the #ifNil: check is correct in Slang.
>
> It’s not correct. Arguably the translation for MiscPrims should deal with it but then there would be special semantics for that, which is probably not a good idea. The way to check arguments in Slang is to see if they have the number of pointer slots you’re expecting.
>
> Besides, for ByteStrings this primitive is always called with a proper matchTable. No check needed.
That's right, but even the C code would become quicker if it wouldn't have
to translate the characters.

Levente

>
> - Bert -
>
>
>
> - Bert -
>
>
>
>