remove Scheduler #5687

vtjnash · 2014-02-05T06:11:03Z

This removes the Scheduler. @JeffBezanson you've talked about doing this for awhile since it should improve performance by avoiding the extra task switch.

@WestleyArgentum This change set frees the sysimg build of the need for starting the Scheduler. It was needed to permit pulling in other modules into the sysimg. After this change, I can add the following lines to the bottom of my sysimg.jl file to have Gtk fully & instantly available at the REPL.

Base.push!(Base.DL_LOAD_PATH, "/opt/local/lib")
Base.require("Gtk")
Base.pop!(Base.DL_LOAD_PATH)

edit: This is WIP because I need to create a flush_gc_msgs Task to avoid the warning that the scheduler is being called recursively. Also need Jeff's thoughts on how source_path should behave (cf. f9dd0bb).

WestleyArgentum · 2014-02-05T06:17:37Z

This is really really exciting! Thanks so much for working on this!

WestleyArgentum · 2014-02-05T07:16:43Z

@bass3m might also be interested in trying this out

jakebolewski · 2014-02-05T14:33:52Z

@vtjnash looking forward to precompiled packages! It looks like you pulled in some unintended changes (see test/ccall and test/linalg.jl)

Keno · 2014-02-05T15:18:11Z

current_module needs to be rooted probably.

StefanKarpinski · 2014-02-05T15:20:38Z

base/boot.jl

@@ -103,10 +103,11 @@
 #end

 # type Task
-#     parent::Task
+#     current_module::Module


How come you don't need to keep track of the parent task anymore?

The parent task used to be whoever first switched to the task, that isn't a very helpful definition and it caused issues with tasks being awakened by some other random task exiting, rather than the condition variable it was waiting for. It was broken before, but harder to notice since the Scheduler was forbidden from waiting on stuff.

vtjnash · 2014-02-05T15:20:49Z

@loladiro where? it's more rooted now than it used to be

StefanKarpinski · 2014-02-05T15:23:17Z

base/loading.jl

    end
+    return default
 end


Not searching the entire parent task chain seems rather likely to break source_path quite badly: we only populate the TLS lazily and this search process is how we find the correct source path for tasks that haven't populated this entry. We should probably generalize this approach since it's quite likely to be something that will be used for other values stored in TLS.

See above for why I think this definition of source_path may have been broken anyways. (task Alice spawns task Bob, task Bob yields for a bit then calls source_path, meanwhile Alice has finished loading the file and proceeded to include some other file. Bob now reports that it is in Alice's other file.)

The idea was for it to at least work for tasks whose lifetimes are contained within loading the file. With this change, it no longer works in that simple case.

source_path is dynamic; it's the file currently being loaded. Setting up tasks to use it potentially long after the file is done loading is a strange thing to do. A function inside the file that calls source_path will also give a different answer after loading the file. This isn't too different from the task case.

does it make sense to capture this as a per-module attribute then? Or should I restore the parent parameter, but set it equal to the task that created the task (as opposed to the task that first switched to the task)?

Maybe, but source files and modules aren't 1-to-1 so that won't really fix it. The nice thing about the parent field is that you can get good default values for things in task-local storage without copying the dictionary for each new task.

Going by the creating task instead of the first-switching task sounds like a good idea; you're right that in general somebody might make a task, stash it in the work queue, and some random other task switches to it later.

The creating task does seem like a much saner choice for the parent. The fact that the creating task and the first task that switches to it are often the same is a lucky accident.

Keno · 2014-02-05T15:33:22Z

I may be missing something, but is t->current_module rooted somewhere?

vtjnash · 2014-02-05T15:34:30Z

It is rooted in t since Task is a julia type (it didn't used to be rooted since it used to be a hidden parameter).

Keno · 2014-02-05T15:36:30Z

Ah, I see.

StefanKarpinski · 2014-02-05T15:43:17Z

test/test_sourcepath.jl

@@ -1,4 +1,8 @@
 # source path in tasks
 path = Base.source_path()
 @test endswith(path, joinpath("test","test_sourcepath.jl"))
-@test yieldto(@task Base.source_path()) == path


Breaking this test is a no-no – this definitely needs to work.

JeffBezanson · 2014-02-05T15:51:13Z

This is a great change, thanks for tackling it.

JeffBezanson · 2014-02-05T15:52:39Z

The binary file ccalltest should be removed, and the unrelated linalg test changes.

JeffBezanson · 2014-02-05T15:55:46Z

base/multi.jl

-                        perform_work()
-                        process_events(false)
-                    end
+                    yield()


This causes worker processes to busy wait (rather, this whole while true loop).

i fixed this locally, but didn't hadn't pushed it yet

StefanKarpinski · 2014-02-05T16:04:16Z

This is a great change, thanks for tackling it.

Yes, definitely a great change to have.

vtjnash · 2014-02-05T18:12:12Z

The binary file ccalltest should be removed, and the unrelated linalg test changes.

I fixed the ccalltest file. I need the linalg changes to run the tests locally. So, to me, they aren't unrelated. I should probably cherry-pick it directly onto master then rebase this.

vtjnash · 2014-02-05T21:33:05Z

this passes all tests now @JeffBezanson ping

StefanKarpinski · 2014-02-05T21:41:57Z

test/linalg.jl

@@ -196,7 +196,7 @@ debug && println("Solve upper trianguler system")

 debug && println("Solve lower triangular system")
    x = tril(a)\b
-    @test_approx_eq_eps tril(a)*x b 1000ε
+    @test_approx_eq_eps tril(a)*x b 1050ε


These changes are unrelated and may be a reversion.

my quick git blame work told me these are new tests added ~1 week ago (just prior to my branch which is why they ended up here)

staticfloat · 2014-02-05T21:44:12Z

@vtjnash I'd appreciate it if you could cherry-pick those linalg test changes, they've been blocking the OSX nightlies for almost a week now. I know @jiahao has been working on a systematic re-think of the linalg tests, but if you've got a relaxation of the bounds that works, I'd love to have that on master.

andreasnoack · 2014-02-05T21:56:01Z

@staticfloat I can relax those tests now. My only problem is that all tests pass on my own machines as well as Travis so I need some feedback. I'll relax those that have been reported already.

staticfloat · 2014-02-05T21:57:42Z

@andreasnoackjensen I have machines that fail on tests. I'll run any branches/commits you want me to and report on how it works, just ping me.

andreasnoack · 2014-02-05T21:59:39Z

Okay. Please try to run anj/relax.

staticfloat · 2014-02-05T22:14:52Z

Confirmed working.

On Wed, Feb 5, 2014 at 1:59 PM, Andreas Noack Jensen <
[email protected]> wrote:

Okay. Please try to run anj/relax.

Reply to this email directly or view it on GitHubhttps://github.com//pull/5687#issuecomment-34256403
.

WestleyArgentum · 2014-02-06T17:31:04Z

I've also been using this and testing on osx and linux. Works great so far!

kmsquire · 2014-02-06T18:14:09Z

I've also been using this and testing on osx and linux. Works great so far!

@WestleyArgentum, I'm pretty sure that @staticfloat's response was in regard to a side conversation with @andreasnoackjensen, and not to this PR. ;-)

But it's great that this PR is working for you!

WestleyArgentum · 2014-02-06T18:17:18Z

@kmsquire heh, right - sorry for the spam

… let the gc know

vtjnash · 2014-02-07T03:30:27Z

@JeffBezanson rebased && ready to merge

JeffBezanson · 2014-02-07T16:45:51Z

base/task.jl

+    local v
+    try
+        v = yieldto(P, values...)
+        if ct.last !== P


You don't have to change this now, but this almost seems more like an error --- there should be no reason a task other than P would yield to this one. Something is really broken if that happens.

Was needed to pass pollfd test

Ok, but we should look into why that happens.

ok. for some reason the old tasks get woken up, finish their work, and then wake up their parent (who was intending to wait for its new task)

First, we keep track of whether a task has asked to be in a particular module (e.g. by initiating a source file load). If it has, then current_module is always set appropriately for that task. That should be uncontroversial. Next, if a task doesn't care what module it is in, use the current_module from its parent task. This is similar to the previous commit's behavior of setting task->current_module at task creation, except that it sees changes in the parent task's current module. So if the parent task finishes loading something, the spawned task won't think it is still inside that module. In other words, remember where we got current_module rather than remembering the module itself.

state don't print fatal error for non-root task without exception handler. this is not fatal and in fact can happen regularly. small cleanups to yield_until()

now multi.jl deals only with distributed parallelism remove no-longer-used WeakRemoteRef

JeffBezanson · 2014-02-08T22:55:47Z

Ok I'm going to merge this now, as it works perfectly well. We can get deeper into other task-related changes afterwards.

remove Scheduler

WestleyArgentum · 2014-02-08T23:07:22Z

I know I say this a lot, but yay!

timholy · 2014-02-09T11:13:00Z

This is totally cool! I love having rapid access to graphics!

I noticed a couple of oddities (e.g., with Gtk compilation fails the 2nd time), but I bet you're aware of them.

My biggest concern is how developers are now going to avoid accidentally contributing their favorite sets of packages to sysimg.jl 😄.

ihnorton · 2014-02-09T13:47:28Z

Maybe add a userimg.jl and put it in .gitignore?

ivarne · 2014-02-09T14:46:41Z

Or better yet, automatically precompile packages if they are listed in a Make variable that can be set in the Make.user file.

vtjnash · 2014-02-09T18:28:23Z

I noticed a couple of oddities (e.g., with Gtk compilation fails the 2nd time), but I bet you're aware of them.

That's annoying. Somehow I hadn't run across this, but I can see why it happens. @JeffBezanson is there some black-magic we could use to delete all old bindings (esp. Modules) from Main after defining the new Base?

timholy · 2014-02-10T01:44:16Z

@ihnorton and @ivarne, I like your suggestions. @ivarne, would your approach allow one to handle the DL_LOAD_PATH issues? (See the example at the top of this issue.)

vtjnash · 2014-02-10T04:08:26Z

ihnorton's solution is nice since it allows arbitrary code to be added to the sysimg

ivarne · 2014-02-10T08:36:53Z

Arbitrary code is definitely more flexible than a make variable. I do not fully understand the LOAD_PATH case, but if packages have different requirements, a simple list of packages to include in the sysimg will be brittle and awkward.

I just wanted to suggest a way to avoid creating a new user editable file, and to simplify debugging because the automatically generated userimg.jl file can print the name of the package that is currently compiling. Automatically finding the compatible package version will also be possible.

StefanKarpinski · 2014-02-10T16:05:00Z

I think the userimg.jl idea seems good. It's not a super general solution, of course, since you have to recompile Julia to make it work, but it will make loading a bunch of packages easier for people who are willing to do so. Using a make variable is less flexible and harder to implement.

StefanKarpinski · 2014-02-12T22:26:55Z

I did the userimg.jl thing – 78e34eb. Unfortunately, it's a big fragile and you need to use Base.require instead of just require inside of that file. Based on how specific this is, it would actually be nice if this could just be a list of package names to preload, but I couldn't get that to work.

vtjnash · 2014-02-13T01:58:59Z

@StefanKarpinski It might be good to move this thread over to pull request #5746, where I address the issue of how fragile it is and the need to use Base.require instead of just require

StefanKarpinski · 2014-02-13T05:22:42Z

Ah, I didn't realize that was what you guys were talking about over there. Yes, will do.

StefanKarpinski reviewed Feb 5, 2014
View reviewed changes

JeffBezanson reviewed Feb 5, 2014
View reviewed changes

StefanKarpinski reviewed Feb 5, 2014
View reviewed changes

remove Scheduler

9c75dc8

vtjnash added 3 commits February 6, 2014 21:29

since jl_task_t->last can now be null (if the thread hasn't started),…

c0c4382

… let the gc know

remove unnecessary changes

fa97162

more cleanup of Scheduler removal change

b083531

JeffBezanson reviewed Feb 7, 2014
View reviewed changes

JeffBezanson added 3 commits February 7, 2014 21:21

propagate exception on wait() or yieldto() on a task in the exception

fc2b480

state don't print fatal error for non-root task without exception handler. this is not fatal and in fact can happen regularly. small cleanups to yield_until()

cleanup and code reorg: move scheduler code to task.jl

7923ccf

now multi.jl deals only with distributed parallelism remove no-longer-used WeakRemoteRef

JeffBezanson added a commit that referenced this pull request Feb 8, 2014

Merge pull request #5687 from JuliaLang/jn/no_scheduler

6a080ea

remove Scheduler

JeffBezanson merged commit 6a080ea into master Feb 8, 2014

JeffBezanson mentioned this pull request Feb 8, 2014

redesign Task states #5736

Closed

vtjnash mentioned this pull request Feb 9, 2014

GtkApplication JuliaGraphics/Gtk.jl#64

Open

vtjnash mentioned this pull request Feb 9, 2014

reinit Main by clearing everything in it after building the sysimg #5746

Closed

vtjnash deleted the jn/no_scheduler branch February 13, 2014 01:58

quinnj mentioned this pull request Feb 25, 2014

default packages? #1906

Closed

remove Scheduler #5687

remove Scheduler #5687

Conversation

vtjnash commented Feb 5, 2014

WestleyArgentum commented Feb 5, 2014

WestleyArgentum commented Feb 5, 2014

jakebolewski commented Feb 5, 2014

Keno commented Feb 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Feb 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented Feb 5, 2014

vtjnash commented Feb 5, 2014

Keno commented Feb 5, 2014

Choose a reason for hiding this comment

JeffBezanson commented Feb 5, 2014

JeffBezanson commented Feb 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Feb 5, 2014

vtjnash commented Feb 5, 2014

vtjnash commented Feb 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staticfloat commented Feb 5, 2014

andreasnoack commented Feb 5, 2014

staticfloat commented Feb 5, 2014

andreasnoack commented Feb 5, 2014

staticfloat commented Feb 5, 2014

WestleyArgentum commented Feb 6, 2014

kmsquire commented Feb 6, 2014

WestleyArgentum commented Feb 6, 2014

vtjnash commented Feb 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Feb 8, 2014

WestleyArgentum commented Feb 8, 2014

timholy commented Feb 9, 2014

ihnorton commented Feb 9, 2014

ivarne commented Feb 9, 2014

vtjnash commented Feb 9, 2014

timholy commented Feb 10, 2014

vtjnash commented Feb 10, 2014

ivarne commented Feb 10, 2014

StefanKarpinski commented Feb 10, 2014

StefanKarpinski commented Feb 12, 2014

vtjnash commented Feb 13, 2014

StefanKarpinski commented Feb 13, 2014