impr type stability on CalcFactor

Open dehann opened this issue 3 years ago • 3 comments

Aug 31 '22 17:08 dehann

I left it like this on purpose as it doesn't seem to make a difference as types are still dynamically dispatched and this just increases the compile time.

Aug 31 '22 17:08 Affie

Codecov Report

Merging #1622 (d7228be) into master (4dec7e2) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1622   +/-   ##
=======================================
  Coverage   77.18%   77.18%           
=======================================
  Files          73       73           
  Lines        5589     5589           
=======================================
  Hits         4314     4314           
  Misses       1275     1275

Impacted Files	Coverage Δ
src/entities/FactorOperationalMemory.jl	`100.00% <ø> (ø)`

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Aug 31 '22 17:08 codecov[bot]

You can go ahead and merge if you are seeing a performance improvement.

Sep 01 '22 08:09 Affie

given JL 1.8 and SnoopPrecompile, compile times are no longer a consideration. If you noticed longer compile times then there should be performance benefits given stronger types. All we should do is make sure the main body of dispatches are captured during precompile. going to merge this thanks.

Sep 04 '22 23:09 dehann

Perhaps just post your benchmark code here for this improvement, or perhaps it's better if we make a separate benchmark issue. I wan't to start collecting benchmark tests to include in CI at a later stage.

Sep 05 '22 08:09 Affie