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

impr type stability on CalcFactor #1622

Merged
merged 1 commit into from
Sep 4, 2022
Merged

impr type stability on CalcFactor #1622

merged 1 commit into from
Sep 4, 2022

Conversation

dehann
Copy link
Member

@dehann dehann commented Aug 31, 2022

No description provided.

@Affie
Copy link
Member

Affie commented Aug 31, 2022

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.

@codecov
Copy link

codecov bot commented Aug 31, 2022

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% <ø> (ø)

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

@Affie
Copy link
Member

Affie commented Sep 1, 2022

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

@dehann
Copy link
Member Author

dehann commented Sep 4, 2022

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.

@dehann dehann merged commit e976248 into master Sep 4, 2022
@Affie
Copy link
Member

Affie commented Sep 5, 2022

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.

@dehann dehann deleted the 22Q3/enh/cftypestab branch November 27, 2024 14:07
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.

2 participants