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

Replace std::function to template argument in Alg::call to avoid its … #4

Merged
merged 1 commit into from Sep 1, 2015

Conversation

pmed
Copy link
Contributor

@pmed pmed commented Aug 27, 2015

…type erasure

In theory, this should improve chances for the alg inlining

…type erasure

In theory, this should improve chances for the alg inlining
@kberezovsky
Copy link
Owner

I failed to see the type erasure here. For example, the algorithm type at the line 52:

std::function<void(std::insert_iterator<DstCont>)> alg

Here we have not some void* - like type. The type is made with the information about DstCont, so the type is concrete.

@pmed
Copy link
Contributor Author

pmed commented Aug 30, 2015

I mean std::function uses type erasure to store various function objects inside.

In the current version lambdas supplied to the Alg::call() would be copied into a temporary std::function object for the alg argument, here for example. This may lead to a dynamic memory allocation for the lambada captured context and virtual function call on the std::function invocation. Here's some additional information: http://blog.demofox.org/2015/02/25/avoiding-the-performance-hazzards-of-stdfunction/

I suggest to use template argument alg for the Alg::call() function to pass the actual lambda object without conversion to std::function.

Actually my simple performance test doesn't show a significant improvement for this change, but at least it removes unnecessary pessimization.

@kberezovsky
Copy link
Owner

Got it. Cool. Thanks.

kberezovsky added a commit that referenced this pull request Sep 1, 2015
Replace std::function to template argument in Alg::call to avoid its …
@kberezovsky kberezovsky merged commit 79db529 into kberezovsky:master Sep 1, 2015
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