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

further latency reductions #447

Merged
merged 11 commits into from Feb 23, 2021
Merged

further latency reductions #447

merged 11 commits into from Feb 23, 2021

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Feb 10, 2021

Additionally, this tests whether renaming master to main worked as expected.

@ranocha ranocha requested a review from sloede February 10, 2021 12:12
@ranocha ranocha changed the title further latency reductions latency further latency reductions Feb 10, 2021
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to document the use of @nospecialize somewhere in the docs: Why it's there, what it does, and when/how to use it yourself. Maybe in the performance section?

Also, did you time how much this saves?

@ranocha
Copy link
Member Author

ranocha commented Feb 11, 2021

Good suggestion! I added some explanation to the docs.

Here are some timings I get from running

julia16 --threads=1 --check-bounds=no -e '@time using Trixi; @time trixi_include("examples/2d/elixir_advection_amr.jl"); @time trixi_include("examples/2d/elixir_euler_ec.jl"); @time trixi_include("examples/2d/elixir_mhd_blast_wave.jl", tspan=(0.0, 1.0e-3))'

three times.

main This PR
using Trixi 3.4 ± 0.3 s 3.4 ± 0.3 s
Advection 29.2 ± 0.3 s 28.6 ± 0.1 s
Euler 6.6 ± 0.1 s 6.4 ± 0.2 s
MHD 13.4 ± 0.1 s 13.1 ± 0.1 s

@ranocha ranocha requested a review from sloede February 11, 2021 09:20
@sloede
Copy link
Member

sloede commented Feb 11, 2021

Not trying to be unpopular, but I am wondering if this optimization is really worth it. As far as I can tell, it's at best a ~3% improvement in startup time, while the code noise grows considerably with these @nospecialize scattered around. My initial feeling is that effort-vs-impact is not really that good... What do you think, @trixi-framework/principal-developers?

@ranocha
Copy link
Member Author

ranocha commented Feb 12, 2021

I don't insist on merging this as it is. Just another data point: Precompiling Trixi on main takes 23.8 s while this PR reduces that to 20.5 s on my system (measured using julia16 --threads=1 --check-bounds=no -e '@time using Trixi'). Since some of you complained about the time it takes to precompile Trixi after modifying something, you might want to consider this, too.

Alternatively, we can also just delete the @nospecialize statements. Nevertheless, some of the additional/changed precompile statement should still be considered (since the output format changed after they were added).

@sloede
Copy link
Member

sloede commented Feb 12, 2021

Hm. Yeah, I also noticed the slowdown in precompilation. The above improvement sounds better, but I am still not sure if it's worth it. I'll try to get a hold of the others today.

@ranocha
Copy link
Member Author

ranocha commented Feb 22, 2021

I will move @nospecialize to the function body comments like

@nospecialize stuff # reduce precompilation time

@ranocha
Copy link
Member Author

ranocha commented Feb 22, 2021

Could you please review this again, @sloede?

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