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

Scheduling and Task Safety Improvements #753

Closed
wants to merge 11 commits into from
Closed

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Jul 27, 2011

Schedulers now each have their own lock, which reduces the contention on the big scheduler lock. This means we regularly get much better performance out of tasks.

Scheduler threads also wait better. Rather than using sync::sleep, they use lock.wait(), which lets them be woken up quicker when tasks become unblocked. Again, a performance improvement.

To make this not crash all the time, it was necessary to use atomic reference counts for task. Most of this will be undone later though, as we move to a handle-based system.

@brson
Copy link
Contributor

brson commented Jul 27, 2011

I haven't reviewed everything thoroughly, but I agree in principle with what's going on here. I'm not sure about the atomic reference counting for tasks - having those have special reference counting needs kinda smells funny.

There's a good bit of asserts and logging that are lost in these patches.

Running this locally I see regressions in multithreaded runs of the make check, invalid reads that didn't exist before. They all seem to originate in task::worker, which is really terribly unsafe, so I'm in the process of removing it from the codebase. I'll see if I get better results then.

@brson
Copy link
Contributor

brson commented Jul 27, 2011

OK, I checked in a commit that removed task::worker and things seem much more reliable now

@brson
Copy link
Contributor

brson commented Jul 27, 2011

Well, single-threaded anyway. With 'RUST_THREADS=32 make check-stage1-cfail' I get errors like:

==2124== Thread 5:
==2124== Invalid read of size 4
==2124== at 0x48FFE5D: rust_chan::disassociate() (sync.h:32)
==2124== by 0x48FFF36: rust_chan::destroy() (rust_chan.cpp:148)
==2124== by 0x4901580: upcall_del_chan (rust_upcall.cpp:155)
==2124== by 0x8078A08: glue_free322 (in /home/banderson/Dev/rust/build/test/compiletest.stage1)
==2124== by 0x80789C2: glue_drop321 (in /home/banderson/Dev/rust/build/test/compiletest.stage1)
==2124== by 0x807890F: glue_drop320 (in /home/banderson/Dev/rust/build/test/compiletest.stage1)
==2124== by 0x80724C8: compiletest::procsrv::worker (in /home/banderson/Dev/rust/build/test/compiletest.stage1)
==2124== by 0x8078130: compiletest::procsrv::mk::anon298 (in /home/banderson/Dev/rust/build/test/compiletest.stage1)
==2124== by 0x8077F88: compiletest::procsrv::mk::spawn_wrapper297 (in /home/banderson/Dev/rust/build/test/compiletest.stage1)
==2124== by 0x48FF050: task_start_wrapper (rust_task.cpp:126)

and

free: ptr 0xcd64fb4 is not in allocation_list
rt: fatal, 'not in allocation_list' failed, ../src/rt/memory_region.cpp:47
==4557== 144 bytes in 1 blocks are possibly lost in loss record 33 of 115
==4557== at 0x48E8327: calloc (vg_replace_malloc.c:467)
==4557== by 0x4410FC7: allocate_dtv (dl-tls.c:300)
==4557== by 0x441175B: _dl_allocate_tls (dl-tls.c:464)
==4557== by 0x4C025A6: pthread_create@@GLIBC_2.1 (allocatestack.c:570)
==4557== by 0x48F8A4F: rust_thread::start() (sync.cpp:47)
==4557== by 0x48F92EB: rust_start (rust.cpp:147)
==4557== by 0x807A397: main (in /home/banderson/Dev/rust/build/test/compiletest.stage1)
==4557==
==4557== 4,608 bytes in 32 blocks are possibly lost in loss record 97 of 115
==4557== at 0x48E8327: calloc (vg_replace_malloc.c:467)
==4557== by 0x4410FC7: allocate_dtv (dl-tls.c:300)
==4557== by 0x441175B: _dl_allocate_tls (dl-tls.c:464)
==4557== by 0x4C025A6: pthread_create@@GLIBC_2.1 (allocatestack.c:570)
==4557== by 0x48F8A4F: rust_thread::start() (sync.cpp:47)
==4557== by 0x4905EFB: rust_kernel::start_task_threads() (rust_kernel.cpp:257)
==4557== by 0x48F954D: rust_start (rust.cpp:164)
==4557== by 0x807A397: main (in /home/banderson/Dev/rust/build/test/compiletest.stage1)

@eholk
Copy link
Contributor Author

eholk commented Jul 27, 2011

Thanks, brson!

Atomic reference counting is there because tasks are reference by multiple threads. No other data (well... almost) is shared between tasks, which is why the task structure itself is special. This is a temporary measure anyway, eventually we'll just have handles.

Eric Holk added 10 commits July 27, 2011 16:46
Tasks are spawned on a random thread. Currently they stay there, but
we should add task migration and load balancing in the future. This
should drammatically improve our task performance benchmarks.
that absolutely will not succeed with a large default stack. This
should be removed once we have stack grown working.

Also updated word-count to succeed under the new test framework.
…s appears to give us much better parallel performance.

Also, commented out one more unsafe log and updated rust_kernel.cpp to compile under g++
…towards atomic reference counting of tasks.
@brson
Copy link
Contributor

brson commented Jul 28, 2011

After further experimentation I don't think the valgrind problems I reported are caused by this patch set.

@eholk
Copy link
Contributor Author

eholk commented Jul 28, 2011

Integrated.

@eholk eholk closed this Jul 28, 2011
pdietl pushed a commit to pdietl/rust that referenced this pull request Apr 23, 2020
ZuseZ4 added a commit to EnzymeAD/rust that referenced this pull request Mar 7, 2023
* add testcase for enzyme loads behind phis

* handle load trough phis

* adjust test for older llvms

* adjust test for older llvms

* adjust test for even older llvms
celinval pushed a commit to celinval/rust-dev that referenced this pull request Jun 4, 2024
* Move compiletest out of `x.py`

* Fixups

* Add `pwd` to `--build-base` and split cmd into several lines

* Use correct mode for `rmc-docs` suite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants