Skip to content

[Core] Add parallel try catch to remaining raw omp loops in builder and solver#14319

Open
rfaasse wants to merge 4 commits intomasterfrom
core/add-parallel-try-catch-to-remaining-raw-omp-loops
Open

[Core] Add parallel try catch to remaining raw omp loops in builder and solver#14319
rfaasse wants to merge 4 commits intomasterfrom
core/add-parallel-try-catch-to-remaining-raw-omp-loops

Conversation

@rfaasse
Copy link
Copy Markdown
Contributor

@rfaasse rfaasse commented Mar 25, 2026

📝 Description
In a previous PR #14193, one of the raw omp loops was provided with error handling. We have found that the other raw OpenMP loops in the same class, the ResidualBasedBlockBuilderAndSolver, result in similar hard crashes on windows.

In the aforementioned PR it was proven that adding these try-catch macros don't have a significant impact on runtime.

There is one small discussion point here about the loop index i, for which I'll add a clarifying comment close to the concerning code.

@rfaasse rfaasse self-assigned this Mar 25, 2026
@rfaasse rfaasse requested a review from a team as a code owner March 25, 2026 06:53
@rfaasse rfaasse added the Parallel-SMP Shared memory parallelism with OpenMP or C++ Threads label Mar 25, 2026
Comment on lines +231 to +244
@@ -237,11 +241,14 @@ class ResidualBasedBlockBuilderAndSolver
Assemble(A, b, LHS_Contribution, RHS_Contribution, EquationId);
}

KRATOS_CATCH_THREAD_EXCEPTION
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macro KRATOS_CATCH_THREAD_EXCEPTION expects i to be in scope to report on the thread number (see the following snippet). This is why I renamed the loop variable to i in multiple occasions in this PR. I know that the loop variable technically doesn't represent the thread number. I would like to have your opinions on how to solve this (e.g. do we leave it as is, change the macro, get the thread number in some other way, or some other solution?).

    #define KRATOS_CATCH_THREAD_EXCEPTION \
    } catch(Exception& e) { \
        KRATOS_CRITICAL_SECTION \
        err_stream << "Thread #" << i << " caught exception: " << e.what(); \
    } catch(std::exception& e) { \
        KRATOS_CRITICAL_SECTION \
        err_stream << "Thread #" << i << " caught exception: " << e.what(); \
    } catch(...) { \
        KRATOS_CRITICAL_SECTION \
        err_stream << "Thread #" << i << " caught unknown exception:"; \
    }

@loumalouomega
Copy link
Copy Markdown
Member

LGTM, I guess the performance penalty will be similar to the previous one. Can you run the benchmark again to verify?

@rfaasse
Copy link
Copy Markdown
Contributor Author

rfaasse commented Mar 25, 2026

LGTM, I guess the performance penalty will be similar to the previous one. Can you run the benchmark again to verify?

Thanks for the review! The benchmark only checks the BuildRHS function. In that benchmark we use in a simplified case with 'empty' elements, so it's almost testing an empty loop, so since we found that even with an almost empty loop body the try-catch does not impact performance in any meaningful way, that means that for these other functions, it also can't have a significant impact (since for non-empty loop bodies in a realistic case, the effect will be even less) 👍

@matekelemen
Copy link
Copy Markdown
Contributor

I would like to have your opinions on how to solve this (e.g. do we leave it as is, change the macro, get the thread number in some other way, or some other solution?).

Since KRATOS_CATCH_THREAD_EXCEPTION is only used in parallel_utilities.h and here, I'd change the macro such that it expects the thread index as an argument.

Otherwise, looks fine.

@rfaasse
Copy link
Copy Markdown
Contributor Author

rfaasse commented Mar 30, 2026

I would like to have your opinions on how to solve this (e.g. do we leave it as is, change the macro, get the thread number in some other way, or some other solution?).

Since KRATOS_CATCH_THREAD_EXCEPTION is only used in parallel_utilities.h and here, I'd change the macro such that it expects the thread index as an argument.

Otherwise, looks fine.

Nice suggestion, implemented! This did mean I needed to create some way of getting the current thread ID from OpenMP in the builder and solver. Curious what you and the @KratosMultiphysics/technical-committee think!

@matekelemen
Copy link
Copy Markdown
Contributor

matekelemen commented Mar 31, 2026

Thanks @rfaasse, looks good.

Before someone mentions performance issues related to omp_get_thread_num, I'll note that it only gets called once an exception is thrown and caught, so no overhead on the happy path.

I'll leave the approval to the @KratosMultiphysics/technical-committee @RiccardoRossi @pooyan-dadvand @sunethwarna @roigcarlo @rubenzorrilla.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Parallel-SMP Shared memory parallelism with OpenMP or C++ Threads

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[GeoMechanicsApplication] Kratos run (sometimes) results in hard crash when element is inverted

3 participants