JuliaObjects / ConstructionBase.jl

Primitives for construction of objects
MIT License
38 stars 22 forks source link

Use strict equality for comparing properties #82

Closed pepijndevos closed 7 months ago

pepijndevos commented 8 months ago

We found that for really large tuples the Julia compiler would give up on constfolding the comparison leading to a runtime check. Tuples of symbols are === so this change makes it easier for the compiler to inline.

jw3126 commented 8 months ago

Awesome thanks! Can you provide a test case? If this is too hard, please add a comment, why the === is important and link this PR.

jw3126 commented 8 months ago

Also there might be other places, where we should use === not sure. Not suggesting you have to vet the codebase for this PR, but would be awesome.

pepijndevos commented 8 months ago

Yeah it's kind of hard to write a test for I guess since it only changes the compiler optimization and not the functionality.

codecov-commenter commented 8 months ago

Codecov Report

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

Project coverage is 25.49%. Comparing base (8e3e773) to head (b25fef5).

Files Patch % Lines
src/ConstructionBase.jl 0.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #82 +/- ## ======================================= Coverage 25.49% 25.49% ======================================= Files 5 5 Lines 153 153 ======================================= Hits 39 39 Misses 114 114 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pepijndevos commented 8 months ago

From a quick search for == and != it could indeed be that the case in tuple_or_ntuple could also be === but I'm not that familiar with this package so I'll leave that for now. For this PR I have at least strong evidence that it works and improves performance in our code.

jw3126 commented 8 months ago

Thanks! If you bump the patch version I can release after merge.

pepijndevos commented 8 months ago

Sweet