Performance improvements

[barche]

This PR fixes an issue with a dynamic dispatch in the normals calculation and improves the performance of the merge_vertex_indices function. These problems were uncovered while working on PR JuliaGeometry/Meshes.jl#1405.

merge_vertex_indices

I kept the two commits for the two steps in the improvement: one is a simple type instability fix, the other a rewrite of the algorithm to avoid relying on a Dict.

Performance was tested using:

using BenchmarkTools
using Colors
using GeometryBasics

RGBf = RGB{Float32}

# Reduce quality of sphere
s = Tessellation(Sphere(Point3f(0), 1f0), 820)
ps = coordinates(s)
fs = faces(s)

FT = eltype(fs); N = length(fs)
cs = FaceView(rand(RGBf, N), [FT(i) for i in 1:N])

# generate normals per face (this creates a FaceView as well)
ns = face_normals(ps, fs)

# Create mesh
m = GeometryBasics.mesh(ps, fs, normal = ns, color = cs)
@benchmark expand_faceviews(m) setup=GC.gc()

Original code

BenchmarkTools.Trial: 4 samples with 1 evaluation per sample.
 Range (min … max):  1.068 s …   1.264 s  ┊ GC (min … max): 4.08% … 18.80%
 Time  (median):     1.099 s              ┊ GC (median):    4.49%
 Time  (mean ± σ):   1.133 s ± 89.476 ms  ┊ GC (mean ± σ):  8.39% ±  7.22%

  █   █        █                                          █  
  █▁▁▁█▁▁▁▁▁▁▁▁█▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁█ ▁
  1.07 s         Histogram: frequency by time        1.26 s <

 Memory estimate: 514.04 MiB, allocs estimate: 8048234.

Type stability fix (still using Dict)

BenchmarkTools.Trial: 8 samples with 1 evaluation per sample.
 Range (min … max):  462.373 ms … 480.578 ms  ┊ GC (min … max): 0.17% … 0.17%
 Time  (median):     467.321 ms               ┊ GC (median):    0.17%
 Time  (mean ± σ):   468.747 ms ±   5.567 ms  ┊ GC (mean ± σ):  0.15% ± 0.06%

  █        █ █   █ █     █        █                           █  
  █▁▁▁▁▁▁▁▁█▁█▁▁▁█▁█▁▁▁▁▁█▁▁▁▁▁▁▁▁█▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁█ ▁
  462 ms           Histogram: frequency by time          481 ms <

 Memory estimate: 348.12 MiB, allocs estimate: 110.

Final version

BenchmarkTools.Trial: 16 samples with 1 evaluation per sample.
 Range (min … max):  102.316 ms … 129.274 ms  ┊ GC (min … max): 0.45% … 0.38%
 Time  (median):     102.873 ms               ┊ GC (median):    0.44%
 Time  (mean ± σ):   104.660 ms ±   6.586 ms  ┊ GC (mean ± σ):  0.44% ± 0.02%

  ▁█                                                             
  ██▄▄▁▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▄ ▁
  102 ms           Histogram: frequency by time          129 ms <

 Memory estimate: 232.86 MiB, allocs estimate: 48.

Normals

The normals were tested using:

using BenchmarkTools
using Colors
using GeometryBasics

RGBf = RGB{Float32}

# Reduce quality of sphere
s = Tessellation(Sphere(Point3d(0), 1.0), 820)
fs = decompose(GLTriangleFace, s)
ps = coordinates(s)

@benchmark normals(ps, fs, normaltype = Vec3f) setup=GC.gc()
@benchmark face_normals(ps, fs) setup=GC.gc()

Original

Each time first normals and then face_normals:

BenchmarkTools.Trial: 19 samples with 1 evaluation per sample.
 Range (min … max):  48.783 ms … 57.149 ms  ┊ GC (min … max): 10.67% … 9.49%
 Time  (median):     49.487 ms              ┊ GC (median):    10.51%
 Time  (mean ± σ):   49.922 ms ±  1.844 ms  ┊ GC (mean ± σ):  10.46% ± 0.29%

  ▃ ██▃█                                                       
  █▁████▇▇▇▁▇▁▁▁▁▁▁▁▇▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▇ ▁
  48.8 ms         Histogram: frequency by time        57.1 ms <

 Memory estimate: 130.52 MiB, allocs estimate: 1341525.
BenchmarkTools.Trial: 16 samples with 1 evaluation per sample.
 Range (min … max):  72.708 ms … 79.799 ms  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     73.887 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   74.361 ms ±  1.689 ms  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▁ █  █ █ █▁        ██▁                                    ▁  
  █▁█▁▁█▁█▁██▁▁▁▁▁▁▁▁███▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁█ ▁
  72.7 ms         Histogram: frequency by time        79.8 ms <

 Memory estimate: 168.88 MiB, allocs estimate: 1341529.

After fix

BenchmarkTools.Trial: 23 samples with 1 evaluation per sample.
 Range (min … max):  9.380 ms …  9.615 ms  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     9.529 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   9.523 ms ± 61.771 μs  ┊ GC (mean ± σ):  0.00% ± 0.00%

                               █              █  ▃         ▃  
  ▇▁▁▁▇▁▁▁▁▁▁▁▁▁▁▁▁▁▇▁▁▇▁▁▇▁▁▁▁█▇▇▇▁▁▁▇▁▇▁▇▁▁▁█▁▁█▁▇▁▁▇▁▁▁▁█ ▁
  9.38 ms        Histogram: frequency by time        9.62 ms <

 Memory estimate: 7.70 MiB, allocs estimate: 3.
BenchmarkTools.Trial: 20 samples with 1 evaluation per sample.
 Range (min … max):  9.168 ms … 13.842 ms  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     9.286 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   9.625 ms ±  1.027 ms  ┊ GC (mean ± σ):  0.00% ± 0.00%

  █▄                                                          
  ██▆▄▁▁▁▄▆▁▁▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▄ ▁
  9.17 ms        Histogram: frequency by time        13.8 ms <

 Memory estimate: 46.06 MiB, allocs estimate: 7.

Use of CodeGlass

These problems were tracked down using CodeGlass. For example, the merge_vertex_indices code showed the following allocation behavior:

This indicated a problem converting from the OffsetIndex type, and also an inability to infer the return type of the inlined function in the get! function applied to the Dict.

For the normals, all the points were dynamically allocated:

This indicated that the compiler could not infer the correct getindex call, and forcing the index type to Int fixed that.

[ffreyer]

I reverted the converts on faces again since the StaticArrays pr got merged. (Also checked that performance is still good). Tests here should fail though, since we have some checks for face[face .== 1] which broke

[ffreyer]

This can be merged now, right?

[barche]

All good, thanks for the updates!