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

integration needs thread discovery (even for e.g. `#[test]`) #2

Open
pnkfelix opened this Issue Nov 5, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@pnkfelix

pnkfelix commented Nov 5, 2015

Consider the following crate source code (small enough for inclusion in tests/smoketest.rs):

extern crate boehm_gc;

#[test]
fn hello_gc_alloc() {
    use boehm_gc::gc_allocate;
    const BYTES: usize = 408;
    println!("Hello gc_allocate {} bytes", BYTES);
    for _i in 0..1000 {
        let mut _c;
        for _l in 0..1000 {
            _c = gc_allocate(BYTES);
        }
    }
}

cargo test on this fails on the first collection cycle and aborts the process.

% cargo test
     Running target/debug/boehm_gc-bd11fb3d075b4da2

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured

     Running target/debug/smoketest-3afb7702ca6b2dad

running 1 test
Collecting from unknown thread
Process didn't exit successfully: `/Users/fklock/Dev/Rust/boehm_gc_allocator/target/debug/smoketest-3afb7702ca6b2dad` (signal: 6)

To learn more, run the command again with --verbose.

The relevant piece of information here, I think, is Collecting from unknown thread.

I believe every spawned thread needs to be registered with the Boehm collector.

There is deprecated functionality to automatically discover the spawned threads, but:

  1. It is provided only on OS X and Windows,
  2. You need to call a function to turn that on explicitly, and
  3. When I tried adding it to the mod sys; and calling it, I got a new abort on OS X, with the message Darwin task-threads-based stop and push unsupported. I have not yet investigated this.

Ideally we would just register the threads as they are created, but I do not think the Rust runtime today has a mechanism for registering a callback to accomplish this.

@swgillespie

This comment has been minimized.

Show comment
Hide comment
@swgillespie

swgillespie Nov 5, 2015

Owner

That's pretty unfortunate - the collector does expect every thread that uses the allocator to call GC_register_my_thread first before doing anything. Reading around, I'm seeing a few mailing list threads expressing similar problems about the collector and c++'s std::thread, so I'm afraid we're not alone.

I wonder, is there any precedent for languages allowing you to attach callbacks for when a thread has been spawned? I suppose we could call one here that's been set globally? I imagine that would impact the performance of spawning threads. I don't tinker much with Rust internals but I'd imagine that would add a static read and a null check as a part of the hot path of thread creation, though it pales in comparison to the cost of creating the thread itself.

Also, the collector requires the program to either call GC_allow_register_threads on the main thread or compile libgc with -DGC_ALWAYS_MULTITHREADED. Maybe that's an argument for building libgc from source as part of the crate build, since we can always define that to free people from that problem?

Also, the second abort that you discovered happened here - maybe you'll fiddle with the defines and rebuild for it to not abort. Another +1 for compiling libgc for ourselves?

Owner

swgillespie commented Nov 5, 2015

That's pretty unfortunate - the collector does expect every thread that uses the allocator to call GC_register_my_thread first before doing anything. Reading around, I'm seeing a few mailing list threads expressing similar problems about the collector and c++'s std::thread, so I'm afraid we're not alone.

I wonder, is there any precedent for languages allowing you to attach callbacks for when a thread has been spawned? I suppose we could call one here that's been set globally? I imagine that would impact the performance of spawning threads. I don't tinker much with Rust internals but I'd imagine that would add a static read and a null check as a part of the hot path of thread creation, though it pales in comparison to the cost of creating the thread itself.

Also, the collector requires the program to either call GC_allow_register_threads on the main thread or compile libgc with -DGC_ALWAYS_MULTITHREADED. Maybe that's an argument for building libgc from source as part of the crate build, since we can always define that to free people from that problem?

Also, the second abort that you discovered happened here - maybe you'll fiddle with the defines and rebuild for it to not abort. Another +1 for compiling libgc for ourselves?

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 5, 2015

@swgillespie I spent some time today hacking up exactly what you describe: I added a callback registration system invoked on every thread start. I don't think the overhead will be too high (at least for someone who isn't using the callbacks -- i.e., our standard "zero cost" goal).

With that in place, and overriding the test driver harness to call my own fn main that both sets up GC_allow_register_threads and the thread registration callback, I was able to get the smoketest to work. Yay!

I'll talk to the libs team about how feasible it might be to add such a callback system.

pnkfelix commented Nov 5, 2015

@swgillespie I spent some time today hacking up exactly what you describe: I added a callback registration system invoked on every thread start. I don't think the overhead will be too high (at least for someone who isn't using the callbacks -- i.e., our standard "zero cost" goal).

With that in place, and overriding the test driver harness to call my own fn main that both sets up GC_allow_register_threads and the thread registration callback, I was able to get the smoketest to work. Yay!

I'll talk to the libs team about how feasible it might be to add such a callback system.

@swgillespie

This comment has been minimized.

Show comment
Hide comment
@swgillespie

swgillespie Nov 6, 2015

Owner

@pnkfelix Cool, that's really awesome!! I look forward to hearing what the libs team has to say, it would be really exciting to get this fully working!

I suppose that in the event of the libs team rejecting this sort of thing, we could either 1) provide an alternative thread-spawning API that registers the thread upon creation, while requiring users to call GC_register_my_thread on any threads spawned through thread::spawn or otherwise, or 2) simply require that GC_register_my_thread get called on any and all threads that allocate.

Both of those have the major downside that they break any code that spawns a thread and allocates immediately afterward, like the test harness, but I guess if you're swapping out the allocator you run the risk of bad things? I don't like it, but maybe there's no other option - libgc works by #defining pthread_create to be a function that does this.

Owner

swgillespie commented Nov 6, 2015

@pnkfelix Cool, that's really awesome!! I look forward to hearing what the libs team has to say, it would be really exciting to get this fully working!

I suppose that in the event of the libs team rejecting this sort of thing, we could either 1) provide an alternative thread-spawning API that registers the thread upon creation, while requiring users to call GC_register_my_thread on any threads spawned through thread::spawn or otherwise, or 2) simply require that GC_register_my_thread get called on any and all threads that allocate.

Both of those have the major downside that they break any code that spawns a thread and allocates immediately afterward, like the test harness, but I guess if you're swapping out the allocator you run the risk of bad things? I don't like it, but maybe there's no other option - libgc works by #defining pthread_create to be a function that does this.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment