reduce allocations

[karajan9]

I believe materializing the array isn't necessary.

[juliohm]

Can you please double check that it works locally? The tests are failing and the errors seem related to the change. It would be nice to remove the allocation indeed.

[karajan9]

Sure, I'll take a look.

[karajan9]

So I ran the tests again (on 1.7.0-rc1); it includes two errors, but both of them I also had before the change.
On 1.6.2 the code errors in the way you've shown -- I didn't remembered to test on this version that's why I missed it. I'm not quite sure what changed in 1.7...

Using findmax instead seems to circumvent that problem somehow. The tests passed for me on both Julia versions (except the two errors on 1.7). Just like the generator-argmax on 1.7 it removes the allocation.

Since there were some allocations left I also changed the centroid function; again, no new errors on both versions.

[codecov]

Codecov Report

Merging #176 (7cc4486) into master (33bea66) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
- Coverage   86.93%   86.76%   -0.17%     
==========================================
  Files         106      106              
  Lines        2303     2305       +2     
==========================================
- Hits         2002     2000       -2     
- Misses        301      305       +4     

Impacted Files	Coverage Δ
src/polytopes.jl	81.17% <100.00%> (ø)
src/supportfun.jl	100.00% <100.00%> (ø)
src/discretization/fist.jl	75.36% <0.00%> (-5.80%)	⬇️
src/sampling.jl	100.00% <0.00%> (ø)
src/sampling/regular.jl	100.00% <0.00%> (ø)
src/sampling/weighted.jl	91.66% <0.00%> (ø)
src/sampling/homogeneous.jl	100.00% <0.00%> (ø)
src/sampling/mindistance.jl	100.00% <0.00%> (ø)

---

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 33bea66...7cc4486. Read the comment docs.

[juliohm]

That is awesome. I made a small change and am just waiting for tests to finish running. Would you mind adding tests with @allocated centroid(..) so that we don't reintroduce the allocations in the future?

[juliohm]

I can merge as is as well, let me know. I am in the process of releasing a new version of the package and would like to include this PR :)

[karajan9]

Sure, I'm currently trying to figure out where to place the test (probably in polytopes.jl).

However, the variables for the current centroid (a Chain and a PolyArea which holds a Chain) do seem to allocate due to vertices(c::Chain), I believe. Therefore I'd like to test the allocations against a Triangle or so.

[juliohm]

I think polytopes.jl is a good place for the centroid test. The supportfun is tested in a separate file if I remember correctly.

Feel free to remove allocations in other places in the future, these improvements are always welcome 💯

[karajan9]

Well, I'll try my best 👍

I have experimented quite a bit now but @allocated doesn't show zero allocations although @benchmark does. I'm not quite sure why that is; do globals come into play here?

I assume it's not very helpful to add a test with some random number of allocations (16 in this case, or 32 from the REPL).

Edit: making my triangle const does remove the allocations, but I'm not sure how to do that in the test. There is an allocation test at the end of polytopes.jl that seems to work fine.

[juliohm]

Maybe you are running into the precompilation issue? Have you tried running @allocated twice? The first time it counts the time and allocations of the compilation process as far as I know. So you need to call the method once to warm it up and then again to show the desired allocation.

[karajan9]

I did that yes, I added a test for the triangle centroid while at it. During the first time the allocations are in the hundreds of thousands. After that it's constant at 16 bytes.

    t = Triangle(P2(0,0), P2(1,0), P2(0,1))
    @test centroid(t) ≈ P2(1/3, 1/3)
    @show typeof(t)
    @show @allocated centroid(t)
    @show @allocated centroid(t)
    vertices(t)
    @show @allocated vertices(t)
    length(vertices(t))
    @show @allocated length(vertices(t))

gives

typeof(t) = Triangle{2, Float32, SVector{3, Point2f}}
#= /home/karajan/.julia/dev/Meshes/test/polytopes.jl:51 =# @allocated(centroid(t)) = 16
#= /home/karajan/.julia/dev/Meshes/test/polytopes.jl:52 =# @allocated(centroid(t)) = 16
#= /home/karajan/.julia/dev/Meshes/test/polytopes.jl:54 =# @allocated(vertices(t)) = 32
#= /home/karajan/.julia/dev/Meshes/test/polytopes.jl:56 =# @allocated(length(vertices(t))) = 32

I looks like a problem with globals to me (especially since using const fixed it in the REPL). Not sure what to do about it though.

A quick search gave invenia/NamedDims.jl#115 (comment)

[juliohm]

We can always push without allocation tests if you feel they are not helpful.

[karajan9]

I think it would be good to add that test, however, I don't know yet how to make it work so for now I would go forward without it and add it later when we found a good solution.

[juliohm]

Thanks for the improvement 💯 Feel free to submit more of those :)