improve falco files loading performance #2151

VadimZy · 2022-08-03T04:34:05Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind design

/kind feature

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:
Improve rules loading/parsing performance
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

update(userspace/engine): improve falco files loading performance

jasondellaluce

Gave a first round of comments. This looks great, specially the evttype part! Can you remove the draft status, so that the CI and tests can run?

userspace/engine/filter_evttype_resolver.h

jasondellaluce · 2022-08-03T09:24:00Z

userspace/engine/filter_evttype_resolver.h

@@ -21,6 +21,124 @@ limitations under the License.
 #include <set>
 #include <memory>

+class falco_event_types


This is awesome!

userspace/engine/rule_loader.cpp

jasondellaluce · 2022-08-03T09:41:38Z

userspace/engine/rule_loader.cpp

@@ -347,8 +364,9 @@ static shared_ptr<ast::expr> parse_condition(
 	}
 	catch (const sinsp_exception& e)
 	{
-		throw falco_exception("Compilation error when compiling \""
-			+ condition + "\": " + to_string(p.get_pos().col) + ": " + e.what());
+		throw falco_exception("Compilation error when compiling \n"


Watch out for conflicts with #2098.

This is not solved, the changes of #2098 introduced a new form of error reposting. We should throw a rule_loader::rule_load_exception with proper err code here.

mstemm · 2022-08-08T20:14:05Z

Should we bother changing any of the json_evt code at all, given that it's effectively dead with the transition of k8s audit support to a plugin?

VadimZy · 2022-08-10T22:21:35Z

Should we bother changing any of the json_evt code at all, given that it's effectively dead with the transition of k8s audit support to a plugin?

it will exist in the code for a while, sysdig might use it for some time as well.

jasondellaluce · 2022-08-11T08:02:59Z

userspace/engine/rule_loader.cpp

@@ -347,8 +364,9 @@ static shared_ptr<ast::expr> parse_condition(
 	}
 	catch (const sinsp_exception& e)
 	{
-		throw falco_exception("Compilation error when compiling \""
-			+ condition + "\": " + to_string(p.get_pos().col) + ": " + e.what());
+		throw falco_exception("Compilation error when compiling \n"


This is not solved, the changes of #2098 introduced a new form of error reposting. We should throw a rule_loader::rule_load_exception with proper err code here.

jasondellaluce · 2022-08-11T08:11:09Z

userspace/engine/rule_loader.cpp

@@ -21,7 +21,12 @@ limitations under the License.
 #include "filter_evttype_resolver.h"
 #include "filter_warning_resolver.h"
 #include <version.h>
-#include <sstream>
+
+#ifndef _WIN32


This is not enough and that's also the reason why the CI builds are failing. We have a very weird setup in the libraries for regex support that depends on:

old compiler versions, that don't support <regex>

inclusion or not of oniguruma in the build, which is a dependency of jq

To solve all the combinations, this trick worked for me:

https://github.com/falcosecurity/libs/blob/d08daff1bf69c2388b7474c1fed96bd6e98b900f/userspace/libsinsp/filter/parser.cpp#L35

https://github.com/falcosecurity/libs/blob/d08daff1bf69c2388b7474c1fed96bd6e98b900f/userspace/libsinsp/CMakeLists.txt#L272 (I think you should be able to use the USE_BUNDLED_ONIGURUMA definition from libsinsp's cmake file)

Mind that this would also cause the regex standard used in the code to differ, depending on what regex library we are building with. So you need to write two copies of each regex, and also handle them in two different ways:

https://github.com/falcosecurity/libs/blob/d08daff1bf69c2388b7474c1fed96bd6e98b900f/userspace/libsinsp/filter/parser.cpp#L46

https://github.com/falcosecurity/libs/blob/d08daff1bf69c2388b7474c1fed96bd6e98b900f/userspace/libsinsp/filter/parser.cpp#L532

Good news is that the CI and the tests won't pass until this is addressed: we have different build types that help us test all the cases.

Bad news is that this is truly terrible, and I really hope we'll be able to do better than this as soon as possible. The bottleneck is the presence of oniguruma brought in by jq, we we manage to remove that we should be able to just stick to one regex standard (posix I guess).

dropping fix for the list inserter altogether - I don't have resources to fight regex portability here.
@jasondellaluce keep in mind that the current list expansion routine is quite inefficient - it keeps on looking for all available lists (100s) in the continuously growing string and does it from the string beginning each time.

Unfortunately, that's what we had in the past with Lua as well. This part will need to be substituted with proper AST traversal+substitutions in the future, however that would introduce ruleset breaking changes so it's something that we'll tackle in the future.

userspace/engine/rule_loader.cpp

poiana · 2022-08-22T07:50:49Z

LGTM label has been added.

Git tree hash: ecf2d1a1e3036d7189b2907fc8bffbfa36438a8b

jasondellaluce · 2022-08-22T07:51:08Z

/milestone 0.33.0

jasondellaluce · 2022-08-26T12:26:05Z

@VadimZy now we introduced RE2. Let me know if you intend to re-introduce your list_interter now that portable regexs are available. If not, this still needs to be rebased on top of master.

…rule sets - replace std::set<uint16_t> with fixed size vector in event types propagation - rework lists expansion by replacing repetitive string::find in constantly growing expansion string with regex tokenization - improve json_event parsing by moving const initializations into static routines Signed-off-by: VadimZy <vadim.zyarko@sysdig.com>

Signed-off-by: VadimZy <vadim.zyarko@sysdig.com>

reverting to the inefficient code. Signed-off-by: VadimZy <vadim.zyarko@sysdig.com>

jasondellaluce

I rebased this on master for you. LGTM, thanks!

/approved

poiana · 2022-09-05T14:59:41Z

LGTM label has been added.

Git tree hash: 074920724c1a8793867fd771a65745ee5e6bfda1

poiana · 2022-09-05T15:05:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasondellaluce, leogr, VadimZy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jasondellaluce,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

userspace/engine/filter_evttype_resolver.h

+
+	void for_each(std::function<bool(uint16_t)> consumer) const
+	{
+		for(uint16_t i = 0; i < m_types.size(); ++i)


poiana added do-not-merge/work-in-progress dco-signoff: no do-not-merge/release-note-label-needed labels Aug 3, 2022

poiana requested review from Kaizhe and mfdii August 3, 2022 04:34

poiana added the size/XXL label Aug 3, 2022

jasondellaluce reviewed Aug 3, 2022

View reviewed changes

VadimZy force-pushed the perf-improve branch from b374d0f to 5d01e5a Compare August 10, 2022 22:13

poiana added dco-signoff: yes and removed dco-signoff: no labels Aug 10, 2022

VadimZy marked this pull request as ready for review August 10, 2022 22:15

poiana removed the do-not-merge/work-in-progress label Aug 10, 2022

jasondellaluce reviewed Aug 11, 2022

View reviewed changes

poiana added size/XL and removed size/XXL labels Aug 11, 2022

jasondellaluce previously approved these changes Aug 22, 2022

View reviewed changes

poiana assigned jasondellaluce Aug 22, 2022

poiana added the lgtm label Aug 22, 2022

poiana added this to the 0.33.0 milestone Aug 22, 2022

poiana added release-note-none and removed do-not-merge/release-note-label-needed labels Aug 25, 2022

VadimZy added 4 commits September 5, 2022 10:37

fix tests, remove dead code

62579f7

Signed-off-by: VadimZy <vadim.zyarko@sysdig.com>

remove sinsp.h public dependencies

cef804a

Signed-off-by: VadimZy <vadim.zyarko@sysdig.com>

use <onigposix.h> instead of <regex.h>

96249f5

Signed-off-by: VadimZy <vadim.zyarko@sysdig.com>

dropping fix for list parsing due to the absence of regex portability.

9bbc564

reverting to the inefficient code. Signed-off-by: VadimZy <vadim.zyarko@sysdig.com>

jasondellaluce dismissed their stale review via 9bbc564 September 5, 2022 14:57

jasondellaluce force-pushed the perf-improve branch from 1dd9c5d to 9bbc564 Compare September 5, 2022 14:57

poiana removed the lgtm label Sep 5, 2022

poiana requested a review from jasondellaluce September 5, 2022 14:57

jasondellaluce approved these changes Sep 5, 2022

View reviewed changes

poiana added the lgtm label Sep 5, 2022

poiana added the approved label Sep 5, 2022

leogr approved these changes Sep 5, 2022

View reviewed changes

poiana assigned leogr Sep 5, 2022

github-advanced-security bot found potential problems Sep 5, 2022

View reviewed changes

poiana merged commit af95455 into falcosecurity:master Sep 5, 2022

jasondellaluce mentioned this pull request Sep 9, 2022

update: support running multiple event sources in parallel #2182

Merged

poiana added release-note and removed release-note-none labels Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve falco files loading performance #2151

improve falco files loading performance #2151

VadimZy commented Aug 3, 2022 •

edited by jasondellaluce

Loading

jasondellaluce left a comment

jasondellaluce Aug 3, 2022

jasondellaluce Aug 3, 2022

VadimZy Aug 10, 2022

jasondellaluce Aug 11, 2022

mstemm commented Aug 8, 2022

VadimZy commented Aug 10, 2022

jasondellaluce Aug 11, 2022

jasondellaluce Aug 11, 2022

VadimZy Aug 11, 2022

jasondellaluce Aug 22, 2022

poiana commented Aug 22, 2022

jasondellaluce commented Aug 22, 2022

jasondellaluce commented Aug 26, 2022

jasondellaluce left a comment

poiana commented Sep 5, 2022

poiana commented Sep 5, 2022

improve falco files loading performance #2151

improve falco files loading performance #2151

Conversation

VadimZy commented Aug 3, 2022 • edited by jasondellaluce Loading

jasondellaluce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstemm commented Aug 8, 2022

VadimZy commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiana commented Aug 22, 2022

jasondellaluce commented Aug 22, 2022

jasondellaluce commented Aug 26, 2022

jasondellaluce left a comment

Choose a reason for hiding this comment

poiana commented Sep 5, 2022

poiana commented Sep 5, 2022

VadimZy commented Aug 3, 2022 •

edited by jasondellaluce

Loading