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

Fix readstring benchmark, add readfloat64. #223

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tkoolen
Copy link

@tkoolen tkoolen commented Aug 4, 2018

g["read"] and g["readstring"] were using the same testbuf, which would be at end-of-file after either one was run.

The real reason for this PR is adding the Float64 benchmark though; I just found the readstring issue in the process. I'm quite unhappy with the fact that reading (and writing, but that's for a later time) primitive types allocates. I thought that maybe this would be resolved in 0.7 (elimination of the allocation of the Ref here), but I guess that's being explicitly prevented by the @noinline here.

BufferedStreams.jl has this implementation, which doesn't allocate. Is something like this acceptable for Base as well?

Result of running run(g["readfloat64"]) on my machine:

BenchmarkTools.Trial: 
  memory estimate:  19.53 KiB
  allocs estimate:  1250
  --------------
  minimum time:     15.996 μs (0.00% GC)
  median time:      17.784 μs (0.00% GC)
  mean time:        21.219 μs (10.89% GC)
  maximum time:     13.179 ms (99.83% GC)
  --------------
  samples:          10000
  evals/sample:     1

@ararslan ararslan requested a review from jrevels August 4, 2018 10:38
@jrevels
Copy link
Member

jrevels commented Aug 4, 2018

which would be at end-of-file after either one was run

IIUC, the benchmark kernel here does a seekstart at every invocation (which is also part of what this kernel was measuring).

That being said, I'm fine with this change (though we might want to decrease the buffer size if we're going to reallocate it for every sample).

@tkoolen
Copy link
Author

tkoolen commented Aug 6, 2018

perf_read! does a seekstart at the beginning, but perf_read! is not used for the readstring benchmark, only the read benchmark. So if the read benchmark is done before the readstring benchmark, testbuf will definitely be at eof, as witnessed by the too-good-to-be-true 80 ns benchmark time.

That said, I guess the implementation in this PR is also currently incorrect, as there can be multiple evaluations per sample. I'll fix it.

`perf_read!` does a `seekstart` at the beginning, but `perf_read!` was not used for the `readstring` benchmark, only the `read` benchmark. So if the `read` benchmark was done before the `readstring` benchmark, `testbuf` would definitely be at eof, as witnessed by the too-good-to-be-true 80 ns benchmark time.

The implementation in the previous commit was also incorrect, as there can be multiple evaluations per sample.

Also reuse `testbuf` again in response to JuliaCI#223 (comment).
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