Discussion - 1.2 io

[Sean Devlin]

IO specific discussion

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To post to this group, send email to cloju...@googlegroups.com.
To unsubscribe from this group, send email to clojure-dev...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.

[Chas Emerick]

Regarding c.c.io/writer, I'll again second David and Phil's suggestion
that it should return non-error-swallowing Writers, rather than
PrintWriters. Original discussion here:

http://groups.google.com/group/clojure-dev/browse_frm/thread/99aa2b3263a0b374

FWIW, here's a couple other recent threads about the use of
PrintWriters:

- in c.c.sql and swank: http://groups.google.com/group/clojure/browse_frm/thread/472afcc2a8b79035

- what concrete type *out* and *err* really should be: http://groups.google.com/group/clojure/browse_frm/thread/3abccdf28bb7ea0

I'd only point out (repeating Phil here, I think) that c.c.io/writer
is used in connection with general-purpose IO, so it really shouldn't
overspecialize its functionality to suit usage for *out* and *err*.

Cheers,

- Chas

[Sean Devlin]

Last time I checked, reader, writer, etc. were implemented as
multimethods. Should they ship as a protocol now? Whatever decision
is made in the next few days will be followed for a long time...

On Apr 22, 9:18 pm, Chas Emerick <cemer...@snowtide.com> wrote:
> Regarding c.c.io/writer, I'll again second David and Phil's suggestion  
> that it should return non-error-swallowing Writers, rather than  
> PrintWriters.  Original discussion here:
>

> http://groups.google.com/group/clojure-dev/browse_frm/thread/99aa2b32...

>
> FWIW, here's a couple other recent threads about the use of  
> PrintWriters:
>

> - in c.c.sql and swank:http://groups.google.com/group/clojure/browse_frm/thread/472afcc2a8b7...

[Chas Emerick]

I'd agree. `class` is the dispatch fn, so it's a perfect fit for
protocols. Is there a good reason why reader/writer should continue
to be multimethods?

Any objections to me opening a ticket with a patch?

- Chas

[Stuart Halloway]

Go for it!

[Stuart Sierra]

On Apr 24, 6:32 am, Sean Devlin <francoisdev...@gmail.com> wrote:
> Last time I checked, reader, writer, etc. were implemented as
> multimethods.  Should they ship as a protocol now?

Yes.
-SS

[Chas Emerick]

OK, I've completed a first cut of this change, along with the
PrintWriter purging, and a variety of other things that fell out of
that.

A ticket has been created, with an attached patch: https://www.assembla.com/spaces/clojure-contrib/tickets/77

For your easier reviewing pleasure, you can see the patch on my branch
here:

http://github.com/cemerick/clojure-contrib/commit/3f299db2a509bd235b4125c14b2a6f67f855eb50

For the lazier among us (;-), the changes so far:

- refactored input-stream, output-stream, reader, and writer into a
Streams protocol

- added appropriate protocol implementations for byte and character
arrays

- eliminated PrintWriter returns from writer impls, write-lines, and
spit

- added *buffer-streams* var in order to prevent redundant buffering,
and to allow users to get unbuffered streams if they really want to

- added copy implementations reading from character arrays

Worth noting is the *buffer-streams* addition -- once the input-stream
and output-stream multimethods were added, writer and reader were
return "double buffered" streams (e.g. a BufferedWriter wrapping a
BufferedOutputStream, etc). The default is now to buffer only at the
top-most call when possible: reader only buffers via a BufferedReader,
writer only buffers via a BufferedWriter, but direct input-stream and
output-stream calls still return BufferedInputStreams and
BufferedOutputStreams. This gets us back to the pre-input-stream/
output-stream state.

Feedback very welcome.

Cheers,

- Chas

[Stuart Halloway]

I don't see the patch. Ah, assembla usability. :-)

I'll apply this patch once it is up and I have tested it. Does anybody
(esp. tool authors) have any concern about tools blowing up because
they demand PrintWriters?

Stu

> to the pre-input-stream/output-stream state.

[Chas Emerick]

My bad -- I thought the "Submit Changes" button would upload the file
and set the status to "Test" in one shot. Should be up there now.

- Chas

On Apr 26, 2010, at 11:48 AM, Stuart Halloway wrote:

> I don't see the patch. Ah, assembla usability. :-)

[Stuart Halloway]

If you find a way to make two logically-related things happen in
Assembla with a single gesture please let me know. :-)

[Tom Faulhaber]

Please make sure the contrib tests keep working. In particular, pretty print. I think it should be fine, but if it's not, I'll apply the patch myself and figure it out.

On Mon, Apr 26, 2010 at 9:55 AM, Stuart Halloway <stuart....@gmail.com> wrote:

If you find a way to make two logically-related things happen in Assembla with a single gesture please let me know. :-)

My bad -- I thought the "Submit Changes" button would upload the file and set the status to "Test" in one shot.  Should be up there now.

- Chas

On Apr 26, 2010, at 11:48 AM, Stuart Halloway wrote:

I don't see the patch. Ah, assembla usability. :-)

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To post to this group, send email to cloju...@googlegroups.com.

To unsubscribe from this group, send email to clojure-dev...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To post to this group, send email to cloju...@googlegroups.com.
To unsubscribe from this group, send email to clojure-dev...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.

[Chas Emerick]

A full contrib build reports ~1200 tests pass clean here, no errors or
failures. I presume there's no separate tower of tests for pretty
print, or other libs/namespaces?

- Chas

On Apr 26, 2010, at 1:10 PM, Tom Faulhaber wrote:

> Please make sure the contrib tests keep working. In particular,
> pretty print. I think it should be fine, but if it's not, I'll apply
> the patch myself and figure it out.

[Tom Faulhaber]

Wow, someone's added more tests since I looked last!

Yeah, that should be good - the top-level build picks up all the component tests.

Thanks,

Tom

[B Smith-Mannschott]

The whole double buffering thing isn't quite what you might expect...

At http://github.com/cemerick/clojure-contrib.git, we find:
>
> commit 3f299db2a509bd235b4125c14b2a6f67f855eb50
> Author: Chas Emerick <ceme...@snowtide.com>
> Date: 2010-04-26 09:13:15 -0400
>

[... snip ...]

> - added *buffer-streams* var in order to prevent redundant
> buffering, and to allow users to get unbuffered streams if they
> really want to

[... snip ...]

> (def #^{:doc "When true, all Streams protocol method implementations should return
> buffered varieties of their respective return types (e.g.
> a BufferedWriter instead of an OutputStreamWriter), by either
> doing so directly or recursively calling back into the same
> Streams protocol method in order to dispatch to an implementation
> that exists solely for this purpose.
>
> Defaults to true. This primarily exists so as to avoid double-
> buffering at both the byte and character levels, though it can
> be bound in application code if you are really sure you don't want
> IO buffering for some reason."}
> *buffer-streams* true)

I did something similar back in november when I was adding
input-stream and friends to c.c.io (then duck-streams), but soon
discovered that the whole double buffering thing wasn't all it's
cracked up to be:

commit 6926aa526a5135c1ff31418d8b881db1997a142f
Author: Ben Smith-Mannschott <bsmit...@gmail.com>
Date: 2009-12-20 10:24:31 +0100

fix: no longer assume that it's best to only buffer once

Happily buffer at both the Character and Byte levels
----------------------------------------------------

Previously, we went to pains to buffer only once, reasoning that
buffering at both the InputStream and Reader levels when opening a
Reader is wasteful and that it's best to buffer at the top of the
stack (i.e. a BufferedReader on a plain InputStream). However, this
design choice may be based on an incorrect assumption:

We use a BufferedInputStream with a 128k buffer to increase the
number of bytes we’re reading from the disk at a time. The reason
we can’t just give the BufferedReader a large buffer is that
InputStreamReader reads in 8192 byte blocks so we really need that
buffer at the byte level.

http://meshy.org/2009/12/13/widefinder-2-with-clojure.html

Reading the source of InputStreamReader confirms
this. InputStreamReader uses a NIO StreamDecoder to do the actual byte
to character translation and this StreamDecoder is always created with
DEFAULT_BYTE_BUFFER_SIZE of 8192.

// Ben

[Stuart Halloway]

Now you guys are scaring me a little. :-)

I'd like to move forward on this by reviewing a patch that both of you
(Ben and Chas) agree handles buffering correctly.

Is the current patch good, or do we need more discussion and another
patch?

Stu

[Chas Emerick]

I was actually thinking of people wanting to use clojure in resource-
constrained environments when I wrote that (e.g. android, GAE, etc).
8K one way or the other and the additional processing associated with
buffering doesn't matter a whit for me, but it might when (hopefully!)
people start targeting android et al. in a serious way.

I'd think that someone looking to do very high-perf stuff won't be
using default impls of things like the stuff defined in Streams
anyway...?

- Chas

[B Smith-Mannschott]

I guess it's a question of goals and priorities. Here's my take:

c.c.io should have developer usability as its first priority. That is,
it should be simple to use correctly while not doing anything
gratuitously stupid. Those who really need high performance can reach
through to java.io or java.nio and talk to those classes directly. I'd
be happy with Chas' solution for what I see as c.c.io's field of
application. I just wanted to bring up this issue so that it's been
seen, considered an understood -- not as a veto.

It's just unfortunate that InputStreamReader always reads from the
underlying InputStream in 8K blocks regardless of the size of the
request made. I guess this makes a certain degree of sense since the
InputStream reader can't actually know how many bytes to read when it
gets a request to read "50'000 characters" since byte->char is not
one-to-one.

It should be possible to get "double buffering" for those who
want/need it by doing this:

(io/reader (io/input-stream "foo.txt"))

So there's that option for those who require it, yes?

// Ben

On Mon, Apr 26, 2010 at 22:44, Chas Emerick <ceme...@snowtide.com> wrote:
> I was actually thinking of people wanting to use clojure in

> resource-constrained environments when I wrote that (e.g. android, GAE,

[Chas Emerick]

In terms of workaround options, yes, explicitly nesting the reader and
input-stream calls would yield "double buffering". Alternatively/in
addition, I could make it so that a bound value on *buffer-streams* is
left untouched, so that:

(binding [*buffer-streams* true]
(io/reader "foo.txt"))

would work. No shorter than explicitly using input-stream, but
perhaps a little more intuitive?

To be clear, I haven't benchmarked the various flavors of streams and
readers this decade, so I'm more than happy to yield to anyone who has
in a serious way and thinks that the proposed default behaviour is, as
you say, gratuitously stupid. Perhaps someone who has some mobile
development experience knows what the actual impact of an additional
8K allocated per reader (for the BufferedInputStream) is?

The more I think about this, the more I think that my *buffer-streams*
mechanism is a premature optimization (that could be easily dropped in
later if necessary) -- just as those looking for high perf will likely
be using the java.n?io classes directly, those who are footprint-
constrained will likely be doing the same thing.

- Chas

[B Smith-Mannschott]

On Tue, Apr 27, 2010 at 12:12, Chas Emerick <ceme...@snowtide.com> wrote:
> In terms of workaround options, yes, explicitly nesting the reader and
> input-stream calls would yield "double buffering".  Alternatively/in
> addition, I could make it so that a bound value on *buffer-streams* is left
> untouched, so that:
>
> (binding [*buffer-streams* true]
>  (io/reader "foo.txt"))
>
> would work.  No shorter than explicitly using input-stream, but perhaps a
> little more intuitive?
>
> To be clear, I haven't benchmarked the various flavors of streams and
> readers this decade, so I'm more than happy to yield to anyone who has in a
> serious way and thinks that the proposed default behaviour is, as you say,
> gratuitously stupid.  Perhaps someone who has some mobile development
> experience knows what the actual impact of an additional 8K allocated per
> reader (for the BufferedInputStream) is?

"additional 8K allocated per reader" is not the actual problem, as I
see it. The problem is that even a InputStreamReader with a very
generous buffer will make requests no larger than 8K from the
underlying InputStream, so if that underlying InputStream is not
itself buffered, this means a lot of 8K read requests when reading in
larger chunks would likely have been faster.

Thoughts:

- If we're only going to buffer once, we should buffer at the stream
(byte) level, not the character (reader) level.

- I think your code buffers at the top of the stack (i.e. the reader),
as my november attempt did, but maybe I'm misreading.

- I think it's good to provide some buffering by default.

- Perhaps we should give control over the size of the buffer? i.e. let
*buffer-size*:

- a positive integer: buffer size in bytes
- 0, nil, false: no buffering
- :default use BufferedInputStream, without explicitly specifying a
buffer size.

By default *buffer-size* would be bound to :default.

(I'll hack up a sketch of this idea based on your patch and make it
available on a
github branch. Then we can look at it and see if the idea is any good.)

> The more I think about this, the more I think that my *buffer-streams*
> mechanism is a premature optimization (that could be easily dropped in later
> if necessary) -- just as those looking for high perf will likely be using

> the java.n?io classes directly, those who are footprint-constrained will

> likely be doing the same thing.

Yea, you could be right. The simplest solution would be to just always
used BufferedInputStream and BufferedReader accepting the default
buffer sizes given by the JVM. That may well be good enough.

[Stuart Sierra]

On Apr 27, 2:17 am, B Smith-Mannschott <bsmith.o...@gmail.com> wrote:
> c.c.io should have developer usability as its first priority. That is,
> it should be simple to use correctly while not doing anything
> gratuitously stupid. Those who really need high performance can reach
> through to java.io or java.nio and talk to those classes directly.

Yes, with an emphasis on not doing anything gratuitously stupid.
-SS

[B Smith-Mannschott]

On Tue, Apr 27, 2010 at 12:43, B Smith-Mannschott <bsmit...@gmail.com> wrote:

>  (I'll hack up a sketch of this idea based on your patch and make it
> available on a
>  github branch.  Then we can look at it and see if the idea is any good.)

I've started this, but am not happy with it yet. It's past my bed time
and I'd rather not check in complete garbage. This will have to wait
at least another 12 hours.

> On Tue, Apr 27, 2010 at 12:12, Chas Emerick <ceme...@snowtide.com> wrote:
>> The more I think about this, the more I think that my *buffer-streams*
>> mechanism is a premature optimization (that could be easily dropped in later
>> if necessary) -- just as those looking for high perf will likely be using
>> the java.n?io classes directly, those who are footprint-constrained will
>> likely be doing the same thing.
>
> Yea, you could be right. The simplest solution would be to just always
> used BufferedInputStream and BufferedReader accepting the default
> buffer sizes given by the JVM. That may well be good enough.

I've this idea sketched out on my branch:

http://github.com/bpsm/clojure-contrib/commits/just-default-buffering

You can find the relevant commit here:

http://github.com/bpsm/clojure-contrib/commit/5a0d1376ff2a9078b04f81e95b0600aefc9fc573

This solution has a pleasing simplicity. Once the other branch is
ready, I'll do some (laughably amateurish) benchmarking of the two
solutions to see if the complexity of giving control over buffering
pays for itself.

[B Smith-Mannschott]

On Tue, Apr 27, 2010 at 12:43, B Smith-Mannschott <bsmit...@gmail.com> wrote:

> - Perhaps we should give control over the size of the buffer? i.e. let
> *buffer-size*:
>
>  - a positive integer: buffer size in bytes
>  - 0, nil, false: no buffering
>  - :default use BufferedInputStream, without explicitly specifying a
> buffer size.
>
>  By default *buffer-size* would be bound to :default.
>
>  (I'll hack up a sketch of this idea based on your patch and make it
> available on a
>  github branch.  Then we can look at it and see if the idea is any good.)

I finally got around to trying this out, after being delayed by an
unexpected overnight at a local hospital. you'll find the results
here:

http://github.com/bpsm/clojure-contrib/tree/more-buffering-control

I'll be honest though, I don't like it. I think my solution is ugly
and over-complicated. I'd like to do some performance tests to see how
much difference varying buffer sizes actually make, but swank stopped
working for me tonight due to some change in contrib, and I'm still to
wiped out to go hunting:

[INFO] [clojure:swank {execution: default-cli}]
Exception in thread "main" java.lang.Exception: Name conflict, can't
def group-by because namespace: swank.util refers
to:#'clojure.core/group-by (util.clj:31)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:5285)
at clojure.lang.Compiler.analyze(Compiler.java:5099)
at clojure.lang.Compiler.analyze(Compiler.java:5060)

[Chas Emerick]

Yeah, I've fixed my clojure and contrib snapshots to a build from
Tuesday night until I have a chance to patch all our projects, their
dependencies, etc etc. :-)

Ben, I generally agree with you.

The more I think of it, the more it seems that this particular avenue
is one that requires some more input and consideration than we can
likely apply before 1.2 wraps up (not that I'm aware of the actual
timeframe people have in mind). An ill-conceived API at this point
will not be easily modified or removed, but we can revisit the issue
and add in a proper solution at any time down the road. Until then,
default buffering won't do any harm.

If there's consensus around that, I can resubmit the patch with the
*buffer-streams* var and associated buffer-control code elided.

- Chas

[Chas Emerick]

On Apr 29, 2010, at 2:18 PM, Chas Emerick wrote:

> The more I think of it, the more it seems that this particular
> avenue is one that requires some more input and consideration than
> we can likely apply before 1.2 wraps up (not that I'm aware of the
> actual timeframe people have in mind). An ill-conceived API at this
> point will not be easily modified or removed, but we can revisit the
> issue and add in a proper solution at any time down the road. Until
> then, default buffering won't do any harm.
>
> If there's consensus around that, I can resubmit the patch with the
> *buffer-streams* var and associated buffer-control code elided.

I've done this now. I think this puts #77 to bed.

- Chas

[Stuart Halloway]

Do you feel confident enough that we should still be looking at moving
this into Clojure?

Stu

[Rich Hickey]

On May 3, 2010, at 9:20 AM, Stuart Halloway wrote:

> Do you feel confident enough that we should still be looking at
> moving this into Clojure?
>

The existence of this question answers itself - we probably shouldn't
be. Usually releases are cut after some time of stability, not a rush
of change. In any case, I recommend a long release candidate period
for 1.2

Rich

[Chas Emerick]

The APIs themselves have proven themselves. If there's a bug
somewhere, it'd certainly be resolvable without resorting to an API
change.

As for the implementation changes -- well, they're straightforward
enough, and tend to yield more strictly-correct results than before
(i.e. the PrintWriter purge). That said, that may or may not be
relevant in whatever criteria you, Rich, et al. would apply.

- Chas

On May 3, 2010, at 9:20 AM, Stuart Halloway wrote:

> Do you feel confident enough that we should still be looking at
> moving this into Clojure?
>
> Stu
>
>> On Apr 29, 2010, at 2:18 PM, Chas Emerick wrote:
>>
>>> The more I think of it, the more it seems that this particular
>>> avenue is one that requires some more input and consideration than
>>> we can likely apply before 1.2 wraps up (not that I'm aware of the
>>> actual timeframe people have in mind). An ill-conceived API at
>>> this point will not be easily modified or removed, but we can
>>> revisit the issue and add in a proper solution at any time down
>>> the road. Until then, default buffering won't do any harm.
>>>
>>> If there's consensus around that, I can resubmit the patch with
>>> the *buffer-streams* var and associated buffer-control code elided.
>>
>> I've done this now. I think this puts #77 to bed.
>>
>> - Chas

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To post to this group, send email to cloju...@googlegroups.com.

[Stuart Sierra]

On May 3, 9:20 am, Stuart Halloway <stuart.hallo...@gmail.com> wrote:
> Do you feel confident enough that we should still be looking at moving  
> this into Clojure?

The more I think about this, the more I get the feeling that the API
for a "proper" I/O library should not look anything like what I wrote
a couple years ago in duck-streams, which was basically a bunch of
convenience wrappers for java.io.

Mind you, I have no idea what a proper library SHOULD look like.

-S

[Rich Hickey]

On May 3, 2010, at 10:09 AM, Chas Emerick wrote:

> The APIs themselves have proven themselves. If there's a bug
> somewhere, it'd certainly be resolvable without resorting to an API
> change.
>
> As for the implementation changes -- well, they're straightforward
> enough, and tend to yield more strictly-correct results than before
> (i.e. the PrintWriter purge). That said, that may or may not be
> relevant in whatever criteria you, Rich, et al. would apply.
>

Just to be clear, when I say we probably shouldn't be (moving io),
that doesn't mean we are not. We will just need a more extended beta
period for this release. Please continue your good work and we'll take
the next step of 'moving' to core.

Thanks,

Rich

[Rich Hickey]

On May 3, 2010, at 12:16 PM, Stuart Sierra wrote:

> On May 3, 9:20 am, Stuart Halloway <stuart.hallo...@gmail.com> wrote:
>> Do you feel confident enough that we should still be looking at
>> moving
>> this into Clojure?
>
> The more I think about this, the more I get the feeling that the API
> for a "proper" I/O library should not look anything like what I wrote
> a couple years ago in duck-streams, which was basically a bunch of
> convenience wrappers for java.io.
>
> Mind you, I have no idea what a proper library SHOULD look like.
>

Since this will end up being called clojure.java.io, will that not
limit its scope, and people's expectations? These *are* just utility
wrappers.

Rich

[Stuart Sierra]

On May 3, 3:59 pm, Rich Hickey <richhic...@gmail.com> wrote:
> > Mind you, I have no idea what a proper library SHOULD look like.
>
> Since this will end up being called clojure.java.io, will that not  
> limit its scope, and people's expectations? These *are* just utility  
> wrappers.

Yes, putting it under .java should suffice.

-S