Use std::shared_ptr for variable resolution

[WGH-]

AnchoredSetVariable::resolve is called for every rule (see RuleWithOperator::evaluate). The previous implementation allocated a new copy of every variable, which quickly added up. In my tests, AnchoredSetVariable::resolve function consumed 7.8% of run time.

AnchoredSetVariable (which is a multimap) values are never changed, only added. This means it's safe to store them in std::shared_ptr, and make resolve return shared_ptr pointing to the same object.

Other resolve implementation could also use this optimization by not allocating new objects, however, they are not hot spots, so this optimization was not implemented there.

In my benchmark, this raises performance from 117 requests per second to 131 RPS, and overhead is lowered from 7.8% to 2.4%.

As a bonus, replacing plain pointer with smart pointers make code cleaner, since using smart pointers makes manual deletes no longer necessary.

Additionally, VariableOrigin is now stored in plain std::vector, since it's wasteful to store structure containing just two integer values using std::list<std::unique_ptr<T>>.

Before (the thing to the left of executeTransformations labeled "modsecuri.."):

After:

SVG files for the flamegraphs: flamegraphs.tar.gz

[zimmerle]

I have rebased it on top of v3/dev/3.1-experimental branch.

I have been unable to test it with my benchmark, because it crashes. Base v3/dev/3.1-experimental crashes as well, so it's not caused by the shared_ptr changes.

(gdb) bt
#0  0x00007ffff7c0cf16 in __dynamic_cast () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#1  0x00007ffff7ea7d6f in modsecurity::RuleWithActions::executeTransformations (this=this@entry=0x5555565eeb00, trans=trans@entry=0x555564ca3390, in="%2F", Python Exception <class 'AttributeError'> 'NoneType' object has no attribute 'pointer':
results=std::__cxx11::list) at rule_with_actions.cc:336
#2  0x00007ffff7ead9f4 in modsecurity::RuleWithOperator::evaluate (this=0x5555565eeb00, trans=0x555564ca3390) at rule_with_operator.cc:303
#3  0x00007ffff7e9bba4 in modsecurity::RulesSet::evaluate (this=0x5555555bc870, phase=phase@entry=3, t=t@entry=0x555564ca3390) at rules_set.cc:300
#4  0x00007ffff7e88303 in modsecurity::Transaction::processRequestBody (this=<optimized out>) at transaction.cc:985
#5  0x0000555555559a8c in process_transaction (it=0x7fffffffe080, modsecTransaction=0x555564ca3390, j=...) at benchmark.cc:103
#6  main (argc=<optimized out>, argv=<optimized out>) at benchmark.cc:194
(gdb) frame 1
#1  0x00007ffff7ea7d6f in modsecurity::RuleWithActions::executeTransformations (this=this@entry=0x5555565eeb00, trans=trans@entry=0x555564ca3390, in="%2F", results=std::__cxx11::list = {...}) at rule_with_actions.cc:336
336             if (dynamic_cast<actions::transformations::None *>(t)) {

@WGH- do you mind to test it again against the top of v3.1-experimental. If it still failing, can you share the set of rules that you are using? I am not able to reproduce it here.

[WGH-]

It seems that it introduces an issue regarding the RunTimeString execution.

I'm having the same crash in my benchmark in base v3.1-experimental. I suppose this patch is not at fault, it just shuffled something a bit so the underlying problem surfaces in the regression test.

I'll create a minimal reproduction, and report back to #2376

[zimmerle]

It seems that push_back was used together with std::make_shared on different places, those could be replaced with emplace_back. I am having it tested here.

[WGH-]

std::make_shared<T>(...) is more efficient than std::shared_ptr(new T(...)), since the former will do only one heap allocation instead of two, though.

[WGH-]

@zimmerle Did you already merge it into v3/dev/3.1-experimental? Or what happened?

[zimmerle]

@zimmerle Did you already merge it into v3/dev/3.1-experimental? Or what happened?

Yes. It is merged on v3.1-experimental. As shared pointer I have delayed the variable resolution to the next minute if needed.

Listed here-
https://github.com/SpiderLabs/ModSecurity/blob/abf59f4b84992195e9ba58c378d8ab4441f73b6b/headers/modsecurity/variable_value.h#L51-L86

I did not collected the numbers yet, trying to make the branch stable first.

Other change that I did, was this: cc5d3f5

[zimmerle]

I am going to close this. We can track the performance numbers in a separated issue, if needed.

Thank you @WGH- .

[WGH-]

It's just the state of this PR after you force-pushed some diverged branch utterly confused me. It's clear now, thanks,

[zimmerle]

R after you force-pushed some diverged branch utterly confused me. It's clear now, thanks,

v3.1-experimental is being rebased against v3/master from time to time, indeed it is confusing.