Add chomp option to readline(s) #19944

mpastell · 2017-01-09T15:57:41Z

This pull request adds chomp::Bool=false option to readline, readlines and eachline

This is "fresh start" based on discussion in #19877 .

The implementation is completely based on Julia, but it could be made a bit faster by modifying C code.

An example with a text file with 1e6 lines, this matches current master:

julia> @time readlines("../readlines/test.txt", false)
  0.213501 seconds (1.00 M allocations: 39.531 MB, 12.46% gc time)
1000000-element Array{String,1}:
 "Line1\n"      
 "Line2\n"

Setting chomp to true has 2x the number allocations and is therefore a bit slower:

julia> @time readlines("../readlines/test.txt", true)
  0.302754 seconds (2.00 M allocations: 70.049 MB, 22.71% gc time)
1000000-element Array{String,1}:
 "Line1"      
 "Line2"

nalimilan · 2017-01-09T16:06:09Z

base/io.jl

+    line = readuntil(s, 0x0a)
+    i = length(line)
+    if i < 1 || line[i] != 0x0a
+        return String(line[1:i])


Why not just do String(line)? This could also be merged with the simple case above, as getting the length of line should have a negligible cost.

nalimilan · 2017-01-09T16:11:02Z

base/io.jl

+    if i < 1 || line[i] != 0x0a
+        return String(line[1:i])
+    elseif i < 2 || line[i-1] != 0x0d
+        return String(line[1:i-1])


This unnecessarily creates a copy of line and explains the allocations you get. Use unsafe_string(pointer(line), i-1). Same below.

unsafe_string still allocates a copy. Maybe String(resize!(line, i-1))?

nalimilan · 2017-01-09T16:14:22Z

base/io.jl

-    readlines(filename::AbstractString)
+    line = readuntil(s, 0x0a)
+    i = length(line)
+    if i < 1 || line[i] != 0x0a


Probably clearer as i == 0? I wondered for one moment when it would be negative.

Also, it could be more efficient to start with the i >= 2 case, which is the common one, to avoid an unnecessary branch in that case.

Does the order branches really make a difference? I've sometimes tried to benchmark with cases I thought were very obvious, but didn't find a clear effect.

No idea, would need to check. You're right that it's probably going to be negligible except maybe for very short strings, so maybe it's OK to have the first branch precisely for them.

nalimilan · 2017-01-09T16:15:22Z

test/iobuffer.jl

@@ -74,6 +74,12 @@ Base.compact(io)
 @test_throws ArgumentError seek(io,0)
 @test_throws ArgumentError truncate(io,0)
 @test readline(io) == "whipped cream\n"
+@test write(io,"pancakes\nwaffles\nblueberries\n") > 0


Could you add a few non-ASCII characters like ë and €? This would ensure we don't assume 1-byte characters.

stevengj · 2017-01-09T16:34:08Z

base/io.jl

-    readline(stream::IO=STDIN)
-    readline(filename::AbstractString)
+    readline(stream::IO=STDIN, chomp::Bool=false)
+    readline(filename::AbstractString, chomp::Bool=false)


I wonder if chomp=true should be the default (here and in eachline)? Almost all uses of readline that I can find in the packages either (a) are insensitive to whether it ends with a newline or (b) explicitly strip the newline.

That would be a breaking change, but I doubt it would break much actual code, since most code calling readline or eachline has to handle the case where the last line in the file does not end in a newline anyway.

The plan was to merge this first, and then open a PR to deprecate the one-argument form of readline in 0.6. Then the default would be changed in 1.0.

We could make the breaking change if you say packages are fine, but the deprecation would allow people to notice they no longer need to remove trailing newlines, which will make code simpler/faster. Might be worth the inconvenience, not sure.

Okay. Would prefer to do the deprecation in the same PR, though.

OK. Let's ensure we're happy with the implementation, and then have a commit to make the switch.

+1 for making chomp=true the default. Does adding a deprecation mean fixing all of deprecation warnings from base as well? I'm afraid I won't have time to do it before 0.6 feature freeze.

Yes, but really that should be much easier than what you've done until now. If you really can't, I'll do it for you.

Thanks! I'll look into it first. I guess the easiest would be to change all the calls to readline to `readline(..., false).

How do you add a deprecation warning?

nalimilan · 2017-01-09T16:50:42Z

base/io.jl

+    readline(filename::AbstractString, chomp::Bool=false)
+
+Read a single line of text from the given I/O stream or file (defaults to `STDIN`).
+Lines in the input can end in `'\\n'` or `'\\r\\n'`. When reading from a file, the text is


Technically, should be "\r\n", so do that and also "\n".

While you're at it, the description of chomp would better be in a separate paragraph.

Thanks, it does look better with separate paragraphs

nalimilan · 2017-01-09T16:58:54Z

base/iostream.jl

+    s = ccall(:jl_readuntil, Ref{String}, (Ptr{Void}, UInt8, UInt8), s.ios, '\n', 1)
+    i = endof(s)
+    if i < 1 || codeunit(s,i) != 0x0a
+        return s[1:i]


We need to find a strategy similar to above to avoid a copy. But I'm not sure how to do that with the new String design.

BTW, is this expected? I thought no copy was involved when creating the string:

julia> x=UInt8['a', 'b'] 2-element Array{UInt8,1}: 0x61 0x62 julia> s=String(x) "ab" julia> x[1] = 'c' 'c': ASCII/Unicode U+0063 (category Ll: Letter, lowercase) julia> s # String hasn't changed "ab"

Unfortunately, String(::Vector) is no longer copy-free unless the vector is allocated with the (undocumented) Base.StringVector:

julia> x = Base.StringVector(2); x[1] = 'a'; x[2] = 'b' 'b': ASCII/Unicode U+0062 (category Ll: Letter, lowercase) julia> x = Base.StringVector(2); x[1] = 'a'; x[2] = 'b'; s = String(x) "ab" julia> x[1] = 'c' 'c': ASCII/Unicode U+0063 (category Ll: Letter, lowercase) julia> s "cb"

Right now, in order to avoid an extra copy, it seems like the chomping will have to be done in C, in jl_readuntil.

See also #19945.

For this PR at least, we don't need these functions to be exported since we're in Base. But here we would need jl_readuntil to return a StringVector. Is that possible?

jl_readuntil can return a String already if str=1 is passed (see also #19946). It would be easy to add an additional chomp argument to it that chomped LF/CRLF, and this might be the only way to completely avoid copies with the current String implementation.

Too bad. We may as well reintroduce jl_readline from #19877 for now, since @mpastell has already written it. It will be easier to remove when possible than modifying jl_readuntil.

nalimilan · 2017-01-09T17:02:47Z

base/iostream.jl

+    chomp || return ccall(:jl_readuntil, Ref{String}, (Ptr{Void}, UInt8, UInt8), s.ios, '\n', 1)
+
+    s = ccall(:jl_readuntil, Ref{String}, (Ptr{Void}, UInt8, UInt8), s.ios, '\n', 1)
+    i = endof(s)


endof is costly since it needs to look for the start of the last Unicode char. Here we're working at the byte level, so use sizeof(s) instead. Then, you cannot index directly into s as you might be in the middle of a character: you need to access the underlying array of bytes directly (cf. point below).

nalimilan · 2017-01-09T17:03:17Z

base/iostream.jl

+
+    s = ccall(:jl_readuntil, Ref{String}, (Ptr{Void}, UInt8, UInt8), s.ios, '\n', 1)
+    i = endof(s)
+    if i < 1 || codeunit(s,i) != 0x0a


Should add @inbounds here since you did the check manually. Same below.

ararslan · 2017-01-09T18:20:06Z

IMO the chomp option might be nice as a keyword argument for clarity.

Another option would be, rather than adding chomp as an option, add a function argument that applies the given function to each element in the resulting iterator. That is, the pattern would be readline(chomp, file), but would also allow things like readline(line -> parse(Int, line), file). (Though this pattern doesn't give you the opportunity to chomp by default.)

Apologies for the noise if the ship has already sailed on the design discussion.

stevengj · 2017-01-09T18:22:20Z

@ararslan, a function argument would not allow the chomp transformation to be copy-free.

nalimilan · 2017-01-09T18:22:27Z

@ararslan I think the idea is that people won't use that argument that often, it's mostly useful as a deprecation path in order to chomp by default.

ararslan · 2017-01-09T18:25:07Z

Okay, thanks for the explanation, both of you. Sorry for the noise! (I do still think a kwarg would be a nice choice, but I will now sink back into the shadows.)

nalimilan · 2017-01-09T18:27:06Z

The positional vs. keyword choice is a hard one. In general I'd also prefer using keyword arguments everywhere for booleans, but they come at a price currently, so they are seldom used in Base. Maybe something to think about for 2.0.

mpastell · 2017-01-09T18:44:12Z

I would also like to have a keyword argument, but @nalimilan pointed out that it comes with a performance penalty. As readline is called for each line in a file we can't afford the overhead (and I think the arguments to readlines and readline need to match.

mpastell · 2017-01-09T22:41:11Z

After the last two commits there is no difference in performance using chomp=false or true for IO or IOStream.

JeffBezanson · 2017-01-09T23:09:48Z

src/sys.c

@@ -292,6 +292,51 @@ JL_DLLEXPORT jl_value_t *jl_readuntil(ios_t *s, uint8_t delim, uint8_t str)
    return (jl_value_t*)a;
 }

+JL_DLLEXPORT jl_value_t *jl_readline(ios_t *s, uint8_t chomp)


This is a lot of near-duplicate code. Could we add an argument to jl_readuntil instead? Similarly ios_copyline does not seem totally necessary.

Agreed, I've changed it now and its much cleaner.

mpastell · 2017-01-10T07:52:53Z

Should I go ahead deprecate the old one argument version? What's the best practice here, should I rebase (and what's the correct git command) from master to avoid conflicts as readline is used in several places?

Or is there an easy way to check for conflicts for a file before modifying it?

tkelman · 2017-01-10T10:33:31Z

Since there are no conflicts here at the moment, you should be able to git fetch origin; git rebase origin/master smoothly on your local copy of this branch. That would be a good idea if you're about to modify more code. Then when you're ready to push you'll just need to push with --force over this branch on your fork to replace it with the rebased copy.

tkelman · 2017-01-10T10:34:31Z

base/io.jl

+"""
+function readlines(filename::AbstractString, chomp::Bool=false)
+    open(filename) do f
+          readlines(f, chomp)


indented a bit too far

tkelman · 2017-01-10T10:36:20Z

base/io.jl

 eltype(::Type{EachLine}) = String

-readlines(s=STDIN) = collect(eachline(s))
+readlines(s::IO=STDIN, chomp::Bool=false) = collect(eachline(s, chomp))


Would this ever make sense to send in to eachline on a non-IO first argument? If so this might be restricting vs what worked before.

As far as I can tell no (there is a separate definition for strings). This also matches documentation.

mpastell · 2017-01-10T12:02:23Z

I have no added deprecation warnings to old methods and tried to fix all warnings from master. I have done clean build locally from scratch, but tests are still running.

nalimilan · 2017-01-10T12:18:56Z

NEWS.md

@@ -165,6 +165,9 @@ Compiler/Runtime improvements
 Deprecated or removed
 ---------------------

+  * One argument methods to `readline`, `readlines` and `eachline` have been depracated in


"deprecated"

nalimilan · 2017-01-10T12:20:07Z

base/io.jl

-eachline(stream::IO) = EachLine(stream)
-function eachline(filename::AbstractString)
+eachline(stream::IO, chomp::Bool=true) = EachLine(stream, chomp)
+


A single new line is probably enough, isn't it?

Yes, thanks, fixed

nalimilan · 2017-01-10T12:25:27Z

src/support/ios.c

@@ -821,7 +821,8 @@ size_t ios_copyuntil(ios_t *to, ios_t *from, char delim)
        }
        else {
            size_t ntowrite = pd - (from->buf+from->bpos) + 1;
-            written = ios_write(to, from->buf+from->bpos, ntowrite);
+            size_t nchomp = ios_nchomp(from, ntowrite, chomp);


I'd check chomp here, and not pass it at all to ios_nchomp. That can avoid a function call when chomp is false. Then you can assume chomp is true inside ios_nchomp.

nalimilan · 2017-01-10T12:26:49Z

test/iobuffer.jl

 @test_throws ArgumentError seek(io,0)
 @test_throws ArgumentError truncate(io,0)
-@test readline(io) == "whipped cream\n"
+@test readline(io, false) == "whipped cream\n"
+@test write(io,"pancakes\nwaffles\nblueberries\n") > 0


Non-ASCII characters, please! :-)

ararslan · 2017-01-12T18:49:50Z

NEWS.md

@@ -33,6 +33,9 @@ Breaking changes

 This section lists changes that do not have deprecation warnings.

+  * `readline`, `readlines` and `eachline` return lines without line ends by default.
+     You can use `readline(s, false`) to get the old behavior and include EOL character(s). ([#19944]).


Misplaced closing backtick: should be after the )

tkelman · 2017-01-12T19:30:46Z

NEWS.md

@@ -33,6 +33,9 @@ Breaking changes

 This section lists changes that do not have deprecation warnings.

+  * `readline`, `readlines` and `eachline` return lines without line ends by default.


"line endings" instead of "line ends" might be clearer

nalimilan · 2017-01-13T14:16:57Z

@stevengj Great. If you post a list of affected packages here, I can help filing issues.

mpastell · 2017-01-13T14:56:45Z

I have updated the news according to comments. I think this should be ready to merge.

musm · 2017-01-13T15:07:41Z

test/repl.jl

-    readline(stdout_read)
-    readline(stdout_read)
+    readline(stdout_read, false)
+    readline(stdout_read, false)


would it also be a good idea to test the default behavior with readline(....) without line endings or is that already being tested?

It's tested in iobuffer and read

musm · 2017-01-13T15:08:48Z

base/libgit2/utils.jl

@@ -31,7 +31,7 @@ function prompt(msg::AbstractString; default::AbstractString="", password::Bool=
        Base.getpass(msg)
    else
        print(msg)
-        chomp(readline(STDIN))
+        readline(STDIN, true)
