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

Specialize adapt_structure for small tuples #81

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

charleskawczynski
Copy link
Contributor

Closes #79.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.85%. Comparing base (9c1b146) to head (5e85c7c).

Files Patch % Lines
src/base.jl 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
- Coverage   93.65%   92.85%   -0.80%     
==========================================
  Files           6        6              
  Lines          63       70       +7     
==========================================
+ Hits           59       65       +6     
- Misses          4        5       +1     

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

@maleadt
Copy link
Member

maleadt commented Mar 14, 2024

Can't you do this with a simple if, assuming it would get compiled away, instead of inserting many methods?

This also reminds me of JuliaLang/julia#53665.

@charleskawczynski
Copy link
Contributor Author

I was going to suggest that we could use an if-statement, is that your preference?

@maleadt
Copy link
Member

maleadt commented Mar 15, 2024

Yes, you're now putting additional pressure on the dispatch system where the compiler should be able to handle this just fine.

@charleskawczynski
Copy link
Contributor Author

Alright, I've updated the PR. This passes inference for me in the #79 example. Does 20 qualify as "small", @maleadt?

@charleskawczynski
Copy link
Contributor Author

bump 🙂

@maleadt
Copy link
Member

maleadt commented Mar 20, 2024

I don't like that we don't have a test for this, but OK I guess.

Seeing how you went with the if approach, I take it using afold (which handles this in Base, https://github.com/JuliaLang/julia/blob/bc7ba3d5c8b2dab1c0e19537739b67c2da902d11/base/operators.jl#L579-L625) didn't solve the allocations?

@maleadt maleadt merged commit 925df4a into JuliaGPU:master Mar 20, 2024
18 checks passed
@charleskawczynski
Copy link
Contributor Author

I don't like that we don't have a test for this, but OK I guess.

Seeing how you went with the if approach, I take it using afold (which handles this in Base, https://github.com/JuliaLang/julia/blob/bc7ba3d5c8b2dab1c0e19537739b67c2da902d11/base/operators.jl#L579-L625) didn't solve the allocations?

Thank you for merging!

Yeah, I agree about the tests. I did try to write a simple reproducer, but I wasn't able to recreate the failure. I'm not sure what ingredients are needed. I'll take another look to see if I can cook something up.

I tend to try to avoid using Julia internals, but I can check to see if afold works if you prefer that over the existing solution.

@maleadt
Copy link
Member

maleadt commented Mar 20, 2024

I tend to try to avoid using Julia internals

Yeah, that's fair.

@charleskawczynski
Copy link
Contributor Author

I've updated, and I'll try to continue updating/simplifying, the reproducer in #79. It's drastically simpler now, although there's still a lot more to go.

@charleskawczynski
Copy link
Contributor Author

Thanks again for fixing this! This fixed 955 inference failures per timestep 😬 found by JET! 🚀

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.

Inference failure in adapt
2 participants