DynamicPPL.jl documentation for PR#1078 is available at:
https://TuringLang.github.io/DynamicPPL.jl/previews/PR1078/

This is v intersting. Here are my completely unprompted thoughts:

1. I've never used ReentrantLock before so I'm not sure whether the drop performance is to do with this specific method of concurrency, or whether it's a concurrency thing in general. The past times I've worked with concurrency, it's always via a Channelhttps://docs.julialang.org/en/v1/base/parallel/#Base.Channel. You can queue (or buffer) up to N things that you want to do, and then there's a separate thread that is solely dedicated to doing those things one by one. My suspicion is that the lock is completely unbuffered and I wonder if changing the buffer size makes a difference. (My recollection when working on AbstractMCMC parallel progress bars is that it makes a difference, although unfortunately I don't remember whether it makes it better or worse!)

2. I didnot implement this so I'm not saying that I think this is the right way to do things or that you should change it. But in my mind when I was thinking about these things, I was thinking that the lock should be at the higher level of tilde_obssume. What that means is that instead of queuing individual getindex / setindex operations, you queue entire tilde_obssume calls. The assumption of course is that everything within a single tilde_obssume is threadsafe. I don't know if that is better or worse. It could be quite conceivably worse.

Codecov Report

❌ Patch coverage is0% with131 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.06%. Comparing base (9bd8f16) to head (8fcfaf4).
⚠️ Report is 2 commits behind head on breaking.

@@             Coverage Diff              @@##           breaking    #1078      +/-   ##============================================- Coverage     82.40%   78.06%   -4.34%============================================  Files            42       41       -1       Lines          3791     3848      +57     ============================================- Hits           3124     3004     -120- Misses          667      844     +177

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

hold on, why is CI failing........ 😱

A local experiment shows me that about 80% of the time in the 8-core case is due to the@lock vi.lock thing inmap_accumulators!!, which is whataccumulate_obssume!! calls. In case you were wondering, like me, whether the problem was with theRefs or in some unexpected place.

We should try locking at a higher level, like attilde_obbsume level. The nice thing with being lower down is that you can do thelogpdf calls in parallel and only drop to serial execution when accumulating the results inaccumulate_observe!!. We could also try locking at a lower level, namely at the level of a individual accumulators. I have no good intuition for whether these would be better/worse.

I've never usedChannels and it's not obvious to me how to use them here. I guess you would queue something likeaccumulate_observe!! calls? Would need to think about how to manage the non-mutating nature of many accumulators.

A quick thought: we could introduce locks at the model level. This would prevent users from using threading, except through an explicit opt-in mechanism such as a hypotheticalx ~ threadsafe_product_distribution(...).

The nice thing with being lower down is that you can do thelogpdf calls in parallel and only drop to serial execution when accumulating the results inaccumulate_observe!!

Oh, yes, that's completely correct. I didn't think of that. I guess that means even the current implementation here is too high level right? Because the logpdf calculation is actually done by the accumulator itself, way way inside map_accumulator.

And yeah, channels are pretty ugly to work with. The general structure of channel based code looks like this

Distributed.@syncbegin    Distributed.@asyncbegin# read from the channel and do things with it one at a timeend    Distributed.@asyncbegin# here do whatever you like that writes to the channelendend

I think the whole thing would get lumped inside_evaluate!!, the first async block we would have to custom write, the second one would basically be model evaluation.

Because the Channel must contain concrete objects (not function calls) probably we'd need a struct that contains all the arguments to accumulate_obssume plus a boolean to indicate whether it's an assume or observe.

I guess that means even the current implementation here is too high level right? Because the logpdf calculation is actually done by the accumulator itself, way way inside map_accumulator.

Oh yes, silly me.

Another option could be to not even try to support threadedtilde_assume!!, but replaceThreadSafeVarInfo with accumulators that useThreads.Atomic. This could be really simple and easy for LogProbAccumulators.

I like the sound of AtomicAccumulator. In a way that is better because we want things like addlogprob to also be threadsafe, not just tildes. I think threadsafe distributions are also a good idea but atm I don't know how to generalise them. It would work very nicely for the iid case though.

I think the problem with atomic accumulators is that if we take that road, all accumulators must be made thread-safe. I can easily wrap theLogPriorAccumulator.logp field in aThreads.Atomic and be very happy, but then I'm also committed to makingDebugAccumulator thread-safe and that doesn't sound fun.

A possible way out would be to have an abstract typeThreadSafeAccumulator <: AbstractAccumulator, and anything that is not of that type falls back onto using locks. I don't care if I pay an overhead for using locks on something likePointwiseLogdensityAccumulator. Not sure where to put this fallback locking code though, in such a way that it doesn't complicate and cause overheads in theaccummulate_obssume!! call stack.