Skip to content

Add _thread.start_new_thread #1936

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
Merged
merged 11 commits into from
May 25, 2020
Merged

Add _thread.start_new_thread #1936

merged 11 commits into from
May 25, 2020

Conversation

coolreader18
Copy link
Member

@coolreader18 coolreader18 commented May 17, 2020 •
edited
Loading

Dependent on #1933 and #1935

@coolreader18 coolreader18 force-pushed the coolreader18/_thread-start_new_thread branch 6 times, most recently from 5df519d to 94dccb3 Compare May 22, 2020 17:35
@coolreader18 coolreader18 marked this pull request as ready for review May 22, 2020 17:35
palaviv
palaviv approved these changes May 23, 2020
View reviewed changes
Copy link
Contributor

@palaviv palaviv left a comment

Choose a reason for hiding this comment

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

Looks great. Can you add a small snippet to see this running?

@coolreader18
Copy link
Member Author

test_threading does some of that already, but I think I could probably verify the ordering now/make sure that it's actually concurrent somehow.

@coolreader18 coolreader18 force-pushed the coolreader18/_thread-start_new_thread branch from 94dccb3 to a6f6b1a Compare May 24, 2020 00:37
@coolreader18 coolreader18 force-pushed the coolreader18/_thread-start_new_thread branch from a6f6b1a to d12a793 Compare May 24, 2020 01:14
@coolreader18
Copy link
Member Author

@palaviv I was actually able to get test.test_thread passing with not too much effort, so I think that should work for tests. I guess the _thread module really doesn't have that much API surface.

palaviv
palaviv approved these changes May 24, 2020
View reviewed changes
Copy link
Contributor

@palaviv palaviv left a comment

Choose a reason for hiding this comment

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

Looks great. I missed that there are already tests for this.

vm/Cargo.toml Outdated
@@ -71,7 +71,7 @@ smallbox = "0.8"
bstr = "0.2.12"
crossbeam-utils = "0.7"
generational-arena = "0.2"
parking_lot = { git = "https://github.com/Amanieu/parking_lot" }
parking_lot = { git = "https://github.com/coolreader18/parking_lot", branch="raw-re-is_locked" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I was a bit iffy when we added the parking_lot from Github. For now at least add a TODO to change this.

Copy link
Member Author

@coolreader18 coolreader18 May 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That was just temporary, the PR for that is merged now. I'll still add a TODO for the official git version, though, hopefully that will be published on crates.io soon.

@coolreader18 coolreader18 force-pushed the coolreader18/_thread-start_new_thread branch from d12a793 to 90223d8 Compare May 25, 2020 02:03
@coolreader18 coolreader18 merged commit e14adc4 into master May 25, 2020
@coolreader18 coolreader18 deleted the coolreader18/_thread-start_new_thread branch May 25, 2020 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@palaviv palaviv palaviv approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@coolreader18 @palaviv