Thread local fallback weak bag #2844

vasilmkd · 2022-02-26T21:51:37Z

Somewhat addresses #2663.

armanbilge · 2022-02-26T21:57:38Z

I know benchmarks are incoming 😉 but does this have the same basic caveat as #2508 (comment)?

ThreadLocal is very expensive, and often exceeds the cost of contention, which is why you don't see it used too often. It also can have some complex GC implications which have their own performance costs.

vasilmkd · 2022-02-26T22:03:28Z

I wanted to update the original comment, but I'll answer here.

Benchmarks are not coming because I'm not sure what to measure exactly.

The code that this replaces is literally a ThreadLocalRandom, which is, you guessed it, a ThreadLocal. And in #2663, it was the locking contention that showed up, instead of the thread local usage.

I would like to call @yanns into the conversation if they would be willing to test this change out, with their workflow and measurement in Mission Control. Thanks in advance.

vasilmkd · 2022-02-27T00:39:04Z

Managed to come up with a benchmark.

series/3.3.x:

Benchmark                        (size)   Mode  Cnt    Score   Error  Units
ThreadLocalBenchmark.contention    2000  thrpt   20  287.505 ± 3.640  ops/s

This PR:

Benchmark                        (size)   Mode  Cnt    Score    Error  Units
ThreadLocalBenchmark.contention    2000  thrpt   20  306.480 ± 22.891  ops/s

In reality, the improvement is a bit misleading due to the bigger margin of error, so it might be a toss up, but thread locals are not strictly slower I guess.

vasilmkd · 2022-02-27T00:48:34Z

Another run is more or less the same:

This PR:

Benchmark                        (size)   Mode  Cnt    Score    Error  Units
ThreadLocalBenchmark.contention    2000  thrpt   20  312.366 ± 30.404  ops/s

durban · 2022-02-27T01:01:09Z

It's unclear to me, what guarantees that the bag.toSet call in def foreignFibers() "sees" the fibers inserted by monitorFallback, as monitorFallback accesses a bag directly, without any synchronization (and WeakBag itself does not seem thread-safe).

vasilmkd · 2022-02-27T01:22:16Z

That's great input @durban. Thank you.

Another benchmark run with the latest changes:
This PR:

Benchmark                        (size)   Mode  Cnt    Score    Error  Units
ThreadLocalBenchmark.contention    2000  thrpt   20  327.031 ± 20.432  ops/s

- If certain thread pools or executors cycle their threads, keeping weak references to each bag lets those bags be eligible for GC when their associated thread exits

armanbilge · 2022-02-28T08:32:41Z

core/jvm/src/main/scala/cats/effect/unsafe/FiberMonitor.scala

+  private[FiberMonitor] final val Bags: ThreadLocal[WeakBag[IOFiber[_]]] =
+    ThreadLocal.withInitial { () =>
+      val bag = new WeakBag[IOFiber[_]]()
+      BagReferences.offer(new WeakReference(bag))


@vasilmkd sorry, I had a follow-up question about this change.

Is it possible that we could lose track of suspended fibers, if the threads that they were suspended from no longer exist? Is that even a realistic situation 😆

That's a possibility, yes. The change was made with the intention that having an already inaccurate reporting mechanism remain that way is better than a memory leak. If people disagree, PRs are welcome.

That's fair, thanks.

Probably over-complicated but I wonder if we could use a PhantomReference to "evacuate" the contents of the bag when its owning thread gets GCed.

Btw, since the WSTP also dynamically adds/removes threads, how is this problem handled there?

The WSTP does not use this code path. I'm open to exploring Phantom References.

The WSTP does not use this code path.

Right :) but it still uses a thread-local fiber bag right? And the threads may be added/removed as the WSTP resizes itself? So it seems like it's a very similar problem.

Yeah, I wasn't sure if it's worth it :) instead of a dedicated thread, is this something we can schedule on the runtime itself?

That's what we had before. It requires solving mapping of threads to bags, which was done using locking. If we come up with a concurrent weak bag/hash map, then sure. But not even JCTools has that afaik. It's a big undertaking.

Edit: I misunderstood your comment and answered something completely different.

Scheduling on the runtime requires answering how often do to run it, which to me doesn't seem like a good strategy for something considered to be memory beneficial/critical. And ReferenceQueue is not too smart of an interface either. You can poll it in a non-blocking way, and when it returns null, when do you try again? The proper way IMO is to block on it and run cleanup on each expiry.

No it doesn't seem very elegant :) I feel like in practice, there must be some reasonable rate at which we can check the ReferenceQueue ... if an application is adding/removing threads too fast seems like its performance would be bounded by other factors anyway. But I don't really know about such things :)

After thinking about this more, seems like it could be important. A situation in which there is a deadlock seems like exactly the situation when a dynamically resizing threadpool would start culling threads due to lack of work, which could cause GC of the fiber bag holding the fibers would help diagnose the deadlock.

@djspiewak 👆🏻

vasilmkd added 3 commits February 26, 2022 22:35

Use thread local weak bags as a fallback instead of locking

1c9eb00

Remove unused class

8fcb7dd

Add a mima exclusion for the deleted class

971469a

External runtime benchmark

b7e1462

vasilmkd marked this pull request as ready for review February 27, 2022 00:48

Explicit types are needed for implicit values

21ef59e

Add a synchronization point on insert

385a14d

Guard against memory leaks in strange executors

4c8f391

- If certain thread pools or executors cycle their threads, keeping weak references to each bag lets those bags be eligible for GC when their associated thread exits

djspiewak approved these changes Feb 27, 2022

View reviewed changes

djspiewak merged commit 90b0205 into typelevel:series/3.3.x Feb 27, 2022

vasilmkd deleted the thread-local-weak-bag branch February 27, 2022 18:27

armanbilge reviewed Feb 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread local fallback weak bag #2844

Thread local fallback weak bag #2844

vasilmkd commented Feb 26, 2022

armanbilge commented Feb 26, 2022

vasilmkd commented Feb 26, 2022

vasilmkd commented Feb 27, 2022 •

edited

Loading

vasilmkd commented Feb 27, 2022

durban commented Feb 27, 2022

vasilmkd commented Feb 27, 2022

armanbilge Feb 28, 2022

vasilmkd Feb 28, 2022

armanbilge Feb 28, 2022

vasilmkd Feb 28, 2022

armanbilge Feb 28, 2022

armanbilge Feb 28, 2022

vasilmkd Feb 28, 2022 •

edited

Loading

vasilmkd Feb 28, 2022 •

edited

Loading

armanbilge Feb 28, 2022

vasilmkd Feb 28, 2022

Thread local fallback weak bag #2844

Thread local fallback weak bag #2844

Conversation

vasilmkd commented Feb 26, 2022

armanbilge commented Feb 26, 2022

vasilmkd commented Feb 26, 2022

vasilmkd commented Feb 27, 2022 • edited Loading

vasilmkd commented Feb 27, 2022

durban commented Feb 27, 2022

vasilmkd commented Feb 27, 2022

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

vasilmkd Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

vasilmkd Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasilmkd commented Feb 27, 2022 •

edited

Loading

vasilmkd Feb 28, 2022 •

edited

Loading

vasilmkd Feb 28, 2022 •

edited

Loading