Skip to content

Remove dynamic dispatch in Hessian evaluation #2740

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

Merged
merged 8 commits into from
Apr 30, 2025
Merged

Conversation

blegat
Copy link
Member

@blegat blegat commented Apr 28, 2025

As chunk is maximum 10, we can reuse the _create_binary_switch we use for type-stable evaluation of our list of operators to call _eval_hessian_inner in a type-stable way.
On this benchmark #2739 (comment)
Before this PR

5.959426 seconds (158.26 k allocations: 11.273 MiB)
5.973144 seconds (158.26 k allocations: 11.273 MiB)
6.078342 seconds (158.26 k allocations: 11.273 MiB)
6.031799 seconds (158.26 k allocations: 11.273 MiB, 0.23% gc time)
6.022742 seconds (158.26 k allocations: 11.273 MiB)
6.013003 seconds (158.26 k allocations: 11.273 MiB)
6.021324 seconds (158.26 k allocations: 11.273 MiB)

After this PR:

5.925775 seconds
5.820862 seconds
5.865539 seconds
5.823728 seconds
5.848671 seconds
5.864283 seconds
5.876498 seconds
5.911389 seconds
5.894025 seconds

@odow
Copy link
Member

odow commented Apr 28, 2025

I'm very very very hesitant about this. At minimum, I think we should manually write out the switch. Macros and @eval are just too complicated and don't justify the extra 20 lines of code. It's a different story when there are 70 something operators.

It would also mean we need tests for every possible chunk size to ensure 100% code coverage.

@odow
Copy link
Member

odow commented Apr 29, 2025

It's nice, there are already tests that check all of the different chunk sizes.

@blegat
Copy link
Member Author

blegat commented Apr 29, 2025

It would also mean we need tests for every possible chunk size to ensure 100% code coverage.

I don't agree. We're using a binary switch that is already heavily tested, what's the risk ?
If you hardcode 10 different if-else then you might make a typo but not with the binary switch.
Also, the advantage of the switch is that you can modify that constant without the need to modify the code

@odow
Copy link
Member

odow commented Apr 29, 2025

I've changed to use the binary switch. I wouldn't say that it is "heavily" tested. It's used in one other place.

If you hardcode 10 different if-else then you might make a typo but not with the binary switch.

Not if we have every line tested 😄

@odow odow merged commit 1409152 into master Apr 30, 2025
30 of 31 checks passed
@odow odow deleted the bl/hessian_bin_switch branch April 30, 2025 00:09
@blegat
Copy link
Member Author

blegat commented Apr 30, 2025

I wouldn't say that it is "heavily" tested. It's used in one other place.

Maybe that was an over-exageration 😄 But if the binary switch had a bug I prefer MOI to be completely broken rather than having a hidden bug. In that sense, giving it even more coverage is good :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants