JuliaIO / VideoIO.jl

Reading and writing of video files in Julia via ffmpeg
https://juliaio.github.io/VideoIO.jl/stable
Other
126 stars 53 forks source link

Make accessing members of structs through pointers more efficient #252

Closed galenlynch closed 4 years ago

galenlynch commented 4 years ago

As part of #250 I benchmarked the performance of av_setfield, and observed that it currently takes ~380 ns. While this is fast, it's not as fast as I would expect for what should boil down to a few machine instructions. This is primarily due to the seemingly inherent type instability of accessing a field by its name. However, upon further reflection, this didn't make sense to me, as this should happen every time you access a field in Julia (e.g. foo.a which is translated to getproperty(foo, sym), which is in turn translated to getfield(foo, sym) where sym = :a). Unlike in the generic case, where you don't know ahead of time which field will be accessed, if the call occurs inside of a function, the field name is a constant which can be utilized by the compiler. I therefore wondered if my benchmarks in #250 were unnecessarily slow, and that calls to av_setfield inside of a function would be more performant because the symbol would be constant, and the data type of the field could then in principle be known by compiler. Simple testing of this idea revealed that it was not the case. However, the problematic line of av_setfield, S = fieldtype(T, name), could be made type stable in other functions. Why wasn't the compiler using the data type of the field in this case, when it can in other cases?

I realized that it was because av_setfield was not being inlined by the compiler, so that the constant field name inside a function could not being utilized to eliminate type instability. By adding the @inline compiler hint to av_setfield, the compiler now knows what c type the arguments must be cast to before storing it in the c struct, although this is only true if you use av_setfield inside of a function. This minimal change successfully eliminates type instability, and simple benchmarking shows that the original 383 ns run time of av_setfield is reduced to 34 ns if it is used inside of a function.

codecov[bot] commented 4 years ago

Codecov Report

Merging #252 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #252   +/-   ##
=======================================
  Coverage   77.00%   77.00%           
=======================================
  Files          14       14           
  Lines         600      600           
=======================================
  Hits          462      462           
  Misses        138      138           
Impacted Files Coverage Δ
src/util.jl 70.37% <ø> (ø)

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 b3dab39...99ab261. Read the comment docs.

IanButterworth commented 4 years ago

Wonderful. Very much appreciated