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

refactor: prevent extra move for get-methods with nrvo #73

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

jirol9xa
Copy link
Contributor

@jirol9xa jirol9xa changed the title [Minor][thread-safe-queue]: Prevent extra move for get-methods by nrvo [Minor][thread-safe-queue]: Prevent extra move for get-methods with nrvo Nov 23, 2024
@DeveloperPaul123 DeveloperPaul123 self-requested a review November 25, 2024 13:35
Copy link
Owner

@DeveloperPaul123 DeveloperPaul123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. If you could please address the build issues, that would be great.

@jirol9xa
Copy link
Contributor Author

sorry, i am not able to compile it locally

@DeveloperPaul123
Copy link
Owner

Ok looks like some tests are failing. I'll have to investigate a bit more if you can't build it locally.

@jirol9xa
Copy link
Contributor Author

I tried it on my home ubuntu 22.04 with gcc-11. Everything is fine, it is not possible to reproduce the crash.

@DeveloperPaul123
Copy link
Owner

@jirol9xa Unfortunately, this is the nature of something as low level as a thread pool. Even if it's no locally reproducible, it is still likely a bug somewhere.

Were you able to run it with thread sanitizer enabled and/or asan?

@DeveloperPaul123
Copy link
Owner

I'll try this out later on on PopOS and Ubuntu 24.04 to see if it's reproducible.

@jirol9xa
Copy link
Contributor Author

Tried to run with tsan, and had an error even if running from trunk =)) Pretty sure I am doing something wrong

@DeveloperPaul123
Copy link
Owner

Oh no, there might be a bug then. What error do you see?

@jirol9xa
Copy link
Contributor Author

CMake Error at /home/voffk4/fast-thread-pool/build/_deps/doctest-src/scripts/cmake/doctestAddTests.cmake:45 (message):
Error running test executable
'thread-pool/build/test/thread-pool-tests':

Result: 66
Output: 

@DeveloperPaul123
Copy link
Owner

CMake Error at /home/voffk4/fast-thread-pool/build/_deps/doctest-src/scripts/cmake/doctestAddTests.cmake:45 (message): Error running test executable 'thread-pool/build/test/thread-pool-tests':

Result: 66
Output: 

I was able to reproduce this. https://stackoverflow.com/questions/77850769/fatal-threadsanitizer-unexpected-memory-mapping-when-running-on-linux-kernels this stackoverflow helped me find a workaround. Setting sudo sysctl vm.mmap_rnd_bits=28 fixed the build issue for me, but I think TSan is giving false positives. I'll have to dig in a bit more. When I run the tests without TSan they all pass.

@DeveloperPaul123
Copy link
Owner

Welp, I re-ran the test and it passed this time 🤷‍♂️

@DeveloperPaul123 DeveloperPaul123 self-requested a review November 26, 2024 03:47
@DeveloperPaul123 DeveloperPaul123 changed the title [Minor][thread-safe-queue]: Prevent extra move for get-methods with nrvo refactor: prevent extra move for get-methods with nrvo Nov 26, 2024
@DeveloperPaul123 DeveloperPaul123 merged commit bc10764 into DeveloperPaul123:master Nov 26, 2024
13 checks passed
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.

2 participants