Fix two scenarios of RDataFrame JIT and explicit multithreading #15588
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vepadulano
added
in:RDataFrame
clean build
pcanal
reviewed
May 21, 2024
pcanal
reviewed
May 21, 2024
Test Results 14 files 14 suites 2d 19h 1m 0s ⏱️ For more details on these failures, see this check. Results for commit 0662195. ♻️ This comment has been updated with latest results. |
vepadulano
force-pushed
the
rdf-threadsafe-jit
branch
2 times, most recently
from
afc5aba to
3249cec
Compare
pcanal
reviewed
May 22, 2024
vepadulano
force-pushed
the
rdf-threadsafe-jit
branch
from
3249cec to
10ea292
Compare
|
This PR is related to this issue #15080 |
vepadulano
force-pushed
the
rdf-threadsafe-jit
branch
from
10ea292 to
7186526
Compare
vepadulano
changed the title
|
@pcanal A second test was added for a different type of scenario (see the commit description). On my local tests, the changes in this PR already cover also the second scenario. |
Test a program with multiple threads managed explicitly by the user and each thread runs an RDataFrame with JITting. In particular, this test probes the following situation:
* Threads start building the computation graph, code to JIT goes in a global accumulating string.
* Thread 1 reaches the RLoopManager::Jit method. Checks the global string, it is not empty.
* Thread 2 arrives as well, checks the string, it is not empty.
* Thread 1 now takes a write lock on the string, moving it into the local function stack, thus leaving it empty.
* Thread 2 wants to move the string as well, but just moves an empty string.
* Thread 2 is faster than Thread 1 at passing its (empty) string to the `InterpreterCalc` function.
* The `InterpreterCalc` function receives an empty string, thus will raise an exception since `gInterpreter->Calc("")` returns a `kDangerous` interpreter error code.
In an explicit multithreading scenario inside the `RLoopManager::Jit` method, a thread might move the global string with the code to JIT into its own function stack while another thread might still believe that string is not empty. This would make the other thread call `InterpreterCalc` with an empty string which would eventually lead to an exception. Check for the string emptiness twice, once at the beginning of the function, then again after taking the write lock so that if another thread already moved the string then the current thread will not continue with JITting.
For the following scenario: * Multiple threads, multiple different RDF computation graphs (one per thread). * Some threads run a valid, working computation graph, others have some problems. * Two classes of problems are used. The first is when the declaration of some code to JIT incurs in an error in the interpreter. The second is when there is an exception at runtime. * All problems are hidden explicitly by a try-catch scope. * Threads running a working computation graph should not be impacted by the threads that are running a flawed computation graph.
vepadulano
force-pushed
the
rdf-threadsafe-jit
branch
from
7186526 to
0662195
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Re-opening this PR to check if the clad failures were real.