Skip to content

Add mutex to setEpsilon#4112

Merged
lindsayad merged 1 commit intolibMesh:develfrom
dschwen:atomic_fparser_epsilon_4111
Mar 24, 2025
Merged

Add mutex to setEpsilon#4112
lindsayad merged 1 commit intolibMesh:develfrom
dschwen:atomic_fparser_epsilon_4111

Conversation

@dschwen
Copy link
Copy Markdown
Member

@dschwen dschwen commented Mar 20, 2025

Closes #4111

Thanks @lindsayad for pointing me to thread_local

@dschwen dschwen force-pushed the atomic_fparser_epsilon_4111 branch from 1995f53 to 598fb8b Compare March 20, 2025 17:29
@roystgnr
Copy link
Copy Markdown
Member

This scares me a bit. If I try to set epsilon to a new value while I'm in a non-threaded section of code, and then I enter into threads, what happens? My first guess would be "if new threads are being forked then they inherit the new value, but if we're using an underlying thread pool then only one of the threads in the pool has the new value", and if that's the case then that would be horrifying.

If there's already a setter function and we're only ever using that, could we just add a lock in the setter instead?

@lindsayad
Copy link
Copy Markdown
Member

excellent point

@moosebuild
Copy link
Copy Markdown

moosebuild commented Mar 20, 2025

Job Coverage, step Generate coverage on a7490b9 wanted to post the following:

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.

@dschwen
Copy link
Copy Markdown
Member Author

dschwen commented Mar 20, 2025

We'll my first commit just makes epsilon atomic. That seems like the easiest way.

@lindsayad
Copy link
Copy Markdown
Member

Push it

@dschwen dschwen force-pushed the atomic_fparser_epsilon_4111 branch from ec4b80d to 84274e2 Compare March 21, 2025 16:49
lindsayad
lindsayad previously approved these changes Mar 22, 2025
@lindsayad lindsayad dismissed their stale review March 22, 2025 04:45

Oops jk it's not building :laugh:

@dschwen
Copy link
Copy Markdown
Member Author

dschwen commented Mar 22, 2025

arithmetic operators aren’t overloaded for std::atomic types.

😭

meaning we'd have to add a crapton of .load().

@dschwen dschwen force-pushed the atomic_fparser_epsilon_4111 branch from 84274e2 to 1a7c594 Compare March 22, 2025 18:18
@dschwen dschwen force-pushed the atomic_fparser_epsilon_4111 branch from 1a7c594 to a7490b9 Compare March 22, 2025 18:19
@dschwen dschwen changed the title Make epsilon thread_local Add mutex to setEpsilon Mar 22, 2025
@dschwen
Copy link
Copy Markdown
Member Author

dschwen commented Mar 22, 2025

This scares me a bit. If I try to set epsilon to a new value while I'm in a non-threaded section of code, and then I enter into threads, what happens? My first guess would be "if new threads are being forked then they inherit the new value, but if we're using an underlying thread pool then only one of the threads in the pool has the new value", and if that's the case then that would be horrifying.

The right answer would be to set epsilon in the thread you expect it to be used (which is what we do in MOOSE). thread_local would be the smallest possible change to fix the underlying issue, and probably the best performing...

Copy link
Copy Markdown
Member

@roystgnr roystgnr left a comment

Choose a reason for hiding this comment

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

Looks good to me now; we'll merge in a day or so unless John has any complaint.

Copy link
Copy Markdown
Member

@jwpeterson jwpeterson left a comment

Choose a reason for hiding this comment

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

My only question was whether making the std::mutex itself static creates another kind of race condition, but that appears to be safe to do.

@lindsayad lindsayad merged commit f4b26a9 into libMesh:devel Mar 24, 2025
20 checks passed
@dschwen
Copy link
Copy Markdown
Member Author

dschwen commented Mar 24, 2025

My only question was whether making the std::mutex itself static creates another kind of race condition, but that appears to be safe to do.

Love the first reply:
Screenshot_20250324_122158_Chrome

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the global FParser epsilon value thread_local

5 participants