tikv / yatp

Yet another thread pool in rust for both callbacks or futures.
Apache License 2.0
135 stars 32 forks source link

Make futures only hold weak ptr of the core #47

Closed sticnarf closed 4 years ago

sticnarf commented 4 years ago

Closes #46

This PR makes Remote not hold a strong reference to the core and always check if the core is still alive before any operation.

This adds extra cost that upgrades a weak ptr to a strong Arc.

chained_spawn/yatp::future/1024                                                                            
                        time:   [906.10 us 913.73 us 919.75 us]
                        change: [-2.0794% -0.7563% +0.6007%] (p = 0.29 > 0.05)
                        No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
  4 (4.00%) low severe
  2 (2.00%) low mild
  4 (4.00%) high mild
  2 (2.00%) high severe
ping_pong/yatp::future/1024                                                                             
                        time:   [1.0532 ms 1.0718 ms 1.0913 ms]
                        change: [-4.8954% -2.4524% +0.2339%] (p = 0.07 > 0.05)
                        No change in performance detected.

     Running target/release/deps/spawn_many-c9d51dfaec34d6eb
spawn_many/yatp::future/1024                                                                             
                        time:   [3.6443 ms 3.7237 ms 3.7983 ms]
                        change: [-2.0927% +0.9957% +4.0339%] (p = 0.52 > 0.05)
                        No change in performance detected.

     Running target/release/deps/yield_many-939c8e996b0eade2
yield_many/yatp::future/1024                                                                             
                        time:   [3.0346 ms 3.0890 ms 3.1437 ms]
                        change: [-1.2660% +1.2049% +3.7731%] (p = 0.34 > 0.05)
                        No change in performance detected.
BusyJay commented 4 years ago

Why not just drain the queue when the last worker thread exits?

sticnarf commented 4 years ago

Why not just drain the queue when the last worker thread exits?

@BusyJay The hardest problem is not draining the existent tasks in the queue. It's to avoid wakers to push new tasks to the injector.

And a simple atomic read cannot avoid it completely. Because writing such a flag can happen between the check and pushing the task to the queue. That's why we need Weak.

BusyJay commented 4 years ago

How about creating a WeakRemote for TaskCell that only contains weak pointer to remote? In such way, there will be no circular references. Although it still requires all Remotes and pools be dropped before releasing all the futures.

/cc @Little-Wallace can this solve your problem?

Little-Wallace commented 4 years ago

@BusyJay But one Remote may be capture to a future.....

BusyJay commented 4 years ago

Yes. In my opinion, a thread pool hardly be shutdown and recreated during runtime. The library is designed under an assumption that threads are allowed to live long. We can try our best effort to avoid leak, but should optimize for the cases we focus.

BusyJay commented 4 years ago

How does it perform when using WeakRemote?

sticnarf commented 4 years ago

How does it perform when using WeakRemote?

I update the benchmark result in the PR description. We don't have remote spawns via future notifies in the benchmark, so there is not much difference.