topdown: Adding cap to caches for regex and glob built-in functions #6846

johanfylling · 2024-07-03T14:49:57Z

Fixing possible memory leak where caches grow uncontrollably when large amounts of regexes or globs are generated or originate from the input document.

Fixes: #6828

Signed-off-by: Johan Fylling <[email protected]>

netlify · 2024-07-03T14:51:19Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`c794780`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/6687b6a2d22893000899df51
😎 Deploy Preview	https://deploy-preview-6846--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

johanfylling · 2024-07-03T14:54:16Z

wasm/src/glob.cc

@@ -101,6 +103,16 @@ opa_value *opa_glob_match(opa_value *pattern, opa_value *delimiters, opa_value *
            return NULL;
        }

+		if (c->size() >= MAX_CACHE_SIZE)


I still need to add tests for this and the regex built-in.

Resolved, isn't it?

Signed-off-by: Johan Fylling <[email protected]>

johanfylling · 2024-07-03T15:15:37Z

topdown/regex_bench_test.go

goos: darwin goarch: arm64 pkg: github.com/open-policy-agent/opa/topdown │ regex_main.txt │ regex_pr.txt │ │ sec/op │ sec/op vs base │ BuiltinRegexMatch/reuse-pattern=true,_pattern-count=10-10 2.216µ ± 1% 2.213µ ± 2% ~ (p=0.838 n=10) BuiltinRegexMatch/reuse-pattern=true,_pattern-count=100-10 12.86µ ± 1% 12.97µ ± 3% ~ (p=0.218 n=10) BuiltinRegexMatch/reuse-pattern=true,_pattern-count=1000-10 117.7µ ± 0% 117.8µ ± 1% ~ (p=0.579 n=10) BuiltinRegexMatch/reuse-pattern=false,_pattern-count=10-10 12.26µ ± 1% 12.32µ ± 5% ~ (p=0.123 n=10) BuiltinRegexMatch/reuse-pattern=false,_pattern-count=100-10 148.3µ ± 0% 148.3µ ± 0% ~ (p=0.353 n=10) BuiltinRegexMatch/reuse-pattern=false,_pattern-count=1000-10 1.712m ± 0% 1.642m ± 0% -4.08% (p=0.000 n=10) BuiltinRegexMatchAsync/reuse-pattern=true,_clients=100,_pattern-count=10-10 215.1µ ± 2% 217.1µ ± 3% ~ (p=0.684 n=10) BuiltinRegexMatchAsync/reuse-pattern=true,_clients=100,_pattern-count=100-10 1.787m ± 1% 1.776m ± 1% -0.60% (p=0.043 n=10) BuiltinRegexMatchAsync/reuse-pattern=true,_clients=100,_pattern-count=1000-10 17.35m ± 0% 17.37m ± 0% ~ (p=0.579 n=10) BuiltinRegexMatchAsync/reuse-pattern=true,_clients=200,_pattern-count=10-10 441.7µ ± 1% 439.9µ ± 1% ~ (p=0.353 n=10) BuiltinRegexMatchAsync/reuse-pattern=true,_clients=200,_pattern-count=100-10 3.596m ± 1% 3.575m ± 1% -0.59% (p=0.009 n=10) BuiltinRegexMatchAsync/reuse-pattern=true,_clients=200,_pattern-count=1000-10 35.07m ± 0% 34.81m ± 1% ~ (p=0.063 n=10) BuiltinRegexMatchAsync/reuse-pattern=false,_clients=100,_pattern-count=10-10 2.450m ± 1% 2.199m ± 0% -10.24% (p=0.000 n=10) BuiltinRegexMatchAsync/reuse-pattern=false,_clients=100,_pattern-count=100-10 26.02m ± 1% 22.58m ± 1% -13.24% (p=0.000 n=10) BuiltinRegexMatchAsync/reuse-pattern=false,_clients=100,_pattern-count=1000-10 266.0m ± 1% 256.9m ± 1% -3.43% (p=0.000 n=10) BuiltinRegexMatchAsync/reuse-pattern=false,_clients=200,_pattern-count=10-10 5.665m ± 1% 4.679m ± 1% -17.41% (p=0.000 n=10) BuiltinRegexMatchAsync/reuse-pattern=false,_clients=200,_pattern-count=100-10 54.16m ± 2% 49.35m ± 1% -8.87% (p=0.000 n=10) BuiltinRegexMatchAsync/reuse-pattern=false,_clients=200,_pattern-count=1000-10 538.1m ± 1% 544.5m ± 1% ~ (p=0.105 n=10) geomean 1.676m 1.621m -3.28%

srenatus · 2024-07-03T15:36:21Z

topdown/glob.go

@@ -64,6 +66,13 @@ func globCompileAndMatch(id, pattern, match string, delimiters []rune) (bool, er
 		if p, err = glob.Compile(pattern, delimiters...); err != nil {
 			return false, err
 		}
+		if len(globCache) >= globCacheMaxSize {
+			// Delete a (semi-)random key to make room for the new one.


why semi? I think it's pretty random 😅

It's not explicitly random, as the randomness is an implementation detail of the map.
I also think that it'd be deterministic over multiple runs with the same map entries.

So we're checking len(globCache) >= globCacheMaxSize but we only remove one. This is the only code path where cached patterns are inserted, right? So it cannot happen that through some other code path, the cache would still continue to grow without bounds because we're only removing one element here.

Correct, discounting tests, this is the only place we touch the cache.

Signed-off-by: Johan Fylling <[email protected]>

srenatus

LGTM. I'm glad this is getting fixed 🥳

srenatus · 2024-07-05T09:11:01Z

topdown/glob.go

@@ -64,6 +66,13 @@ func globCompileAndMatch(id, pattern, match string, delimiters []rune) (bool, er
 		if p, err = glob.Compile(pattern, delimiters...); err != nil {
 			return false, err
 		}
+		if len(globCache) >= globCacheMaxSize {
+			// Delete a (semi-)random key to make room for the new one.


So we're checking len(globCache) >= globCacheMaxSize but we only remove one. This is the only code path where cached patterns are inserted, right? So it cannot happen that through some other code path, the cache would still continue to grow without bounds because we're only removing one element here.

srenatus · 2024-07-05T09:12:18Z

wasm/src/glob.cc

@@ -101,6 +103,16 @@ opa_value *opa_glob_match(opa_value *pattern, opa_value *delimiters, opa_value *
            return NULL;
        }

+		if (c->size() >= MAX_CACHE_SIZE)


Resolved, isn't it?

johanfylling added 4 commits July 2, 2024 14:45

Adding bench tests for regex built-in

c42020f

Signed-off-by: Johan Fylling <[email protected]>

Capping built-in regex cache to 100 entries

2f98658

Signed-off-by: Johan Fylling <[email protected]>

Capping built-in glob cache to 100 entries

3fd3fd5

Signed-off-by: Johan Fylling <[email protected]>

Capping Wasm built-in regex and glob caches to 100 entries

6cacf17

Signed-off-by: Johan Fylling <[email protected]>

johanfylling commented Jul 3, 2024

View reviewed changes

Making linter happy

f8075c8

Signed-off-by: Johan Fylling <[email protected]>

johanfylling commented Jul 3, 2024

View reviewed changes

srenatus reviewed Jul 3, 2024

View reviewed changes

Adding tests

528d32a

Signed-off-by: Johan Fylling <[email protected]>

johanfylling marked this pull request as ready for review July 4, 2024 14:58

Merge branch 'main' into fix/regex_mem_leak

c794780

srenatus approved these changes Jul 5, 2024

View reviewed changes

johanfylling merged commit 34349c5 into open-policy-agent:main Jul 5, 2024
28 checks passed

johanfylling deleted the fix/regex_mem_leak branch July 5, 2024 09:18

amirsalarsafaei mentioned this pull request Aug 2, 2024

Performance Degrade since v0.67.0 on glob.match with lots of patterns #6908

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

topdown: Adding cap to caches for regex and glob built-in functions #6846

topdown: Adding cap to caches for regex and glob built-in functions #6846

johanfylling commented Jul 3, 2024

netlify bot commented Jul 3, 2024 •

edited

Loading

johanfylling Jul 3, 2024

srenatus Jul 5, 2024

johanfylling Jul 3, 2024

srenatus Jul 3, 2024

johanfylling Jul 4, 2024

srenatus Jul 5, 2024

johanfylling Jul 5, 2024

srenatus left a comment

srenatus Jul 5, 2024

srenatus Jul 5, 2024

topdown: Adding cap to caches for regex and glob built-in functions #6846

topdown: Adding cap to caches for regex and glob built-in functions #6846

Conversation

johanfylling commented Jul 3, 2024

netlify bot commented Jul 3, 2024 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jul 3, 2024 •

edited

Loading