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.

Sign up for GitHub

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

Jump to bottom

Move to std types for concurrency #1731

Merged
merged 9 commits into from
Jan 30, 2019
Merged

Move to std types for concurrency #1731

merged 9 commits into from
Jan 30, 2019

Conversation

jackgerrits
Copy link
Member

Closes #1725

  • Moves to std::thread, std::condition_variable and std::mutex
  • Uses scope based locking for better exception safety
  • Move from calloc to new so that destructor will be called and object local types will be freed
  • Had to trick the compiler a bit when compiling for CLR

@jackgerrits
Copy link
Member Author

This actually contains a regression, I think a deadlock. I'm going to close it until I get time to figure it out

kumpera
kumpera reviewed Jan 29, 2019
View reviewed changes
vowpalwabbit/parser.cc Outdated Show resolved Hide resolved
vowpalwabbit/parser.h Show resolved Hide resolved
vowpalwabbit/parser.h Show resolved Hide resolved
@jackgerrits jackgerrits reopened this Jan 29, 2019
@JohnLangford
Copy link
Member

This seems like a clear win from a code complexity and dependency standpoint. Let's merge after addressing minor comments.

@JohnLangford JohnLangford merged commit 6d9194a into VowpalWabbit:master Jan 30, 2019
jackdoe pushed a commit to lucjb/vowpal_wabbit that referenced this pull request Mar 7, 2019
@jackgerrits
Move to std types for concurrency (VowpalWabbit#1731)
08d4ab1 
* change to std datatypes for multithreading. Still need to fix usage in CLR mode, should probably use pimpl pattern

* Fix concurrency classes for managed cpp

* unwind recursive call and fix deadlock

* Address comments

* Add comment

* Add back required undef and define
@jackgerrits jackgerrits deleted the jagerrit/std_multithreading branch April 5, 2019 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumpera kumpera kumpera left review comments

@JohnLangford JohnLangford JohnLangford left review comments

@lokitoth lokitoth lokitoth left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jackgerrits @JohnLangford @kumpera @lokitoth