Skip to content
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

Use File(RawFD(fd)) in preference to fdio(fd). #15091

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samoconnor
Copy link
Contributor

Use LibUV in preference to ios.c (following on from https://github.com/JuliaLang/julia/pull/9450/files#diff-ac5d350530574f3f82c860d1b963bc0aR240 ).

Includes @vtjnash's fix for #15088 taken from #15090.

Includes cmdlineargs.jl pipe deadlock fix per 31a0c55#commitcomment-16106258

@samoconnor
Copy link
Contributor Author

bump?

(Use LibUV in preference to ios.c).
@samoconnor
Copy link
Contributor Author

rebased

@vtjnash
Copy link
Member

vtjnash commented Mar 7, 2016

sorry for the slow response. the historic concern with making this change has been that fdio is locally buffered, while File is not. this may require some benchmarking to show that the performance is unchanged. I suspect it will probably require adding local buffering to ensure that performance is good for repeated small writes. I think now that UVStream has an implementation of write buffering, that can be cloned to File. Then it think it will also require figuring out how to implement a read buffer (readcsv would probably provide a good performance test of this). Then this can likely be merged.

@samoconnor
Copy link
Contributor Author

Hi @vtjnash.

Note that the change in this PR only effects mktemp() and the STDIN, STDOUT and STDERR streams, so any performance concerns are limited to use of those streams.

With regard to read and write buffering. What do you think about separating out the buffering in the style of BufferedInputStream and BufferedOutputStream ?

i.e. Make all the low-level IO implementations un-buffered, and wrap them with a generic buffering layer when buffering is desired.

I think it makes sense for high level user functions like open(filename) to return BufferedInputStream(File.open(filename)) rather than forcing the user to write BufferedInputStream all over the place. Most people want buffered IO most of the time.
However, It seems that it would be a good thing to separate buffering from low-level IO under-the-hood. This is partly just good separation of concerns, but it also ensures that an un-buffered IO interface is available when that is needed (e.g. unwanted read-ahead can be a problem depending on the type of low-level driver/pipe/socket etc).

@vtjnash
Copy link
Member

vtjnash commented Mar 8, 2016

What do you think about separating out the buffering in the style of BufferedInputStream and BufferedOutputStream ?

I'm open to being convinced otherwise, but I'm not a huge fan of this style, simply because of the verbosity. And as you pointed out, usually the user wants the buffered object anyways. Buffering a stream vs. a file is also potentially a slightly different operation. The design so far has been to use PipeBuffer internally as the optional nullable buffer, recognizing that buffering is generally an inherent property of using a file efficiently in most cases (while also limiting to a single type of buffer, unlike the layered approach). But I don't think anyone has put serious effort into trying to design the tradeoffs of that approach for File, so it would be nice to see an investigation of the usage and performance of the various options.

@samoconnor
Copy link
Contributor Author

That sounds fair enough. I think the verbosity issue and the implementaiton layering are seperate issues. but there is no doubt that there are cases where "Buffering a stream vs. a file is also potentially a slightly different operation." etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants