Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Using C++11 threads instead of pthread #1212

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gian21391
Copy link

I have ported vvp (and related shared libraries) to MSVC. To do that, I decided to remove the dependency from pthread and use the threading support offered by C++11.

I think it makes sense to have this also in the main repository.

@gian21391 gian21391 marked this pull request as draft February 19, 2025 13:27
Copy link
Collaborator

@larsclausen larsclausen left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@caryr
Copy link
Collaborator

caryr commented Feb 22, 2025

I would need to check, but I think the files we take directly from GTKWave (specifically fstapi.c) currently use pthreads so to get this completely clean we would have to get Tony to change this to C++ on his end as well. If we provide a patch that may be enough. I'm not against using C++ threads for the C++ code, but we need to remember there is also C code that can only use pthreads so do we really save anything by switching the C++ code?

gian21391 and others added 2 commits February 25, 2025 12:54
@gian21391
Copy link
Author

I would need to check, but I think the files we take directly from GTKWave (specifically fstapi.c) currently use pthreads so to get this completely clean we would have to get Tony to change this to C++ on his end as well. If we provide a patch that may be enough. I'm not against using C++ threads for the C++ code, but we need to remember there is also C code that can only use pthreads so do we really save anything by switching the C++ code?

Here the point is to have a working version of vvp that I can compile using MSVC, rather than removing the dependency per se. That works perfectly without enabling the parallel writer.

I have now also fixed the bug related to the LXT2 writer: the LXT2 writer invoked vcd_work_terminate() in 2 separate places, which is invalid when using std::thread since all resources are freed when joining.

@gian21391 gian21391 marked this pull request as ready for review February 25, 2025 15:13
@caryr
Copy link
Collaborator

caryr commented Feb 26, 2025

Do you know if the performance is similar? Either anecdotal reports or (preferred) actual test results.

Also is the finish_cb() always called so we know vcd_work_terminate() never needs to be called from close_dumpfile()?

@gian21391
Copy link
Author

Do you know if the performance is similar? Either anecdotal reports or (preferred) actual test results.

As far as I know, POSIX threads are used for the actual implementation of std::thread, when available. At least, that's what libstdc++ does.
Moreover, here we are talking about a single thread, so any overhead would be negligible anyway.

Also is the finish_cb() always called so we know vcd_work_terminate() never needs to be called from close_dumpfile()?

Both close_dumpfile and finish_cb are used as callbacks. The first one is registered by using atexit in open_dumpfile. finish_cb is registered as a callback called at the end of the simulation in install_dumpvars_callback. Since vvp can be used in principle also as a library, I think that the solution implemented is the better one.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants