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

Add inference test and improve inference of _SpectralElementGrid2D #1818

Merged
merged 2 commits into from
Jun 16, 2024

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Jun 16, 2024

This PR changes inference failures from 351 -> 189 in _SpectralElementGrid2D.

The main problem here, I think, was that the compiler was boxing CoordType2D, so this PR moves that logic to a separate function (sometimes the compiler struggles with understanding types if a variable is computed in multiple places, even in the context of union-splitting).

@charleskawczynski
Copy link
Member Author

Some benchmarks, main:

julia> @benchmark Grids._SpectralElementGrid2D(Spaces.topology(space), Spaces.quadrature_style(space); enable_bubble=false)
BenchmarkTools.Trial: 206 samples with 1 evaluation.
 Range (min  max):  22.108 ms  186.010 ms  ┊ GC (min  max): 0.00%  87.63%
 Time  (median):     22.855 ms               ┊ GC (median):    0.00%
 Time  (mean ± σ):   24.227 ms ±  11.390 ms  ┊ GC (mean ± σ):  3.85% ±  6.40%

   ▁▆▇█▁                     ▃
  ▃█████▅▄▄▆▄▄▃▃▃▃▁▃▃▁▃▁▃▃▄▄▆█▃▄▇▄▄▃▆▃▃▄▃▃▁▁▁▃▃▁▁▁▃▁▁▁▁▁▁▃▃▃▃▃ ▃
  22.1 ms         Histogram: frequency by time         26.9 ms <

 Memory estimate: 4.71 MiB, allocs estimate: 185908.

This PR

julia> @benchmark Grids._SpectralElementGrid2D(Spaces.topology(space), Spaces.quadrature_style(space); enable_bubble=false)
BenchmarkTools.Trial: 331 samples with 1 evaluation.
 Range (min  max):  13.639 ms  175.915 ms  ┊ GC (min  max): 0.00%  91.53%
 Time  (median):     14.057 ms               ┊ GC (median):    0.00%
 Time  (mean ± σ):   15.071 ms ±   8.921 ms  ┊ GC (mean ± σ):  4.22% ±  6.06%

     ▆█▄
  ▄▄████▅▄▆▅▃▃▄▂▃▄▂▁▁▁▁▁▂▂▄▅▄▃▃▃▄▂▄▃▃▃▃▃▃▂▁▂▁▂▃▁▁▁▁▁▁▂▃▁▁▁▂▁▁▂ ▃
  13.6 ms         Histogram: frequency by time         17.9 ms <

 Memory estimate: 3.83 MiB, allocs estimate: 135198.

@charleskawczynski charleskawczynski merged commit 6d354f0 into main Jun 16, 2024
17 of 19 checks passed
@charleskawczynski charleskawczynski deleted the ck/space_inference3 branch June 16, 2024 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant