StandardFileStream>>#readInto:startingAt:count: appears badly broken

The method, as stated by its comment, is supposed to answer the number
of bytes actually transferred from the file-system.  In fact, it
answers the size of the String input into the method.

Magma won't even come close to running in the trunk image with this
problem.  StandardFileStream>>#readInto:startingAt:count: is breaking
its contract.

Nicolas and/or Levente, can you help?  These recent changes you made
appear to be an optimization of FileStream?

Thanks,
  Chris

My sense is that StandardFileStream>>#readInto:startingAt:count: was
supposed to provide (outside) access to the primitive method,
#primRead:into:startingAt:count.  Now we don't have access to that
primitive at all without doing a bunch of other stuff.  So I'm
hard-pressed to believe this is really going to perform better...



On Mon, Mar 15, 2010 at 10:02 AM, Chris Muller <[hidden email]> wrote:

2010/3/15 Chris Muller <[hidden email]>:
> My sense is that StandardFileStream>>#readInto:startingAt:count: was
> supposed to provide (outside) access to the primitive method,
> #primRead:into:startingAt:count.  Now we don't have access to that
> primitive at all without doing a bunch of other stuff.  So I'm
> hard-pressed to believe this is really going to perform better...
>

Because all file stream are now buffered, you cannot access the
primitive directly.
The logical stream pointer (in the buffer) is not the file pointer.
In my XTream experiment I clearly dissociate buffered input from non
buffered, so you would have choice, but we cannot replace ugly huge
Stream hierarchy like that.

I thought I did fix it In my image I have:
^(self next: count into: byteArray startingAt: startIndex) size - startIndex + 1

and next:into:startingAt: just return a copy truncated to last byte
read, so it relates to size read.
This stupid API was here before us, so we are just maintaining compatibility...
If it would be just me, no doubt code would be different (can't speak
for Levente, but I guess same for him).

>> Nicolas and/or Levente, can you help?  These recent changes you made
>> appear to be an optimization of FileStream?
>>
>> Thanks,
>>  Chris
>>
>
>

> I thought I did fix it In my image I have:
> ^(self next: count into: byteArray startingAt: startIndex) size - startIndex + 1
>
> and next:into:startingAt: just return a copy truncated to last byte
> read, so it relates to size read.

I just committed a test demonstrating the bug to the trunk.  Please
see testReadIntoStartingAtCount.

> This stupid API was here before us, so we are just maintaining compatibility...

Glad to hear it, right now I just want compatibility too, to get moved to 3.11.

 - Chris

2010/3/15 Chris Muller <[hidden email]>:
OK, I see my workaround don't work in all cases.

next: n into: aString startingAt: startIndex
always answer aString if is success reading the n items, and it did it
previously, that did not change...
It truncates only if it fails to read requested count.
It makes sense only when used thru #nextInto:
On the other hand, current stupid implementation prevents performing a
useless copy, except near end of stream...

I will publish a new workaround.

Nicolas

> On the other hand, current stupid implementation prevents performing a
> useless copy, except near end of stream...

Yes, that's a big problem!  Not only do I no longer have access to the
primitive, it costs me an allocation every time I want just to read
from a file into MY ByteArray.  Aren't there any other solutions?  Can
we talk about this?

On Mon, 15 Mar 2010, Chris Muller wrote:

>> On the other hand, current stupid implementation prevents performing a
>> useless copy, except near end of stream...
>
> Yes, that's a big problem!  Not only do I no longer have access to the
> primitive, it costs me an allocation every time I want just to read
> from a file into MY ByteArray.  Aren't there any other solutions?  Can
> we talk about this?
>
>

Thanks for the bug report Chris and for fix Nicolas. I think we now have
the best we can. I doubt that losing direct access to the primitive
will be a bottleneck. On my pc, I could achieve 360MB/second read speed
with the preivous implementation of #next:into:startingAt: if the data was
already cached in memory. If you really have to access the primitive, you
can still do that by turning off the read buffer (#disableReadBuffering)
and use the primitive accessor #primRead:into:startingAt:count: directly.


Levente