Simplified handling of RuleMessage by removing usage of std::shared_ptr #3253
Conversation
In addition to simplifying the codebase, this change introduces some minor performance improvements by replacing the copies of These results were obtained running the
|
eduar-hte
force-pushed
the
rule-message
branch
from
64375ab to
f9575ef
Compare
Addressed most of the simple issues (unrelated to this PR) reported by Sonarcloud to reduce 'technical debt'. The following issues were not addressed to avoid introducing potential bugs in code that was not implemented in this PR:
|
eduar-hte
force-pushed
the
rule-message
branch
from
0efe244 to
9982ac6
Compare
|
Hi @eduar-hte, many thanks for this great PR. Basically everything is fine, I just have one "side" question: you moved two files from This is no problem at all, you don't need to modify them, but let me ask: was that intentional or accidentally? I already thinking we should introduce a style convention (for both versions) which should be checked in CI workflow. If once we get there (at this stage), this should be reformatted, I guess. |
eduar-hte
force-pushed
the
rule-message
branch
from
9982ac6 to
ba98278
Compare
…ired to execute transactions - Simplifies memory management on error conditions - Context will be used in unit tests too, in order to provide Transaction related instances.
…ead.
- Avoids copying std::shared_ptr when lifetime of the RuleMessage
is controlled by the caller.
- The RuleMessage instance is created in RuleWithActions::evaluate and
then used to call the overloaded version of this method that is
specialized by subclasses.
- Once the call to the overloaded method returns, the std::shared_ptr
is destroyed as it's not stored by any of the callers, so it can
be replaced with a stack variable and avoid paying the cost of
copying the std::shared_ptr (and its control block that is
guaranteed to be thread-safe and thus is not a straightforward
pointer copy)
- Introduced RuleMessage::reset because this is required by
RuleWithActions::performLogging when it's not the 'last log', the rule
has multimatch and it's to be logged.
- The current version is creating allocating another instance of
RuleMessage on the heap to copy the Rule & Transaction related state
while all the other members in the RuleMessage are set to their
default values.
- The new version leverages the existent, unused and incomplete
function 'clean' (renamed as 'reset') to do this on the current
instance.
- Notice that the current code preserves the value of m_saveMessage,
so 'reset' provides an argument for the caller to control whether
this member should be reinitialized.
- src/modsecurity.cc
- Replace the redundant type with "auto".
- src/transaction.cc
- Avoid this unnecessary copy by using a "const" reference.
- test/common/custom_debug_log.cc
- Use "=default" instead of the default implementation of this special
member functions.
- Removed the unnecessary destructor override instead.
- Annotate this function with "override" or "final".
- Removed the unnecessary destructor override instead.
- Remove this "const" qualifier from the return type in all
declarations.
- test/common/modsecurity_test_context.h
- Replace the redundant type with "auto".
- test/regression/regression.cc
- Use the "nullptr" literal.
- Replace this declaration by a structured binding declaration.
- Replace "reinterpret_cast" with a safer operation.
- Addresses Sonarcloud issues: - Rewrite the code so that you no longer need this "delete". - Make the type of this variable a reference-to-const.
eduar-hte
force-pushed
the
rule-message
branch
from
ba98278 to
75d31a4
Compare
|
You're right, this was an oversight in the 'Introduce ModSecurityTestContext to encapsulate setup of objects required to execute transactions' commit. My editor sometimes does update the formatting when saving files, and I usually catch this and revert those changes. I've edited the commits to update the formatting to align with the rest of the codebase, as I don't like introducing formatting changes (that overwrite 'blame' history). Other than the moved |
I haven't used it, but Did you notice the |
Oh, many thanks! |
I still haven't used Also haven't noticed that part of In another project where I participate as developer we use astyle, which is easy to integrate into the CI workflow. We can take a look at that. Thanks again. Please let me know if you finished the work on this PR, I will merge that. |
Yes, I'm done with this PR. sonarcloud is now reporting a couple of issues about using |
|
Thanks again, @eduar-hte! |
what
Replace usage of
std::shared_ptrto pass aRuleMessageinstance with a reference.why
A
RuleMessageinstance is created inbool RuleWithActions::evaluate(Transaction *transaction)(seesrc/rule_with_actions.cc), and is then passed around many times in the evaluation of the rule with astd::shared_ptrcopy in every call. Notice that this doesn't copy theRuleMessageinstance, which could be costly, but copying astd::shared_ptris not cheaper than a reference because its control block is guaranteed to be thread-safe, and thus these copies use synchronization primitives (such as atomics) which decrease performance.Because the lifetime of the
RuleMessageis limited to theRuleWithActions::evaluatemethod where it's created, an instance can be created in the stack (instead of allocating it in the heap and creating astd::shared_ptr) and then simply pass the instance by reference in each function call.NOTE: Additionally, the current implementation doesn't validate that the received
std::shared_ptrstores a valid instance, and just dereferences it. The current version of the code guarantees that the pointer is not going to benullptrbut this is not checked nor asserted. Using a reference explicitly states in the code that each function expects a valid instance, and thus the compiler can enforce it in every method call and the code is explicit in its requirements.RuleWithActions::performLoggingcurrently creates a newRuleMessageinstance on the heap and switches it with the one held in thestd::shared_ptr(after storing the currentRuleMessagein the list of messages in theTransaction) in a specific condition (whenlastLogisfalseand the following condition holds:hasMultimatch() && isItToBeLogged. This code has been adjusted to leverage the existing (and unused)cleanmethod inRuleMessage, which has been renamed asreset, and just reset the fields in theRuleMessageinstance being used.misc
This is part of a series of PRs to improve performance of the library (6/n). Previous: #3248