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

[WIP] Introducing libuv global lock on master #29706

Closed
wants to merge 8 commits into from

Conversation

anton-malakhov
Copy link

@anton-malakhov anton-malakhov commented Oct 18, 2018

As title
Partially replaces #29606 (removing kp/partr specifics)

@StefanKarpinski
Copy link
Member

Out of curiosity, why not just continue it there?

@anton-malakhov
Copy link
Author

I rebased it against master and wanted to keep original branch name for kp/partr-based branch there. GitHub does not allow changing source branch in a PR. If I'd retarget that PR to master 1) I'd loose few partr-specific changes which I can just keep in that branch and 2) it would look like if I want to merge kp/partr :)

@StefanKarpinski
Copy link
Member

GitHub does not allow changing source branch in a PR.

It does: click Edit and then choose a different base branch.

@anton-malakhov
Copy link
Author

anton-malakhov commented Oct 19, 2018

I know it allows to change base branch but I said it does not allow changing source branch.
Let me probably clarify. I created initial PR against kp/partr branch because we were in a rush of making it stable during our F2F and anticipated that we can enable more than one thread by the end of the week. So, I made these quick changes in order to prepare for it at least in the libuv part and assuming it will remain being under review after merge into kp/partr. However, as Kiran's PR keeps struggling with more and more issues with single thread and I started to get full-fledged reviews for what was supposed to be a quick hack to meet our deadlines, I've decided to separate it out from kp/partr and continue addressing reviews and getting real CI tests on master branch. Thus I need new source branch. So, here we are.

@StefanKarpinski
Copy link
Member

Closes issue #14494.

@c42f
Copy link
Member

c42f commented Jan 25, 2019

It looks like @yuyichao already reviewed this (partially? fully?) over at #29606.

Is the plan still to merge this before merging the other more PARTR-specific work? If so, what's holding it up and would it benefit from another review?

@anton-malakhov
Copy link
Author

He made good point on not using JL_LOCK_NOGC. However, it never worked for me the easy way, triggering some bug or a problem during bootstrap. I'll be glad if somebody will take over this patch since my work is no longer connected to supporting development of multi-threading in Julia-lang

@c42f
Copy link
Member

c42f commented Jan 25, 2019

Ah that's a real shame. I could possibly pick it up. No promises yet though, I'll have to see what's involved.

@vtjnash what's your take on the order this stuff should get merged?

@StefanKarpinski
Copy link
Member

@vtjnash what's your take on the order this stuff should get merged?

In a recent compiler call we discussed this and his take was that this is completely independent of the other multithreading work. It can happen before or after—it just affects whether calling I/O from different Julia threads is safe or not, which is an issue now and would still be an issue after the other threading PRs are merged as well.

@mbauman mbauman added the multithreading Base.Threads and related functionality label Mar 5, 2019
@JeffBezanson
Copy link
Member

Rebased as #31437.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants