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

force specialization in sv_ecef_to_ecef #2

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

disberd
Copy link
Contributor

@disberd disberd commented Aug 15, 2023

I found out that I had unexpected allocations when using the SatelliteToolbox ecosystem and I pinpointed this down to the sv_ecef_to_ecef function.

This small PR tries to force specialization for this function as the generic args... can often lead to allocations due to lack of specialization (see this section of the performance tips)

A benchmark of the function before and after this PR on my linux machine gives the following improvement:

# Before this PR
BenchmarkTools.Trial: 10000 samples with 162 evaluations.
 Range (min  max):  656.858 ns  39.471 μs  ┊ GC (min  max):  0.00%  96.59%
 Time  (median):     687.685 ns              ┊ GC (median):     0.00%
 Time  (mean ± σ):   870.394 ns ±  2.221 μs  ┊ GC (mean ± σ):  15.98% ±  6.12%

  ▅██▇▅▄▄▃▃▃▃▃▃▂▂▁▂▁▁▁                                         ▂
  ███████████████████████▇▇█▇▆▇▅▆▆▆▅▃▅▄▄▄▅▅▅▆▆▆▆▄▆▆▆▇▆▇▅▇▇█▆▆▆ █
  657 ns        Histogram: log(frequency) by time      1.31 μs <

 Memory estimate: 1.11 KiB, allocs estimate: 15.

# After this PR
BenchmarkTools.Trial: 10000 samples with 900 evaluations.
 Range (min  max):  123.292 ns  200.348 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     126.317 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   127.951 ns ±   6.883 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

    ▄▆█▆                                                         
  ▆█████▅▃▃▃▃▃▃▃▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂▂▂▂▂▂▂▂▂▂▂▂ ▃
  123 ns           Histogram: frequency by time          166 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.
Test code (taken from package tests)
using SatelliteToolboxTransformations
using BenchmarkTools

eop_iau1980  = read_iers_eop("test/eop_IAU1980.txt",  Val(:IAU1980))

JD_UTC = date_to_jd(2004, 4, 6, 7, 51, 28.386009)

# ITRF => PEF
# ======================================================================================

r_itrf  = [-1033.4793830; 7901.2952754; 6380.3565958]
v_itrf  = [-3.225636520; -2.872451450; +5.531924446]
sv_itrf = OrbitStateVector(JD_UTC, r_itrf, v_itrf)
@benchmark sv_ecef_to_ecef($sv_itrf, ITRF(), PEF(), $JD_UTC, $eop_iau1980)

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4f24b33) 100.00% compared to head (4129ee7) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #2   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           30        30           
  Lines         1160      1160           
=========================================
  Hits          1160      1160           
Files Changed Coverage Δ
src/orbit/sv_ecef_to_ecef.jl 100.00% <100.00%> (ø)

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

@ronisbr
Copy link
Member

ronisbr commented Aug 15, 2023

Awesome! Thank you very much @disberd !

@ronisbr ronisbr merged commit f05c93b into JuliaSpace:main Aug 15, 2023
12 checks passed
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.

None yet

2 participants