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

Remove unnecessary copies of submitted functions to the threadpool. #90

Closed
wants to merge 1 commit into from

Conversation

radekvit
Copy link

@radekvit radekvit commented Dec 16, 2022

Pull request policy (please read)

Contributions are always welcome. However, I release my projects in cumulative updates after editing and testing them locally on my system, so my policy is not to accept any pull requests. If you open a pull request, and I decide to incorporate your suggestion into the project, I will first modify your code to comply with the project's coding conventions (formatting, syntax, naming, comments, programming practices, etc.), and perform some tests to ensure that the change doesn't break anything. I will then merge it into the next release of the project, possibly together with some other changes. The new release will also include a note in CHANGELOG.md with a link to your pull request, and modifications to the documentation in README.md as needed.

Describe the changes

Currently, the threadpool performs two unnecessary copies: one inside submit(), and another inside push_task.

This PR addresses these unnecessary copies by adding std::move to the two std::functions created in these methods when they're used.

I added a test for submit() that covers these unnecessary copies. Please note that the submitted closures must remain copyable because they're put into std::function.

Testing

Have you tested the new code using the provided automated test program and/or performed any other tests to ensure that it works correctly? If so, please provide information about the test system(s):

I ran both versions of tests, and wrote a new test that ensures the copies are actually avoided.

  • CPU model, architecture, # of cores and threads:
  • Operating system: Windows 10 Enterprise, version 10.0.19044
  • Name and version of C++ compiler: Visual Studio 2019, version 16.11.19
  • Full command used for compiling, including all compiler flags: cl BS_thread_pool_test.cpp /std:c++17 /permissive- /O2 /W4 /EHsc /Fe:BS_thread_pool_test.exe, cl BS_thread_pool_light_test.cpp /std:c++17 /permissive- /O2 /W4 /EHsc /Fe:BS_thread_pool_light_test.exe

Additional information

Example code that will benefit from this change:


int main() {
    BS::thread_pool pool(12);
    pool.submit(
        [big_data {std::vector<int>(100'000)}] {
	    // do stuff
	    return big_data.size();
    }).get();
    return 0;
}

Currently, the threadpool performs two unnecessary copies: one inside
submit(), and another inside push_task.

This PR addresses these unnecessary copies by adding std::move to the
two std::functions created in these methods when they're used.

I added a test for submit() that covers these unnecessary copies. Please
note that the submitted closures must remain copyable because they're
put into std::function.

Example code that will benefit from this change:
```

int main() {
    BS::thread_pool pool(12);
    pool.submit(
        [big_data {std::vector<int>(100'000)}] {
	    // do stuff
	    return big_data.size();
    }).get();
    return 0;
}
```
@radekvit
Copy link
Author

I saw the comment wrt. pull requests in README.md, and submitting this seemed like the best way to present these suggested changes anyways (I understood that you will not merge pull requests, but will look at them and may use them).

Please let me know if I should submit this in a different way.

@bshoshany
Copy link
Owner

Hi @radekvit, thanks for opening this PR. Sorry it took me so long to reply. I'm currently on hiatus from developing this package, since I'm too busy teaching. However, I plan to release a new version in the summer.

You did submit this in the correct way, thanks for your contribution! As soon as I have time I will test your code locally and incorporate it into the next release, along with other upcoming changes.

@bshoshany
Copy link
Owner

Update: This issue has been fixed in v3.5.0, which was just released. (I did it in a slightly different way than what you suggested.) Thanks again!

@bshoshany bshoshany closed this May 26, 2023
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