Skip to content

Fix type instability in normals #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

mtsch
Copy link
Contributor

@mtsch mtsch commented Oct 5, 2020

I noticed mesh in Makie sometimes take a long time to plot. This is especially apparent when animating a large, growing mesh. The problem is a type instability in normals, which this PR fixes.

The only change I made is that I added a function barrier. This greatly improves performance (see below).

MWE:

using BenchmarkTools
using GeometryBasics

points = [Point(rand(), rand(), rand()) for _ in 1:200]
faces = typeof(GLTriangleFace(1,2,3))[]
for u in 1:200, v in u+1:200, w in v+1:200
    push!(faces, GLTriangleFace(u, v, w))
end

# Before this PR
@benchmark normals(points, faces) samples=10 seconds=10
# BenchmarkTools.Trial:
#  memory estimate:  761.56 MiB
#  allocs estimate:  14447403
#  --------------
#  minimum time:     607.730 ms (12.67% GC)
#  median time:      617.510 ms (12.60% GC)
#  mean time:        619.235 ms (12.62% GC)
#  maximum time:     637.565 ms (12.63% GC)
#  --------------
#  samples:          10
#  evals/sample:     1

# After this PR
@benchmark normals(points, faces) samples=10 seconds=10
# BenchmarkTools.Trial:
#  memory estimate:  400.82 MiB
#  allocs estimate:  2626801
#  --------------
#  minimum time:     156.252 ms (8.53% GC)
#  median time:      163.279 ms (8.52% GC)
#  mean time:        167.465 ms (8.39% GC)
#  maximum time:     193.659 ms (7.38% GC)
#  --------------
#  samples:          10
#  evals/sample:     1

@SimonDanisch
Copy link
Member

Cool, thanks a lot :)

@SimonDanisch SimonDanisch merged commit 3595616 into JuliaGeometry:master Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants