Improve performance of readuntil #20621

omus · 2017-02-16T05:14:44Z

Was looking into readuntil and found a way to improve performance. Below are some of the benchmarks I used for comparison:

julia> function readuntil_old(s::IO, r::AbstractString)
           l = length(r)
           if l == 0
               return ""
           end
           out = IOBuffer()
           m = Array{Char}(l)  # last part of stream to match
           t = collect(r)
           i = 0
           while !eof(s)
               i += 1
               c = read(s, Char)
               write(out, c)
               if i <= l
                   m[i] = c
               else
                   # shift to last part of s
                   for j = 2:l
                       m[j-1] = m[j]
                   end
                   m[l] = c
               end
               if i >= l && m == t
                   break
               end
           end
           return String(take!(out))
       end
readuntil_old (generic function with 1 method)

julia> function readuntil_new(s::IO, t::AbstractString)
           l = length(t)
           if l == 0
               return ""
           end
           out = IOBuffer()
           i = 1
           while !eof(s)
               c = read(s, Char)
               write(out, c)
               i = c == t[i] ? i + 1 : 1
               if i > l
                   break
               end
           end
           return String(take!(out))
       end

readuntil_new (generic function with 1 method)

julia> using BenchmarkTools

julia> str = String([rand('A':'Z', 50000); '1']);

julia> io = IOBuffer(str);

julia> goal = str[end-1000:end];
julia> b1 = @benchmark readuntil_old(seekstart($io), $goal);
julia> b2 = @benchmark readuntil_new(seekstart($io), $goal);
julia> judge(median(b2),median(b1))
BenchmarkTools.TrialJudgement: 
  time:   -95.01% => improvement (5.00% tolerance)
  memory: -10.88% => improvement (1.00% tolerance)

julia> goal = str[end:end];
julia> b1 = @benchmark readuntil_old(seekstart($io), $goal);
julia> b2 = @benchmark readuntil_new(seekstart($io), $goal);
julia> judge(median(b2),median(b1))
BenchmarkTools.TrialJudgement: 
  time:   +2.53% => invariant (5.00% tolerance)
  memory: -0.28% => invariant (1.00% tolerance)

julia> goal = str;
julia> b1 = @benchmark readuntil_old($(seekstart(io)), $goal);
julia> b2 = @benchmark readuntil_new($(seekstart(io)), $goal);
julia>judge(median(b2),median(b1))
BenchmarkTools.TrialJudgement: 
  time:   +4.36% => invariant (5.00% tolerance)
  memory: -85.45% => improvement (1.00% tolerance)

I'll add these benchmarks to BaseBenchmarks.jl

vtjnash · 2017-02-16T05:56:52Z

base/io.jl

-            m[l] = c
-        end
-        if i >= l && m == t
+        i = c == t[i] ? i + 1 : 1


Need to advance 'i' here by a whole character to handle Unicode. I think it'll be easiest to do that by using start/next/done instead of 1/+1/l

In the original code i represents a character number and not a index into the string.

vtjnash · 2017-02-16T06:03:06Z

I think this proposed algorithm would fail with a terminator like "aab" matching against "aaab" (or in general, any string where the first character is repeated anywhere else in the string)

omus · 2017-02-17T16:41:47Z

Benchmarks for latest version. Note the original benchmarks weren't rewinding the stream correctly so I also update the description's benchmarks.

julia> using BenchmarkTools
julia> str = String([rand('A':'Z', 50000); '1']);
julia> io = IOBuffer(str);

julia> goal = str[end-1000:end];
julia> b1 = @benchmark readuntil_old(seekstart($io), $goal);
julia> b2 = @benchmark readuntil_new(seekstart($io), $goal);
julia>judge(median(b2),median(b1))
BenchmarkTools.TrialJudgement: 
  time:   -95.66% => improvement (5.00% tolerance)
  memory: +5.27% => regression (1.00% tolerance)

julia> goal = str[end:end];
julia> b1 = @benchmark readuntil_old(seekstart($io), $goal);
julia> b2 = @benchmark readuntil_new(seekstart($io), $goal);
julia>judge(median(b2),median(b1))
BenchmarkTools.TrialJudgement: 
  time:   -16.59% => improvement (5.00% tolerance)
  memory: +0.00% => invariant (1.00% tolerance)

julia> goal = str;
julia> b1 = @benchmark readuntil_old(seekstart($io), $goal);
julia> b2 = @benchmark readuntil_new(seekstart($io), $goal);
julia>judge(median(b2),median(b1))
BenchmarkTools.TrialJudgement: 
  time:   +0.27% => invariant (5.00% tolerance)
  memory: +42.69% => regression (1.00% tolerance)

Memory regression is caused from the new array backtrack.

Heavily inspired by omus and #20621

omus · 2017-02-17T22:07:30Z

Travis failure appears unrelated.

Heavily inspired by omus and #20621

omus · 2017-03-23T20:20:05Z

O(n + k) version of the algorithm which computes backtracking information on-the-fly. I ended up storing the backtracking information in a SparseVector which is more memory efficient than an Vector and computationally faster than a Dict.

omus · 2017-03-23T20:20:50Z

I have a unicode test I want to add but currently that results in:

Error During Test
  Test threw an exception of type Base.UVError
  Expression: readuntil(io(t), s) == m
  read: network is down (ENETDOWN)

StefanKarpinski · 2017-03-23T21:28:57Z

Wat?

omus · 2017-03-23T21:30:45Z

It looks like the I/O producers "File" and "PipeEndpoint" choke with unicode input in test/read.jl

tkelman · 2017-03-23T21:46:59Z

JuliaStrings/LegacyStrings.jl#4

Heavily inspired by omus and #20621

Looking over my original test I realized it potentially could be offensive. I've used a different example to avoid any potential issues.

Caches backtracking information as it is needed. Using a SparseVector which has a lower memory footprint than Vector but is more performant than Dict.

Skip testing the I/O producers "File" and "PipeEndpoint" when working with unicode.

makes it possible to use readuntil with any array (indexable) object and optimizes a few more cases

vtjnash · 2017-09-19T17:25:45Z

@omus PTAL at my latest updates to your code :)

omus · 2017-09-19T17:42:47Z

Looks really solid. I'll try to dig up some of my old benchmarks and give this a spin.

vtjnash · 2017-09-19T18:03:47Z

To be slightly more fair to the current algorithm, I was using the following modified version:

function readuntil_old(s::IO, r::Vector{UInt8})
                         l = sizeof(r)
                         if l == 0
                             return ""
                         end
                         out = Base.StringVector(0)
                         m = Array{UInt8}(l)  # last part of stream to match
                         i = 0
                         while !eof(s)
                             i += 1
                             c = read(s, UInt8)
                             push!(out, c)
                             if i <= l
                                 m[i] = c
                             else
                                 # shift to last part of s
                                 for j = 2:l
                                     m[j-1] = m[j]
                                 end
                                 m[l] = c
                             end
                             if i >= l && m == r
                                 break
                             end
                         end
                         return String(out)
                     end
 @time let g2 = Vector{UInt8}(goal); for i in 1:100; readuntil_old(seekstart(io), g2); end; end

I also added the worst case to my tests, where the new algo in this PR really shines:

str = ("A" ^ 50000) * "B";
goal = ("A" ^ 5000) * "Z";

omus · 2017-09-19T18:52:28Z

I think you need to remove the String(...) from readuntil_old to make it a fair fight.

The only benchmark where the old algorithm was slightly faster is in this case:

goal = "A" ^ 50000;
str = "A" ^ 50000;

omus · 2017-09-19T18:52:54Z

I'll make a PR to BaseBenchmarks.

ararslan · 2017-09-22T00:54:40Z

Nanosoldier is now ready to run the readuntil benchmarks whenever you are.

omus · 2017-09-22T00:57:23Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2017-09-22T02:46:20Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

omus · 2017-09-25T13:28:02Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2017-09-25T17:26:18Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

vtjnash

please squash when merging

omus · 2017-09-28T15:27:18Z

The new changes look great!

omus added io Involving the I/O subsystem: libuv, read, write, etc. performance Must go faster labels Feb 16, 2017

vtjnash reviewed Feb 16, 2017

View reviewed changes

vtjnash added a commit that referenced this pull request Feb 17, 2017

Improve performance of readuntil using strings

2e4f28e

Heavily inspired by omus and #20621

vtjnash mentioned this pull request Feb 17, 2017

Improve performance of readuntil using strings (v2) #20656

Closed

omus pushed a commit that referenced this pull request Mar 23, 2017

Improve performance of readuntil using strings

2a449a2

Heavily inspired by omus and #20621

omus mentioned this pull request Apr 10, 2017

LibGit2 credential callback testing framework #20738

Merged

omus mentioned this pull request Apr 26, 2017

Test some libgit2 warnings #21566

Merged

tkelman mentioned this pull request Jul 7, 2017

Warnings during libgit2 tests #22702

Closed

omus mentioned this pull request Aug 2, 2017

Support using EOF char (^D) to abort credential prompt #23092

Merged

vtjnash force-pushed the cv/readuntil branch from a55cbf7 to 4237a46 Compare September 15, 2017 16:52

vtjnash added a commit that referenced this pull request Sep 15, 2017

Improve performance of readuntil using strings

2ca94ba

Heavily inspired by omus and #20621

omus and others added 8 commits September 18, 2017 15:11

Improve performance of readuntil using strings

99e6180

Add backtracking

7e3efb5

Improve performance of readuntil using strings

85d6d5e

Heavily inspired by omus and #20621

Revise test to be completely unoffensive

9f6d764

Looking over my original test I realized it potentially could be offensive. I've used a different example to avoid any potential issues.

readuntil with on-the-fly backtrack caching

e1d222a

Caches backtracking information as it is needed. Using a SparseVector which has a lower memory footprint than Vector but is more performant than Dict.

Add unicode test for readuntil

700771e

Skip testing the I/O producers "File" and "PipeEndpoint" when working with unicode.

Remove need for readuntil file

f012092

[wip] reduce code duplication, allow generalized Int indexes

1a94dc5

vtjnash force-pushed the cv/readuntil branch from 4237a46 to e2b5902 Compare September 19, 2017 03:04

refactor into single method instead of a type,

938793d

makes it possible to use readuntil with any array (indexable) object and optimizes a few more cases

vtjnash force-pushed the cv/readuntil branch from e2b5902 to 938793d Compare September 19, 2017 04:55

omus mentioned this pull request Sep 19, 2017

Add readuntil benchmarks JuliaCI/BaseBenchmarks.jl#119

Merged

vtjnash self-assigned this Sep 21, 2017

vtjnash approved these changes Sep 25, 2017

View reviewed changes

omus merged commit 056b374 into master Sep 28, 2017

omus deleted the cv/readuntil branch September 28, 2017 15:28

KristofferC mentioned this pull request Nov 28, 2017

random: introduce Sampler to formalize hooking into rand machinery #23964

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of readuntil #20621

Improve performance of readuntil #20621

omus commented Feb 16, 2017 •

edited

Loading

vtjnash Feb 16, 2017

omus Feb 17, 2017

vtjnash commented Feb 16, 2017

omus commented Feb 17, 2017 •

edited

Loading

omus commented Feb 17, 2017

omus commented Mar 23, 2017 •

edited

Loading

omus commented Mar 23, 2017

StefanKarpinski commented Mar 23, 2017

omus commented Mar 23, 2017

tkelman commented Mar 23, 2017

vtjnash commented Sep 19, 2017

omus commented Sep 19, 2017

vtjnash commented Sep 19, 2017

omus commented Sep 19, 2017

omus commented Sep 19, 2017

ararslan commented Sep 22, 2017

omus commented Sep 22, 2017

nanosoldier commented Sep 22, 2017

omus commented Sep 25, 2017

nanosoldier commented Sep 25, 2017

vtjnash left a comment

omus commented Sep 28, 2017

Improve performance of readuntil #20621

Improve performance of readuntil #20621

Conversation

omus commented Feb 16, 2017 • edited Loading

vtjnash Feb 16, 2017

Choose a reason for hiding this comment

omus Feb 17, 2017

Choose a reason for hiding this comment

vtjnash commented Feb 16, 2017

omus commented Feb 17, 2017 • edited Loading

omus commented Feb 17, 2017

omus commented Mar 23, 2017 • edited Loading

omus commented Mar 23, 2017

StefanKarpinski commented Mar 23, 2017

omus commented Mar 23, 2017

tkelman commented Mar 23, 2017

vtjnash commented Sep 19, 2017

omus commented Sep 19, 2017

vtjnash commented Sep 19, 2017

omus commented Sep 19, 2017

omus commented Sep 19, 2017

ararslan commented Sep 22, 2017

omus commented Sep 22, 2017

nanosoldier commented Sep 22, 2017

omus commented Sep 25, 2017

nanosoldier commented Sep 25, 2017

vtjnash left a comment

Choose a reason for hiding this comment

omus commented Sep 28, 2017

omus commented Feb 16, 2017 •

edited

Loading

omus commented Feb 17, 2017 •

edited

Loading

omus commented Mar 23, 2017 •

edited

Loading