opalj / opal

https://www.opal-project.de
Other
51 stars 27 forks source link

Evaluation-Depth in `PKECPropertyStore` is not thread-safe #208

Closed maximilianruesch closed 2 months ago

maximilianruesch commented 2 months ago

The evaluationDepth local field is used in the doApply method of the PKECPropertyStore and as such modified by all worker threads of this property store. Due to many threads usually being active at the same time, this variable might experience race conditions and consistency problems.

In my case, while working on #1, the evaluationDepth reached negative values up to -300, which completely defeated the purpose of the MaxEvaluationDepth limit that is configured in the reference.conf.

The aforementioned value should be made thread-safe by giving each thread their own value, since it is used to decide when to schedule computation tasks to a different thread. There might be no trivial way to go about this while maintaining cache coherence, but ignoring that one could just put up an array-like data structure containing values for all PKECThreads + the main thread (since forcing value computations outside of workers is done in e.g. the tests).

Since the doApply method is located outside the PKECThreads, a simple field inside this class cannot be used, and moving contexts around to the code inside doApply can be difficult. Obtaining a thread id (with the main thread obtaining the last one) can be done via:

val threadId = Thread.currentThread() match {
    case thread: PKECThread => threads.indexOf(thread)
    case _                  => THREAD_COUNT
}

[!NOTE] Implementers of this issue might want to have a look at ThreadLocal Variables, since wrapping the current variable in such an instance might just do the job.

errt commented 2 months ago

ThreadLocal is probably the way to go here. It's not super efficient, but it should still be more efficient than using a dedicated map.

Only alternative I see given the problems that you mention with doApply not being part of the threads themselves might be to have a regular field in the threads (plus one in the store for the main thread), then get the thread object like you show and access the field via that. Still, the final field would not be thread safe (who's to say that only the main thread could call doApply?) and I'm not sure currentThread is super efficient either.