data_limits dispatch for Mesh to omit slow iteration over buffer #2874

koehlerson · 2023-04-20T07:57:31Z

Description

This PR changes the performance of the following:

julia> begin
           fig = GLMakie.Figure()
           ax = GLMakie.LScene(fig[1, 1])
           @time meshplot =GLMakie.mesh!(ax, plotter.mesh)
           fig
       end
  0.637678 seconds (15.12 k allocations: 566.358 MiB)

to

julia> begin
           fig = GLMakie.Figure()
           ax = GLMakie.LScene(fig[1, 1])
           @time meshplot =GLMakie.mesh!(ax, plotter.mesh)
           fig
       end
0.186790 seconds (15.13 k allocations: 1.111 MiB)

by using extrema in data_limits instead of computing the bbox by iterating over mesh.position which is still expensive. I'm not sure tho what extrema uses under the hood the avoid the performance issues. I looked up the source code at

julia> @which extrema(plotter.physical_coords_mesh)
extrema(a::AbstractArray; dims, kw...) in Base at reducedim.jl:994

~~but I don't understand anything that's going on there :D~~

I included a function bbox_mesh that iterates over the coordinates and checks the bbox manually. There is some weird issue that if I move the function's body inside data_limits the for loop never stops. Not sure what's going on there

The PR is also beneficial for GeometryBasics.Meshes without ShaderAbstractions.Buffer. From this:

julia> not_buffered_mesh = GeometryBasics.Mesh(plotter.physical_coords,plotter.vis_triangles);

julia> begin
           fig = GLMakie.Figure()
           ax = GLMakie.LScene(fig[1, 1])
           @time meshplot =GLMakie.mesh!(ax, fake_mesh)
           fig
       end
  0.431182 seconds (15.12 k allocations: 1.111 MiB)

to

julia> begin
           fig = GLMakie.Figure()
           ax = GLMakie.LScene(fig[1, 1])
           @time meshplot =GLMakie.mesh!(ax, fake_mesh)
           fig
       end
  0.209928 seconds (15.12 k allocations: 1.111 MiB)

Type of change

Delete options that do not apply:

Bug fix (non-breaking change which fixes an issue)

Checklist

Added an entry in NEWS.md (for new features and breaking changes)
Added or changed relevant sections in the documentation
Added unit tests for new algorithms, conversion methods, etc.
Added reference image tests for new plotting functions, recipes, visual options, etc.

… then the for loop never stops? sus

src/layouting/data_limits.jl

asinghvi17 · 2023-04-20T16:32:16Z

some errors in tests:

running  59: space 2D autolimits
space 2D autolimits: Error During Test at /home/runner/work/Makie.jl/Makie.jl/ReferenceTests/src/database.jl:45
  Test threw exception
  Expression: save_result(joinpath(RECORDING_DIR[], "space 2D autolimits"), result)
  MethodError: Cannot `convert` an object of type Tuple{Float32, Float32} to an object of type Float32
  Closest candidates are:
    convert(::Type{T}, ::Base.TwicePrecision) where T<:Number at twiceprecision.jl:273
    convert(::Type{T}, ::AbstractChar) where T<:Number at char.jl:185
    convert(::Type{T}, ::CartesianIndex{1}) where T<:Number at multidimensional.jl:130
    ...
  Stacktrace:
    [1] (::GeometryBasics.var"#23#34"{Float32, Tuple{Float32, Float32}})(i::Int64)
      @ GeometryBasics ~/.julia/packages/GeometryBasics/kt2Em/src/fixed_arrays.jl:45
    [2] ntuple
      @ ./ntuple.jl:50 [inlined]
    [3] Vec{3, Float32}(x::Tuple{Float32, Float32})
      @ GeometryBasics ~/.julia/packages/GeometryBasics/kt2Em/src/fixed_arrays.jl:45
    [4] convert
      @ ~/.julia/packages/StaticArrays/4uslg/src/convert.jl:176 [inlined]
    [5] HyperRectangle{3, Float32}(origin::StaticArraysCore.SizedVector{2, Float32, Vector{Float32}}, widths::StaticArraysCore.SizedVector{2, Float32, Vector{Float32}})
      @ GeometryBasics ~/.julia/packages/GeometryBasics/kt2Em/src/primitives/rectangles.jl:10
    [6] data_limits(plot::MakieCore.Mesh{Tuple{GeometryBasics.Mesh{2, Float32, TriangleP{2, Float32, PointMeta{2, Float32, Point{2, Float32}, (:uv, :normals), Tuple{Vec{2, Float32}, Vec{3, Float32}}}}, FaceView{TriangleP{2, Float32, PointMeta{2, Float32, Point{2, Float32}, (:uv, :normals), Tuple{Vec{2, Float32}, Vec{3, Float32}}}}, PointMeta{2, Float32, Point{2, Float32}, (:uv, :normals), Tuple{Vec{2, Float32}, Vec{3, Float32}}}, NgonFace{3, OffsetInteger{-1, UInt32}}, StructArrays.StructVector{PointMeta{2, Float32, Point{2, Float32}, (:uv, :normals), Tuple{Vec{2, Float32}, Vec{3, Float32}}}, NamedTuple{(:position, :uv, :normals), Tuple{Vector{Point{2, Float32}}, Vector{Vec{2, Float32}}, Vector{Vec{3, Float32}}}}, Int64}, Vector{NgonFace{3, OffsetInteger{-1, UInt32}}}}}}})
      @ Makie ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:215
    [7] (::Makie.var"#775#777"{Makie.var"#exclude#1397"{Int64}, Base.RefValue{HyperRectangle{3, Float32}}})(plot::MakieCore.Mesh{Tuple{GeometryBasics.Mesh{2, Float32, TriangleP{2, Float32, PointMeta{2, Float32, Point{2, Float32}, (:uv, :normals), Tuple{Vec{2, Float32}, Vec{3, Float32}}}}, FaceView{TriangleP{2, Float32, PointMeta{2, Float32, Point{2, Float32}, (:uv, :normals), Tuple{Vec{2, Float32}, Vec{3, Float32}}}}, PointMeta{2, Float32, Point{2, Float32}, (:uv, :normals), Tuple{Vec{2, Float32}, Vec{3, Float32}}}, NgonFace{3, OffsetInteger{-1, UInt32}}, StructArrays.StructVector{PointMeta{2, Float32, Point{2, Float32}, (:uv, :normals), Tuple{Vec{2, Float32}, Vec{3, Float32}}}, NamedTuple{(:position, :uv, :normals), Tuple{Vector{Point{2, Float32}}, Vector{Vec{2, Float32}}, Vector{Vec{3, Float32}}}}, Int64}, Vector{NgonFace{3, OffsetInteger{-1, UInt32}}}}}}})
      @ Makie ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:198
    [8] foreach(f::Makie.var"#775#777"{Makie.var"#exclude#1397"{Int64}, Base.RefValue{HyperRectangle{3, Float32}}}, itr::Vector{AbstractPlot})
      @ Base ./abstractarray.jl:2774
    [9] foreach_plot
      @ ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:109 [inlined]
   [10] foreach_plot
      @ ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:103 [inlined]
   [11] data_limits
      @ ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:196 [inlined]
   [12] getlimits(la::Axis, dim::Int64)
      @ Makie ~/work/Makie.jl/Makie.jl/src/makielayout/blocks/axis.jl:881
   [13] autolimits(ax::Axis, dim::Int64)
      @ Makie ~/work/Makie.jl/Makie.jl/src/makielayout/blocks/axis.jl:951
   [14] xautolimits
      @ ~/work/Makie.jl/Makie.jl/src/makielayout/blocks/axis.jl:982 [inlined]
   [15] reset_limits!(ax::Axis; xauto::Bool, yauto::Bool, zauto::Bool)
      @ Makie ~/work/Makie.jl/Makie.jl/src/makielayout/blocks/axis.jl:582
   [16] reset_limits!
      @ ~/work/Makie.jl/Makie.jl/src/makielayout/blocks/axis.jl:[569](https://github.com/MakieOrg/Makie.jl/actions/runs/4752774705/jobs/8443537615?pr=2874#step:6:570) [inlined]
   [17] update_state_before_display!(ax::Axis)
      @ Makie ~/work/Makie.jl/Makie.jl/src/makielayout/blocks/axis.jl:1376
   [18] update_state_before_display!(f::Figure)
      @ Makie ~/work/Makie.jl/Makie.jl/src/figureplotting.jl:178

updating multiple meshes: Error During Test at /home/runner/work/Makie.jl/Makie.jl/ReferenceTests/src/database.jl:32
  Got exception outside of a @test
  type Array has no field position
  Stacktrace:
    [1] getproperty(x::Vector{GeometryBasics.Mesh{3, Float32, TriangleP{3, Float32, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}}, FaceView{TriangleP{3, Float32, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}}, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}, NgonFace{3, OffsetInteger{-1, UInt32}}, StructArrays.StructVector{PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}, NamedTuple{(:position, :normals), Tuple{Vector{Point{3, Float32}}, Vector{Vec{3, Float32}}}}, Int64}, Vector{NgonFace{3, OffsetInteger{-1, UInt32}}}}}}, f::Symbol)
      @ Base ./Base.jl:38
    [2] data_limits(plot::MakieCore.Mesh{Tuple{Vector{GeometryBasics.Mesh{3, Float32, TriangleP{3, Float32, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}}, FaceView{TriangleP{3, Float32, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}}, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}, NgonFace{3, OffsetInteger{-1, UInt32}}, StructArrays.StructVector{PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}, NamedTuple{(:position, :normals), Tuple{Vector{Point{3, Float32}}, Vector{Vec{3, Float32}}}}, Int64}, Vector{NgonFace{3, OffsetInteger{-1, UInt32}}}}}}}})
      @ Makie ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:213
    [3] (::Makie.var"#775#777"{Makie.var"#774#776", Base.RefValue{HyperRectangle{3, Float32}}})(plot::MakieCore.Mesh{Tuple{Vector{GeometryBasics.Mesh{3, Float32, TriangleP{3, Float32, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}}, FaceView{TriangleP{3, Float32, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}}, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}, NgonFace{3, OffsetInteger{-1, UInt32}}, StructArrays.StructVector{PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}, NamedTuple{(:position, :normals), Tuple{Vector{Point{3, Float32}}, Vector{Vec{3, Float32}}}}, Int64}, Vector{NgonFace{3, OffsetInteger{-1, UInt32}}}}}}}})
      @ Makie ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:198
    [4] foreach(f::Makie.var"#775#777"{Makie.var"#774#776", Base.RefValue{HyperRectangle{3, Float32}}}, itr::Vector{AbstractPlot})
      @ Base ./abstractarray.jl:2774
    [5] foreach_plot
      @ ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:109 [inlined]
    [6] foreach_plot
      @ ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:103 [inlined]
    [7] data_limits
      @ ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:196 [inlined]
    [8] data_limits
      @ ~/work/Makie.jl/Makie.jl/src/layouting/data_limits.jl:195 [inlined]
    [9] is2d
      @ ~/work/Makie.jl/Makie.jl/src/scenes.jl:590 [inlined]
   [10] plot(P::Type{MakieCore.Mesh}, args::Vector{GeometryBasics.Mesh{3, Float32, TriangleP{3, Float32, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}}, FaceView{TriangleP{3, Float32, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}}, PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}, NgonFace{3, OffsetInteger{-1, UInt32}}, StructArrays.StructVector{PointMeta{3, Float32, Point{3, Float32}, (:normals,), Tuple{Vec{3, Float32}}}, NamedTuple{(:position, :normals), Tuple{Vector{Point{3, Float32}}, Vector{Vec{3, Float32}}}}, Int64}, Vector{NgonFace{3, OffsetInteger{-1, UInt32}}}}}}; axis::NamedTuple{(), Tuple{}}, figure::NamedTuple{(), Tuple{}}, kw_attributes::Base.Pairs{Symbol, Vector{RGB{Float32}}, Tuple{Symbol}, NamedTuple{(:color,), Tuple{Vector{RGB{Float32}}}}})
      @ Makie ~/work/Makie.jl/Makie.jl/src/figureplotting.jl:49
   [11] #mesh#39
      @ ~/work/Makie.jl/Makie.jl/MakieCore/src/recipes.jl:34 [inlined]

src/layouting/data_limits.jl

koehlerson · 2023-04-21T07:54:54Z

Remaining CI fails seem unrelated, locally all tests pass for me

ffreyer · 2023-04-21T09:28:47Z

The test case that fails has plot.mesh[] of type Vector{GeometryBasics.Mesh} not having a positions field which seems like a reasonable failure. There should probably be another method for that.

koehlerson · 2023-04-21T09:31:52Z

Should I restrict to ::Mesh{<:Tuple{<:GeometryBasics.Mesh} again and in case of ::Mesh{<:Tuple{Vector{<:GeometryBasics.Mesh}} merge the bboxes?

ffreyer · 2023-04-21T09:36:08Z

Sounds reasonable to me

SimonDanisch · 2023-04-21T14:02:07Z

I still think we should just fix the iteration performance in Buffer, instead of special casing in Makie.

koehlerson · 2023-04-21T15:06:03Z

would it be possible to dispatch data_limits on the args instead of the plottype ?

termi-official · 2023-04-21T15:10:20Z

After peeking into this further, I do not think it is an iteration over the buffer.

function f(buf::T) where T
           sum = zero(eltype(T))
           for val ∈ buf
                 sum += val^2
              end
           return sum
  end;

buf = GLMakie.Buffer(ones(100));

@btime f($buf); #  92.409 ns (0 allocations: 0 bytes)

I think most of the allocations come from

Makie.jl/src/layouting/data_limits.jl

Line 59 in 84a1cdf

point_iterator(mesh::GeometryBasics.Mesh) = decompose(Point, mesh)

Benchmark:

brain = load(assetpath("brain.stl"))
brain_1 = GeometryBasics.Mesh(metafree(coordinates(brain)), faces(brain))
@btime mesh(
           $brain_1
       ) # 9.735 ms (36781 allocations: 2.28 MiB)
@btime decompose(Point, $brain_1) # 3.120 ns (0 allocations: 0 bytes)
brain_2 = GeometryBasics.Mesh(Buffer(metafree(coordinates(brain))), Buffer(faces(brain)))
@btime mesh(
           $brain_2
       ) # 10.634 ms (36789 allocations: 6.30 MiB)
@btime decompose(Point, $brain_2) # 131.912 μs (2 allocations: 1.34 MiB) | is called 3 times => 4.02MB

koehlerson · 2023-04-21T15:19:13Z

After peeking into this further, I do not think it is an iteration over the buffer.

I guess with iterator Simon means the point_iterator implementation

koehlerson · 2023-04-21T15:31:16Z

would it be possible to dispatch data_limits on the args instead of the plottype ?

Because it would be very nice to squeeze out every bit of possible performance for this. At least I'd appreciate it very much :D

SimonDanisch · 2023-07-18T13:06:04Z

Is this actually still needed after the data_limit changes I introduced?

koehlerson · 2023-07-18T13:11:43Z

Which changes exactly? Just that I know what version to try

SimonDanisch · 2023-07-19T15:29:43Z

Does #3076 solve your issue?

SimonDanisch · 2023-09-07T09:02:45Z

I'm closing this for now, since we need a better undestanding of what this is supposed to solve!

koehlerson added 5 commits April 20, 2023 09:49

data_limits dispatch for Mesh to omit slow iteration over buffer

2166335

Merge branch 'master' into mk/datalimits_mesh

2fd3aa5

restrict dispatch to GeometryBasics.Mesh arg

406f507

still no allocs, but slower as it could be, should be general bbox

9cbd0ec

faster and non-allocating. however, if bbox_mesh in data_limits moved…

d4de926

… then the for loop never stops? sus

asinghvi17 reviewed Apr 20, 2023

View reviewed changes

src/layouting/data_limits.jl Show resolved Hide resolved

fix dimensional issue and ignore NaNs and Infs

16b0413

koehlerson commented Apr 21, 2023

View reviewed changes

src/layouting/data_limits.jl Outdated Show resolved Hide resolved

dimension agnostic

882b7f3

koehlerson added 3 commits April 21, 2023 11:41

restrict dispatch to meshes

9c91c7c

data_limits for Vector{GeometryBasics.Mesh}

60a0dc7

Merge branch 'master' into mk/datalimits_mesh

ce431d4

koehlerson mentioned this pull request Apr 21, 2023

Reduction of Allocations -> make FerriteViz go vroom Ferrite-FEM/FerriteViz.jl#66

Open

SimonDanisch closed this Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data_limits dispatch for Mesh to omit slow iteration over buffer #2874

data_limits dispatch for Mesh to omit slow iteration over buffer #2874

koehlerson commented Apr 20, 2023 •

edited

Loading

asinghvi17 commented Apr 20, 2023

koehlerson commented Apr 21, 2023

ffreyer commented Apr 21, 2023

koehlerson commented Apr 21, 2023

ffreyer commented Apr 21, 2023

SimonDanisch commented Apr 21, 2023

koehlerson commented Apr 21, 2023

termi-official commented Apr 21, 2023 •

edited

Loading

koehlerson commented Apr 21, 2023 •

edited

Loading

koehlerson commented Apr 21, 2023

SimonDanisch commented Jul 18, 2023

koehlerson commented Jul 18, 2023

SimonDanisch commented Jul 19, 2023

SimonDanisch commented Sep 7, 2023

data_limits dispatch for Mesh to omit slow iteration over buffer #2874

data_limits dispatch for Mesh to omit slow iteration over buffer #2874

Conversation

koehlerson commented Apr 20, 2023 • edited Loading

Description

Type of change

Checklist

asinghvi17 commented Apr 20, 2023

koehlerson commented Apr 21, 2023

ffreyer commented Apr 21, 2023

koehlerson commented Apr 21, 2023

ffreyer commented Apr 21, 2023

SimonDanisch commented Apr 21, 2023

koehlerson commented Apr 21, 2023

termi-official commented Apr 21, 2023 • edited Loading

koehlerson commented Apr 21, 2023 • edited Loading

koehlerson commented Apr 21, 2023

SimonDanisch commented Jul 18, 2023

koehlerson commented Jul 18, 2023

SimonDanisch commented Jul 19, 2023

SimonDanisch commented Sep 7, 2023

koehlerson commented Apr 20, 2023 •

edited

Loading

termi-official commented Apr 21, 2023 •

edited

Loading

koehlerson commented Apr 21, 2023 •

edited

Loading