Closed galenlynch closed 4 years ago
Merging #250 into master will decrease coverage by
0.02%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #250 +/- ##
==========================================
- Coverage 76.83% 76.80% -0.03%
==========================================
Files 14 14
Lines 600 595 -5
==========================================
- Hits 461 457 -4
+ Misses 139 138 -1
Impacted Files | Coverage Δ | |
---|---|---|
src/util.jl | 70.37% <100.00%> (-1.51%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 8849d0f...82ea173. Read the comment docs.
Good catch. It would be good to benchmark this before merging, I want to be sure that we don't impede performance (I agree with your logic, but still worth checking).
Would you mind constructing BenchmarTools.@btime
tests for frame read/write and compare to master? thanks!
I only measured the write functionality, which is the majority of this PR (though reads are also affected), but the PR is about 10 times faster, and uses less memory:
on master:
julia> using VideoIO, BenchmarkTools
julia> using VideoIO: avcodec_find_encoder_by_name, avcodec_alloc_context3, AVCodecContext, av_setfield
julia> codec_name = "libx264";
julia> codec = avcodec_find_encoder_by_name(codec_name);
julia> apCodecContext = Ptr{AVCodecContext}[avcodec_alloc_context3(codec)];
julia> width = 1024;
julia> @benchmark av_setfield(apCodecContext[1],:width,width)
BenchmarkTools.Trial:
memory estimate: 1.77 KiB
allocs estimate: 8
--------------
minimum time: 3.070 μs (0.00% GC)
median time: 3.141 μs (0.00% GC)
mean time: 3.364 μs (1.17% GC)
maximum time: 146.271 μs (95.39% GC)
--------------
samples: 10000
evals/sample: 8
for this PR:
...
julia> @benchmark av_setfield(apCodecContext[1],:width,width)
BenchmarkTools.Trial:
memory estimate: 96 bytes
allocs estimate: 6
--------------
minimum time: 384.736 ns (0.00% GC)
median time: 401.701 ns (0.00% GC)
mean time: 414.897 ns (0.39% GC)
maximum time: 4.677 μs (89.38% GC)
--------------
samples: 10000
evals/sample: 201
Ok great. Thanks
The current code to access a member of a struct pointer uses a package function,
fieldposition
, that has identical functionality to a function provided in base,fieldindex
. While this latter function is not exported, it is documented and will hopefully remain a part of Julia's API. I imagine that the base function is more performant than the implementation infieldposition
, which compares each field name to the target field name in order to find the position of a field. As such, I have replaced calls tofieldposition
with calls toBase.fieldindex
, and deleted the functionfieldposition
.Additionally, when setting a member of a struct pointer, the current implementation calculates the pointer position, wraps it in a julia array with
unsafe_wrap
, and then sets the first element of that array to the desired value. The same result is achieved, perhaps more simply, by usingunsafe_store!
and not using an intermediate array. I have therefore replaced the call tounsafe_wrap
and subsequent code withunsafe_store!
.While I do not know if the existing code causes a measurable decrease in performance, using the base functions reduces the amount of code, and most likely makes it faster.