Skip to content
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

ExclusiveTopology with ArrayOfVectorViews backend #974

Merged
merged 62 commits into from
Jul 9, 2024

Conversation

KnutAM
Copy link
Member

@KnutAM KnutAM commented Jun 5, 2024

See discussion in #965, this PR introduces a cleaned-up version.

Most important changes in this PR compared to master are:

  • Storage from Array{EntityNeighborhood{T}, N} -> ArrayOfVectorViews{T, N} <: AbstractArray{"View{Vector{T}}", N}
  • getcells dispatches removed (since these would now act on Vector{<:BoundaryIndex}). In any case, getcells normally returns ::AbstractCells, but in topology they return ::Ints on mater, if any, cellid should be overloaded).
  • EntityNeighborhood is gone

Compared to #965, the support for hashed indexing is removed and only ExclusiveTopology is implemented. Before finalizing, I would like some feedback from the topology side, and then I'll add

Let me know if more details are required for the initial review.

Note that in relation to #942, this PR explicitly errors if trying to construct ExclusiveTopology with grids containing embedded cells, thus avoiding silent bugs.

Benchmarks

using Ferrite, BenchmarkTools
function sum_indices(nbh::AbstractMatrix)
    v = 0
    for nbs in nbh
        for n in nbs
            v += n[1] + n[2]
        end
    end
    return v
end

function sum_indices(top::ExclusiveTopology)
    v  = sum_indices(top.vertex_vertex_neighbor)
    v += sum_indices(top.edge_edge_neighbor)
    v += sum_indices(top.face_face_neighbor)
    return v
end
function sum_skeleton_facet_neighbors(top, grid)
    skeleton = Ferrite.facetskeleton(top, grid)
    v = 0
    for facetidx in skeleton
        v += sum(n[1] + n[2] for n in getneighborhood(top, grid, facetidx); init = 0)
    end
    return v
end

grid = generate_grid(Hexahedron, (10, 10, 10));
top = ExclusiveTopology(grid); # Compilation

@btime sum_indices($top); 
# Master: 55.700 μs (0 allocations: 0 bytes)
#     PR: 36.000 μs (0 allocations: 0 bytes)
Ferrite.facetskeleton(top, grid); # Compilation

grid = generate_grid(Hexahedron, (100, 100, 100));
@time top = ExclusiveTopology(grid);
# Master: 27.150434 seconds (64.58 M allocations: 7.195 GiB, 72.43% gc time)
#     PR:  3.799651 seconds (45 allocations: 1.895 GiB, 13.84% gc time)
@time Ferrite.facetskeleton(top, grid);
# Master: 0.143506 seconds (2 allocations: 46.234 MiB)
#     PR: 0.032492 seconds (2 allocations: 46.234 MiB)
@btime sum_skeleton_facet_neighbors($top, $grid);
# Master: 59.683 ms (0 allocations: 0 bytes)
#     PR: 21.887 ms (0 allocations: 0 bytes)

(PR refers to b7e2add)

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 98.99497% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.58%. Comparing base (2c8b751) to head (2961031).
Report is 1 commits behind head on master.

Files Patch % Lines
src/Grid/topology.jl 98.57% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #974      +/-   ##
==========================================
+ Coverage   93.20%   93.58%   +0.37%     
==========================================
  Files          38       39       +1     
  Lines        5816     5888      +72     
==========================================
+ Hits         5421     5510      +89     
+ Misses        395      378      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KnutAM KnutAM requested a review from koehlerson June 6, 2024 20:03
@KnutAM KnutAM added this to the v1.0.0 milestone Jun 8, 2024
@KnutAM
Copy link
Member Author

KnutAM commented Jun 8, 2024

c276cb2 brings construction in benchmark further down from
5.527448 seconds (58 allocations: 2.107 GiB, 9.91% gc time)
to
4.217167 seconds (45 allocations: 1.895 GiB, 12.49% gc time)

@KnutAM
Copy link
Member Author

KnutAM commented Jun 21, 2024

When rechecking the timings, I saw some worse performance now.
So took @termi-official's suggestion and optimized the sizehints, benchmark results are updated with this optimization.

Script
function analyze_top_sizes(grid; show_avg=false)
    top = ExclusiveTopology(grid);
    for key in fieldnames(typeof(top))
        nbh = getfield(top, key)
        nbh isa Ferrite.ArrayOfVectorViews || continue
        max_size = maximum(length, nbh; init=0)
        avg_size = sum(length, nbh; init=0)/length(nbh)
        if show_avg
            println(key)
            println("  max: ", max_size, " entities")
            println("  avg: ", avg_size, " entities")
        else
            println(key, ": ", max_size)
        end
    end
end

function print_sizes(n=10, celltypes=[Line, Triangle, Quadrilateral, Tetrahedron, Hexahedron])
   for CT in celltypes
       grid = generate_grid(CT, ntuple(_->n, Ferrite.getrefdim(CT)))
       println(CT)
       analyze_top_sizes(grid)
       println()
   end
end

Max number of entities for different generated grids

field Line Triangle Quadrilateral Tetrahedron Hexahedron
vertex_to_cell 2 6 4 24 8
cell_neighbor 2 12 8 70 26
face_face_neighbor 0 0 0 1 1
edge_edge_neighbor 0 1 1 3 1
vertex_vertex_neighbor 1 3 1 13 1

@KnutAM KnutAM added the awaiting review PR is finished from the authors POV, waiting for feedback label Jun 22, 2024
@KnutAM
Copy link
Member Author

KnutAM commented Jun 26, 2024

I will fix #988 after merging this PR to avoid annoying conflicts.

@termi-official
Copy link
Member

Resolves #617 ?

@KnutAM KnutAM linked an issue Jul 1, 2024 that may be closed by this pull request
3 tasks
Copy link
Member

@termi-official termi-official left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvement on the performance and I think the data structure will also help in the long run. I left a few comments.

src/Grid/topology.jl Outdated Show resolved Hide resolved
src/Grid/topology.jl Show resolved Hide resolved
src/Grid/topology.jl Show resolved Hide resolved
src/Grid/topology.jl Show resolved Hide resolved
test/test_grid_dofhandler_vtk.jl Outdated Show resolved Hide resolved
src/Grid/topology.jl Outdated Show resolved Hide resolved
KnutAM and others added 3 commits July 1, 2024 13:48
@KnutAM
Copy link
Member Author

KnutAM commented Jul 1, 2024

I left a few comments.

Thanks!

src/Grid/topology.jl Show resolved Hide resolved
src/Grid/topology.jl Outdated Show resolved Hide resolved
Copy link
Member

@termi-official termi-official left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can you dump the benchmark code in the benchmark folder (here or separate PR)?

@KnutAM KnutAM merged commit 8163473 into master Jul 9, 2024
9 of 11 checks passed
@KnutAM KnutAM deleted the kam/exclusive_topology branch July 9, 2024 17:35
@KnutAM
Copy link
Member Author

KnutAM commented Jul 9, 2024

LGTM. Can you dump the benchmark code in the benchmark folder (here or separate PR)?

Thanks! I haven't looked so much at the benchmark setup, will just open an issue for now to not forget to do it later unless someone solves it before I have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review PR is finished from the authors POV, waiting for feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Topology construction is slow
4 participants