Avoid doing extra TLS lookups in FunctionParser. #13745
Merged
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
|
Would you mind explaining some more what changed? I see that you introduced another data structure but which part is actually responsible for reducing the number of calls to lock the mutex? |
drwells
commented
May 16, 2022
bangerth
reviewed
May 16, 2022
Comment on lines
267
to
300
| * std::is_copy_constructible gives the wrong answer for containers with | ||
| * non-copy constructible types (e.g., std::vector<std::unique_ptr<int>>). | ||
| * Work around that problem by defining our own explicitly uncopyable | ||
| * per-thread data class (instead of, e.g., using std::pair). | ||
| * | ||
| * We define exactly one per-thread data structure to avoid locking and | ||
| * unlocking the TLS data lookup more than once per function call. | ||
| */ | ||
| mutable Threads::ThreadLocalStorage<std::vector<double>> vars; | ||
| struct ParserData | ||
| { | ||
| ParserData() = default; | ||
|
|
||
| ParserData(const ParserData &) = delete; | ||
|
|
||
| /** | ||
| * Scratch array used to set independent variables (i.e., x, y, and z) | ||
| * before each muParser call. | ||
| */ | ||
| std::vector<double> vars; | ||
|
|
||
| /** | ||
| * muParser implementations. | ||
| */ | ||
| std::vector<std::unique_ptr<internal::muParserBase>> parsers; | ||
| }; |
There was a problem hiding this comment.
Isn't this the exact same class as for the function_parser.h file?
There was a problem hiding this comment.
Yes. I need to move some more functions into mu_parser_internal.h - if its OK with you I would like to do that refactoring in the next PR.
masterleinad
approved these changes
May 17, 2022
While we are at it, get rid of the calls to begin_raw() and end_raw() (they are deprecated).
|
Thanks for fixing this and to @masterleinad for doing the merge. I'm at the ASPECT hackathon, it's been a busy week. |
mkghadban
pushed a commit
to OpenFCST/dealii
that referenced
this pull request
Sep 8, 2022
Avoid doing extra TLS lookups in FunctionParser.
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.
Fixes #13578 to the largest extent possible right now. There may be better thread locking and unlocking strategies but by moving some things around I can evaluate simple expressions about 3x faster.
While we are at it, get rid of the calls to begin_raw() and end_raw() (they are deprecated).