The Trunk: Collections-tfel.613.mcz

Tim Felgentreff uploaded a new version of Collections to project The Trunk:
http://source.squeak.org/trunk/Collections-tfel.613.mcz

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

Name: Collections-tfel.613
Author: tfel
Time: 9 April 2015, 9:09:08.368 am
UUID: 9e6e47d3-17ac-f847-976f-40872d002ab8
Ancestors: Collections-bf.612

Fix replaceFrom:to:with:startingAt: for ByteArray if run on a VM without primitive 105

=============== Diff against Collections-bf.612 ===============

Item was added:
+ ----- Method: ByteArray>>at:put: (in category 'accessing') -----
+ at: index put: value
+ <primitive: 61> "try primitiveAtPut, convert value to integer if that fails and try again"
+ ^ self byteAt: index put: value asInteger
+ !

Item was changed:
  ----- Method: ByteArray>>byteAt: (in category 'accessing') -----
  byteAt: index
  <primitive: 60>
+ ^ super at: index!
- ^self at: index!

Item was changed:
  ----- Method: ByteArray>>byteAt:put: (in category 'accessing') -----
  byteAt: index put: value
  <primitive: 61>
+ ^ super at: index put: value!
- ^self at: index put: value!

Egads!  This is a performance critical method that has now been slowed by half!

     old version:    '94,500,000 per second. 10.6 nanoseconds per run.'
   new version:     '49,400,000 per second. 20.3 nanoseconds per run.'

When it comes to a low level operation like putting bytes into a
ByteArray, the system should not be so forgiving if the user tried to
put an invalid object in there.

And look, it does not protect the user from doing:

     (ByteArray new: 1) at: 1 put: 300.

So what are we really "protecting" from here?

IMO, ByteArray's should be strict about users trying to manipulate
them, not halfway forgiving and halfway strict.  It should be the
_callers_ responsibility to send #asInteger if necessary.

> + !
>
> Item was changed:
>   ----- Method: ByteArray>>byteAt: (in category 'accessing') -----
>   byteAt: index
>         <primitive: 60>
> +       ^ super at: index!
> -       ^self at: index!

Since there is no new implementation of ByteArray>>#at:, I guess this
change makes no difference..?

> Item was changed:
>   ----- Method: ByteArray>>byteAt:put: (in category 'accessing') -----
>   byteAt: index put: value
>         <primitive: 61>
> +       ^ super at: index put: value!
> -       ^self at: index put: value!

Calling super for a different method -- smells.

This change could negatively affect Magma's performance. :-(   Please,
is there another way to accomplish what you want?

What exactly are you testing?

- Bert -

On Thu, Apr 9, 2015 at 10:11 AM, Bert Freudenberg <[hidden email]> wrote:
|ba| ba:= ByteArray new: 1.
[ba byteAt: 1 put: 200] bench

and

|ba| ba:= ByteArray new: 1.
[ba at: 1 put: 200] bench

With this bench code, there shouldn't be any difference at all if your VM has primitive 61. 200 actually is an integer, isn't it? :)

Best,
Marcel

Oh, 60, 61, ... misread the numbers. Please ignore.

Best,
Marcel

On 09.04.2015, at 17:16, Chris Muller <[hidden email]> wrote: These two are not equivalent. #at:put: is optimized by the VM.

I would be very surprised if Tim's changes have any effect on performance one way or other. They're purely about correctness.

To test, you would have to run the exact same code before and after updating.

- Bert -

I made a mistake in my earlier benching -- one image was Spur the
other was Cog, oops!

Still, there is a > 5% speed loss in byteAt:put:, which is strange
unless that prim is failing...

|ba| ba:= ByteArray new: 1.
[ba byteAt: 1 put: 200] bench

bf-612:  '92,700,000 per second. 10.8 nanoseconds per run.'
tfel.613:   '87,900,000 per second. 11.4 nanoseconds per run.'

What also is not so elegant is the inconsistency with ByteString.

     ByteString>>#byteAt:put:  ---> coercion on prim fail
     ByteArray>>#byteAt:put:   ---> no-coercion on prim fail

     ByteString>>#at:put:  ----> no-coercion on prim fail
     ByteArray>>#at:put:   ----> coercion on prim fail

IMHO, #at:put: should fail if the wrong type of argument is passed.

Also, I don't understand the change to ByteArray>>#byteAt:.  Since
there is no #at: implementation in ByteArray, isn't the change to call
super unnecessary..?

On Thu, Apr 9, 2015 at 11:19 AM, Bert Freudenberg <[hidden email]> wrote:

You're right the inconsistency isn't nice, but performance shouldn't be affected, so that is strange...

About the inconsistency, I don't really see that ByteString>>byteAt:put: should do coercion. If the caller already knows they're putting a byte, than make sure that you're doing that. On the other hand, at:put: is the generic interface and for objects that can only accept bytes here, they should a) fail when it's not a byte or b) be nice and try to coerce it.

My goal was to make the primitive truly optional again. That's what the test case is designed to check (although that also is also a bit brittle). I tried to find a small change that doesn't duplicate code and that passes the tests. Changing the fallback code in SequenceableCollection may also be possible, but I didn't want to add handling of byte subclasses there.

That being said, the fact that the primitive allows me to at:put: a Character into a ByteArray seems inconsistent in any case (an artifact of the underlying representation in the C?). If those characters are implicitly converted into SmallIntegers in the byte range, why then can't we also put Characters into WordArrays, for example?