More efficient fix for #13559.

[hessammehr]

This fix causes far fewer allocations than the previous PR #13632. Performance identical to C code.

[tkelman]

for the third time, can we delete jl_ios_get_nbyte_int here now that it's unused?

[hessammehr]

@tkelman Sure, should I do that?

I did some more benchmarking and it looks like memory-wise we're on par with the C version, but about 3x slower for something like this:

function bench_readInt64()
       buf = Array{Int64}(1000000)
       f = open("/dev/zero", "r")
       for i = 1:1000000
              buf[i] = myread(f, Int64)
       end
       close(f)
       return buf
end

Perhaps someone more experienced can say if this is a reasonable tradeoff.

[tkelman]

Yes, searching all of github for uses of jl_ios_get_nbyte_int shows you're deleting the only one that isn't in a fork of Julia. If I'm wrong and it's still used somewhere in base that I can't find, then the tests would hopefully show it.

[hessammehr]

You're right. It would be a new pull request, is that okay? I don't know if there's such a thing as a followup pull request.

[tkelman]

I think it would be better as a separate commit within this PR.

[vtjnash]

just adding a cross-reference to the past PR: #12281

[hessammehr]

Since the C function is only used by this particular Julia function, a small change in the C code would fix #13559, without affecting the rest of Julia or performance. I will follow up with a separate pull request.

[tkelman]

You could also force push over the same branch, either here or reopen #13620 then force push to your patch-1 branch.

[tkelman]

The commit went through, but looks like it caused a hang on Travis (e: and appveyor). Do you have a local source build, do you see the same thing in tests? What platform are you on?

[hessammehr]

It could be loop I added. My computer just finished building LLVM; it'll be another ~20 min before I can confirm.

[nalimilan]

I must be missing something, the only commit I can see here changes the C code. Did you destroy the PR when you force-pushed?

[hessammehr]

Yes, this supersedes my previous attempt, which I still have on a local branch.

[tkelman]

also viewable at 93ac74d and ea7060c

[nalimilan]

OK.

The indentation fix didn't move the curly braces, though. Also, adding a test is the only way to make sure this doesn't regress in the future.

[hessammehr]

@nalimilan I looked at the rest of the file and it seems to use the same style for loops.

The build hangs on both my linux boxes as well. This might be too invasive of a change as ios_readprep is used in a few other places. I'll post a better solution in a little bit. Tests will follow.

[tkelman]

Could you squash the commits?

[hessammehr]

Sorry, but how would you recommend I go about doing that?

[tkelman]

You do git rebase -i and change from pick to squash on your latter two commits. Tests would also be good to add, if possible to do without draining entropy or requiring user input.

[hessammehr]

Got it, thanks.

The main difficulty with tests is that it is hard to make assumptions about /dev/random. On the one hand, you want to drain entropy at least at some point to cause a ios_prepread to read a partial sequence, e.g. 6 bytes instead of 8. Doing that without requiring user input is tricky. How about a reading from a FIFO as a mock /dev/random?

[tkelman]

If you can make a FIFO reproduce the conditions that were causing the EOFError without this PR, that sounds like a good test. Might have to be @unix_only if appveyor has any trouble with your test (worth running it through once to find out though)

[hessammehr]

FIFOs work like a charm. Should I put the tests in a new .jl file?

[tkelman]

nah, test/file.jl or test/iobuffer.jl should work fine

[hessammehr]

I have no idea how to do this in Windows, unfortunately. Would it be worth adding a non-blocking test for n-byte read that worked on all platforms? It wouldn't cover the RNG case but that problem doesn't affect in Windows anyway.

[tkelman]

Yeah if that wouldn't be much trouble to write, more tests is always good on principle.

[hessammehr]

Is there anything else that needs to be done? Docs?

[tkelman]

This is pretty subtle to be worth mentioning in the docs I think. Thanks for the PR!

@vtjnash does this look okay to merge?

[pao]

Thanks @hessammehr for pursuing this through the iterations. We appreciate the contribution!

[hessammehr]

You're welcome! This is a pretty fantastic community; thanks for being so patient with me everyone.

[tkelman]

Thoughts on whether this should be backported for 0.4.2?

[vtjnash]

looks safe