Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary dynamic casts #3218

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

eduar-hte
Copy link
Contributor

@eduar-hte eduar-hte commented Aug 8, 2024

what

This short PR removes a number of dynamic_cast casts in the codebase, some of which are performed twice and could be rewritten to perform the runtime cast only once.

why

dynamic_cast is performed at runtime and requires inspecting the object's RTTI information to perform the cast. Executing it more than once incurs a bit of a performance cost that could be avoided by rewriting the code snippet. Additionally, the rewritten code is shorter and thus easier to read and maintain.

For example (from src/rule_with_actions.cc):

                    } else if (dynamic_cast<actions::Severity *>(a)) {
                        m_severity = dynamic_cast<actions::Severity *>(a);

can be rewritten as:

                    } else if (auto sa = dynamic_cast<actions::Severity *>(a)) {
                        m_severity = sa;

misc

This is part of a series of PRs to improve performance of the library (2/n). Previous: #3164

@eduar-hte eduar-hte force-pushed the remove-dynamic-casts branch 2 times, most recently from 7056476 to bc82c5a Compare August 8, 2024 13:39
@marcstern marcstern added the 3.x Related to ModSecurity version 3.x label Aug 8, 2024
@eduar-hte eduar-hte force-pushed the remove-dynamic-casts branch 8 times, most recently from a6fe127 to ba11edd Compare August 8, 2024 19:56
@eduar-hte eduar-hte force-pushed the remove-dynamic-casts branch 2 times, most recently from 705ea06 to 394c946 Compare August 8, 2024 20:22
- Refactored duplicate code in RuleWithOperator::getVariablesExceptions
- Leveraged auto to simplify declaration of dynamic_cast pointers.
Copy link

sonarcloud bot commented Aug 8, 2024

@airween
Copy link
Member

airween commented Aug 9, 2024

Thanks - I tried to measure the performance with this patch, but didn't get any significant difference (but it might improve it in real environments).

Never mind, the patch makes the code more visible indeed, going to merge.

Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

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

LGTM - thanks.

@airween airween merged commit 6f0e566 into owasp-modsecurity:v3/master Aug 9, 2024
49 checks passed
@eduar-hte eduar-hte deleted the remove-dynamic-casts branch August 9, 2024 16:20
@eduar-hte
Copy link
Contributor Author

Thanks - I tried to measure the performance with this patch, but didn't get any significant difference (but it might improve it in real environments).

That's right, these changes are not going to make a noticeable difference, but the goal is to shave off as much as possible over time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants