BaseBenchmarks.jl icon indicating copy to clipboard operation
BaseBenchmarks.jl copied to clipboard

Fix readstring benchmark, add readfloat64.

Open tkoolen opened this issue 7 years ago • 2 comments

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

tkoolen avatar Aug 04 '18 01:08 tkoolen

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).

jrevels avatar Aug 04 '18 17:08 jrevels

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.

tkoolen avatar Aug 06 '18 15:08 tkoolen