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

fix inline caching deoptimization #21

Merged
merged 2 commits into from
Nov 29, 2021
Merged

fix inline caching deoptimization #21

merged 2 commits into from
Nov 29, 2021

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 28, 2021

According to deoptigate, there are two inline caching issues.

Screenshot from 2021-11-28 13-42-07

This one fixes one of them.

Checklist

@kibertoad
Copy link
Member

@Uzlopak After #17 is merged, can you post benchmark results before and after this change?

@kibertoad
Copy link
Member

@Uzlopak Can you pull latest master into this branch before running benchmark?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 28, 2021

Actually this benchmark is not representative. Will open an Issue.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 29, 2021

This PR does not have an effect on the performance currently. The hot path is still searchstream.

Screenshot from 2021-11-29 13-47-53

Probably if we optimize searchstream further more it could have an effect. :/

@kibertoad
Copy link
Member

@Uzlopak Actually, I'm not sure we need all of this change, the way I read the report, it is unhappy due to ev being initialized as undefined and then changed into a string, and then being used as a key. Can you replace it just with a new const variable and see if that also addresses the cache deoptimization?

@kibertoad
Copy link
Member

However, your approach still should be more efficient, as it replaces monomorphic variable with just a constant, which should be more predictable. Let's go with your implementation.

@kibertoad
Copy link
Member

Can we drop ev from line 142 now? I think it should not be used anymore

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 29, 2021

I removed the ev instantiation :)

@Uzlopak Uzlopak merged commit e17c412 into fastify:master Nov 29, 2021
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