-
Notifications
You must be signed in to change notification settings - Fork 86
IO Streams in Standard Library #1620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some ideas to explore, but I think this is good to be merged and then we exlore later.
One major issue is whether we want parameterized types on elements; I don't think we'll ever have other element types (non-octet byte streams are mostly useless with customizable endianness support). In that case, we may expose TextualStream and BinaryStream externally, and keep the parameterized stream only for internal implementaiton. But that can come in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am blocking this on my review. I will try to finish my review in the next couple of days.
Performance is not great when reading millions of bytes compared to a plain CL stream. Here are some ways to improve:
With these changes we can get as good or better performance compared to CL |
* speeeed * fundeps are fun
Added some functions included in #1644, better to merge that first with its included tests I think. I need to clean up and benchmark write operations. |
Table of ContentsSetup
Read Functions
Write Functions
Read Benchmarks
Write Benchmarks
|
Awesome benchmarks. I'm surprised Coalton is faster than Lisp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some critical design questions to answer. Perhaps the biggest one is whether the stream types offered should be parameterized on type.
(coalton-toplevel | ||
(repr :native cl:stream) | ||
(define-type (InputStream :elt) | ||
"A stream that can be read from.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to promise that these are Lisp streams so that they can be passed into lisp
forms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question remains. If so, we should document it in the type here, and also in the interop doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the (repr :native cl:stream)
promise this?
Signals a condition on error." | ||
(:stream :elt -> :elt))) | ||
|
||
(define-class (IntoPeekable :stream :elt :peekablestream (:stream -> :peekablestream)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should make this a type class; we might provide some functions to take our offered stream data types and turn them into peekable streams. This class suggests we will have all sorts of (stream, peekable-stream) pairs to extend with in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any new integer-type stream might want to implement make-peekable
(define (read stream) | ||
"Consume 1 element from a stream." | ||
(catch (Some (read-unchecked stream)) | ||
(_ None))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we would limit to the cl:end-of-file
condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a way to catch specific CL conditions yet
;; High Level Functions | ||
;; | ||
|
||
(coalton-toplevel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the below functions should be a part of this module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the resolution on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the tokenizing read functions with the understanding that boolean predicate read-to functions were still desired
|
||
(coalton-toplevel | ||
(declare stdout (Unit -> OutputStream :elt)) | ||
(define (stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not type safe.
- We don't know what kind of stream
*standard-output*
will be bound to when this call is made. - Even if we did, the function is presenting itself as polymorphic on the stream element type.
The standard is kind of bad when it comes to specifying streams, but we can be guaranteed *terminal-io*
won't be re-bound, and we can basically guarantee all such streams will be character streams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:elt
below needs to be changed.
…ed function feels right in coalton...
(coalton-toplevel | ||
(repr :native cl:stream) | ||
(define-type (InputStream :elt) | ||
"A stream that can be read from.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question remains. If so, we should document it in the type here, and also in the interop doc.
(stream (IOStream :elt)) | ||
(buffer (vec:Vector :elt))) | ||
|
||
(define-class (Closable :stream :elt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see :elt
as superfluous here. Just :stream
should work.
"Consume 1 element from a stream. | ||
|
||
Signals a condition on error." | ||
(:stream :elt -> :elt)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have :stream -> :elt
, and not have :stream
be higher-kinded.
"Consume `n` elements from a stream and destructively fill array." | ||
(:stream :elt -> array:LispArray :elt -> UFix))) | ||
|
||
(define-class (Writable :stream :elt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as Readable.
"Flush buffered output." | ||
(:stream :elt -> Unit))) | ||
|
||
(define-class (Readable :stream :elt => Peekable :stream :elt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as Readable.
(define (unread stream elt) | ||
(vec:push! elt (.buffer stream)) | ||
Unit) | ||
(define (peek-unchecked stream) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the resolution here?
library/stream.lisp
Outdated
(vec:push! elt (.buffer stream)) | ||
elt)))) | ||
|
||
(define-instances (Readable (PeekableInputStream U8) (PeekableIOStream U8)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be specialized on U8?
library/stream.lisp
Outdated
(vec:clear! (.buffer stream)) | ||
result)))) | ||
|
||
(define-instance (Writable PeekableIOStream U8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question for everything below. Does it need to be specialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, although right now I can't figure out how to define the instances to do it correctly
;; High Level Functions | ||
;; | ||
|
||
(coalton-toplevel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the resolution on this?
|
||
(coalton-toplevel | ||
(declare stdout (Unit -> OutputStream :elt)) | ||
(define (stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:elt
below needs to be changed.
Some improvements made, I am struggling to figure out how to reduce the kind arity though |
This is a stream library pulled from https://github.com/garlic0x1/coalton-streams. I think it ought to be in the standard library since most basic IO will need something like this and I think the current file streams are insufficient.
TODO (maybe in later PRs?):
coalton-library/file
One cost of this PR is adding
flexi-streams
as a dependency to the standard library.