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

chore: remove all dyn for opcode that reduce dyn trait cost #660

Closed
wants to merge 2 commits into from

Conversation

yjhmelody
Copy link
Contributor

No description provided.

@rakita
Copy link
Member

rakita commented Aug 29, 2023

yjhmelody

wdym by dyn trait cost? In the end this shoudn't this be dynamically dispatched as it can be called in recursion

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Aug 29, 2023

Could not see it will be a problem for recursion.
And you could still use dyn outside. I just remove the inner dyn.

@rakita
Copy link
Member

rakita commented Aug 29, 2023

Resolved here: #659 wrong issue

@rakita rakita closed this Aug 29, 2023
@rakita rakita reopened this Aug 29, 2023
@rakita
Copy link
Member

rakita commented Aug 29, 2023

Could not see it will be a problem for recursion. And you could still use dyn outside. I just remove the inner dyn.

Sure, but the present code works as well, question is why would we use this and not dyn Host?

Could you run cachegrind, instructions are here: #582
Just to check performance.

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Aug 29, 2023

You could try. I am using macOS but valgrind: Linux is required for this software.

I don't think this will cause performance degradation at least. After all, the eval function mixed the static dispatch and dynamic dispatch of Host before. I just remove the dynamic part.

@DaniPopes
Copy link
Collaborator

DaniPopes commented Aug 29, 2023

Can we try after the interpreter refactor is merged? I wanted to explore this as well after that PR (#582).

@rakita
Copy link
Member

rakita commented Aug 29, 2023

You could try. I am using macOS but valgrind: Linux is required for this software.

I don't think this will cause performance degradation at least. After all, the eval function mixed the static dispatch and dynamic dispatch of Host before. I just remove the dynamic part.

Same here :) will try it with docker as it is something i will need in future. tbh i don't expect a difference but it is best just to check.

Can we try after the interpreter refactor is merged? I wanted to explore this as well after that PR (#582).

That makes sense, will come back to this after #582 gets merged.

@yjhmelody
Copy link
Contributor Author

Fine

@rakita
Copy link
Member

rakita commented Sep 21, 2023

Tested this on macbook inside docker for snailtracer.
Main is currently at 400,122,558 Instructions
And this branch gives 447,359,474 instructions

Just for reference main before perf2 branch was showing 464,501,741 instructions.

Will close this for now.

@rakita
Copy link
Member

rakita commented Sep 21, 2023

@yjhmelody as dani wrote in #739 there is a 2% performance, so we are changing it to H: Host

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.

3 participants