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

Avoid using std::function for performance #106

Merged
merged 2 commits into from
Aug 27, 2019
Merged

Avoid using std::function for performance #106

merged 2 commits into from
Aug 27, 2019

Conversation

CleverSource
Copy link
Contributor

@CleverSource CleverSource commented Aug 25, 2019

Avoiding the usage of std::function in event dispatcher for performance. This has an 20%+ improvement in speed. You can read more about this via issue #85.

Full credit to @Chlorie for this.

This PR was never made so I've decided to make it.
This resolves #85.

@LovelySanta LovelySanta added the Impact:Optimization Optimalisation of existing code label Aug 25, 2019
@LovelySanta
Copy link
Collaborator

You still have to deduce the F parameter, so we can just call it with a single template parameter as before? I expect it not to work as of now, since dispatch has 2 template parameters, where in all the code we only provide one

@CleverSource
Copy link
Contributor Author

CleverSource commented Aug 25, 2019

You still have to deduce the F parameter, so we can just call it with a single template parameter as before? I expect it not to work as of now, since dispatch has 2 template parameters, where in all the code we only provide one

(am mainly referring to you saying this wouldn't work)
I've tested this on a local copy of Hazel, I directly cloned the repo and just changed that one thing before making this PR. The data that the 20% improvement comes from is from someone using the code I've changed.
image

@LovelySanta
Copy link
Collaborator

I've tested this on a local copy of Hazel, I directly cloned the repo and just changed that one thing before making this PR. The data that the 20% improvement comes from is from someone using the code I've changed.

I am aware from where the improvement comes from, but I am talking about this
image
The template has now 2 parameters, while in every dispatcher, we're only providing 1 template parameter where the 2nd one doesn't have some default...
That's where the whole deduction in the rest of the post comes in... Or am I interpreting something wrong?

@CleverSource
Copy link
Contributor Author

CleverSource commented Aug 25, 2019

Odd.. the person that says it's a 20% improvement did not test with the deduction piece. Which is why I don't think that matters.

I mainly made this PR to implement the performance bit of things, but if you have an idea for how to implement the deduction I'd love to hear it.

@LovelySanta
Copy link
Collaborator

Maybe @Chlorie can help us implement it, as we both hit a solid brick wall on trying to get the deduction working...

@ilikenoodles2
Copy link
Contributor

@LovelySanta The "F" parameter is deduced by the compiler, so it can be ignored. We care about parameter "T", which is the parameter that the user already provides, so the code in this pull request is basically complete(The optimization).

I'm a bit skeptical about the deduction piece though. If I understand correctly, the idea is to automatically deduce parameter "T"? Because if that's the case, it would seem nicer to have to explicitly declare "T" to confirm "I want this type of event to be handled by this function".

@Chlorie
Copy link

Chlorie commented Aug 27, 2019

I was busy doing something else recently so I didn't respond to this pull request, my fault.
The solution for template argument deduction mentioned in the original issue was kind of incomplete, now that you've brought up this question, I'm thinking of a more complete solution to that.

@Chlorie
Copy link

Chlorie commented Aug 27, 2019

After some trial and error I found that it's really hard to do the full type deduction (let the compiler to deduce T) when using generic function objects (function pointers are easy, non-generic lambdas are easy, but generic function objects like the return value of std::bind are hard). If this is not really needed, like what @ilikenoodles2 said, the solution in this PR is good enough.
I'll try to play with these templates a little more to see whether the full deduction can be achieved, but not with much hope.

@LovelySanta
Copy link
Collaborator

LovelySanta commented Aug 27, 2019

Now that I look at it, it might indeed be more readable to supply T. I'll approve as it is, no need to deduce T.

@CleverSource
Copy link
Contributor Author

Added the comment.

@LovelySanta
Copy link
Collaborator

LGTM

@LovelySanta LovelySanta merged commit 30b5c24 into TheCherno:master Aug 27, 2019
@LovelySanta LovelySanta added this to Merged in Community additions Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact:Optimization Optimalisation of existing code
Projects
Community additions
  
Merged PR's
Development

Successfully merging this pull request may close these issues.

Avoid using std::function in event dispatcher for performance
4 participants