On Tue, May 31, 2016 at 06:12:53PM +0200, Levente Uzonyi wrote:
> On Tue, 31 May 2016, Chris Muller wrote:
> 
> >>It's an optimization, so it's not mandatory. Of course it's nice to have,
> >
> >An optimization of what?  Execution speed?  Is it such an optimization
> >that any human could ever possibly notice?  Setting the mode of a
> >stream, really?
> 
> I suspect that you expect one to set mode at most once per stream, 
> which is not always the case.
> I could easily give you an artificial benchmark showing 30x speedup, but 
> that would make no sense.
> It's a library method and as such, why not make it quicker when possible?
> 
> >
> >I like most of your optimizations but, IMO, this one is so
> >infinitesmal, I don't think it exceeds the cost of the decreased
> >legibility / complexity..
> 
> Please explain the legibility / complexity part.
>

I don't think there is any problem with complexity or legibility here.
The original use of "collection asByteArray" is very readable. The faster
"ByteArray adoptInstance: collection" may require a little more thought,
but both are clear enough in this context.

That said, in the case of setting a stream to #binary or #ascii, the
#adoptInstance: approach does seem fragile compared to #asByteArray.
If Behavior>>adoptInstance: fails, the fallback code attempts to use
Object>>primitiveChangeClassTo: which also fails, this time with no fallback
path. Thus if the primitives for changing the class of an object fail,
there is no fallback and it may no longer be possible to write a new binary
file (such as new image and changes files).

Another way to look at it - if the optimization is worthwhile, maybe it
belongs in the Collections package, which already needs to be different
for Spur versus V3.

For Spur, shouldn't it just be this:

ByteString>>asByteArray
     ByteArray adoptInstance: self

I did not really test this, but it should give us the same performance
optimization in a more general way.

Dave