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

optimized copyuntil #76

Merged
merged 7 commits into from
Aug 14, 2023
Merged

optimized copyuntil #76

merged 7 commits into from
Aug 14, 2023

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jul 12, 2023

Speeds up copyuntil (and hence readuntil, readline, etcetera) in Julia 1.11 (via JuliaLang/julia#48273).

(I also verified locally that the new readlines tests pass with Julia master, and that the test indeed exercises the new copyuntil method.)

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.40% 🎉

Comparison is base (9602735) 87.32% compared to head (66c7cf2) 87.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   87.32%   87.72%   +0.40%     
==========================================
  Files           5        5              
  Lines         426      440      +14     
==========================================
+ Hits          372      386      +14     
  Misses         54       54              
Files Changed Coverage Δ
src/bufferedinputstream.jl 97.68% <100.00%> (+0.13%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stevengj stevengj requested a review from KristofferC July 12, 2023 21:45
@KristofferC
Copy link
Member

For posterity, what benchmark did you use and what were the results?

@stevengj
Copy link
Member Author

stevengj commented Jul 13, 2023

Using the benchmark from JuliaLang/julia#48273 (comment) (with readuntil! updated to copyuntil), I got

PRIOR TO THIS PR:

  • sum(length, eachline(seekstart(bio), keep=true)) (allocating a String per line): 3.363 ms (31055 allocations: 2.44 MiB)
  • doit!((seekstart(bio); bio), buf, '\n') (re-using a StringView): 2.874 ms (0 allocations: 0 bytes)

AFTER THIS PR:

  • sum(length, eachline(seekstart(bio), keep=true)) (allocating a String per line): 2.048 ms (31052 allocations: 2.44 MiB)
  • doit!((seekstart(bio); bio), buf, '\n') (re-using a StringView): 1.666 ms (0 allocations: 0 bytes)

which is a nice improvement but not overwhelming since the fallback method of character-by-character reading of a BufferedInputStream is not too terrible.

@KristofferC
Copy link
Member

Side note, it doesn't feel like the tests of this package should have to take this long.

@KristofferC KristofferC merged commit 5b15938 into master Aug 14, 2023
@stevengj stevengj deleted the copyuntil branch November 28, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants