beacon-biosignals / EDF.jl

Read and write EDF files in Julia
MIT License
18 stars 5 forks source link

avoid double buffering + slight API clean up #28

Closed jrevels closed 4 years ago

jrevels commented 4 years ago

Ideally, I would've tackled both of these in the review of #26, I just didn't have the foresight at the time. My bad for the API churn!

This makes things slightly more flexible/composable with existing Base.open methods rather than implementing our own. It also avoids an extra double-buffering that was happening while reading sample data.

master:

julia> @benchmark EDF.read("test/data/test.edf")
BenchmarkTools.Trial:
  memory estimate:  3.32 MiB
  allocs estimate:  6812
  --------------
  minimum time:     1.012 ms (0.00% GC)
  median time:      1.229 ms (0.00% GC)
  mean time:        1.653 ms (19.47% GC)
  maximum time:     9.163 ms (80.07% GC)
  --------------
  samples:          3017
  evals/sample:     1

this branch:

julia> @benchmark EDF.read("test/data/test.edf")
BenchmarkTools.Trial:
  memory estimate:  1.21 MiB
  allocs estimate:  7224
  --------------
  minimum time:     780.557 μs (0.00% GC)
  median time:      918.493 μs (0.00% GC)
  mean time:        1.109 ms (13.31% GC)
  maximum time:     10.500 ms (84.98% GC)
  --------------
  samples:          4495
  evals/sample:     1

Extra allocations might be attributable to the view usage, but haven't really dug into it. We could probably get the time/memory usage a bit farther down using this approach with a bit of profiling, but this is a strict improvement time/memory-wise so far.

OTDE commented 4 years ago

Some of the tests appear to be erroring. Related to the API changes on the latest commit, maybe?

jrevels commented 4 years ago

Looks like the read! method I was using actually only handles views on Julia >= v1.4; I bumped the minimum Julia version out of laziness. I'm fine with dropping lower Julia versions if other folks are okay with that.

OTDE commented 4 years ago

LGTM!!