Optimize ACL Parser Performance by Conditional Application of Recursive Analysis

[kevinmichaelbowersox]

Problem

When the ACL SETUSER command is invoked with a large number of commands in the ACL (15+) the Garnet Server spins for a long time (1-2+ min) and can exceed the default timeout settings of a client in some cases.

The degraded runtime performance can be attributed to the RationalizeACLDescription method which unconditionally applies a recursive algorithm to eliminate duplicate commands in the ACL description returned to the user. This method has a high time complexity (n^3 according to copilot) and performance of the method degrades quickly when there are a large number of ACLs. Comments surrounding the method also indicate that it is expensive. In some test cases, execution of the method can take minutes.

Fix

This change applies an optimization that avoids the expensive recursive analysis/rationalization when it is unnecessary, significantly improving the performance of the ACL parser.

The logic works on the following premises:

• When the parser is processing an ACL it is moving from left to right.
• Each time a new command is parsed, the existing CommandPermissionSet contains all commands the user has permission to execute.
• If the existing CommandPermissionSet does not grant access to execute the current command to be added, there is no redundancy in the command description that needs to be reduced.
• Because there is no redundancy between the new command to add and the existing command set we can forgo the expensive RationalizeACLDescription method.

Testing Approach

To validate the change, I took the following steps:

• Wrote unit tests to produce the performance problem and tested it against current/historical branches.
• Create more advanced unit tests to capture the current behavior of the parsers and to avoid introducing any obvious regressions. These are not all encompassing but I think they cover a large amount of scenarios.
• Some test cases I created had issues with rationalization of the command description. I considered these out of scope for the optimization PR. However, I included them to capture the problem (explicit to prevent running in CI).

Performance

I captured before / after results of my test executions in Visual Studio. Here are the results. I believe the first test suffers from a cold start problem, however there are significant improvements on the long running tests.

ParseACLRuleDescriptionTest

Name	Before	After	Improvement (%)
1-command	354ms	164ms	53.67%
2-command	3ms	1ms	66.67%
3-command-duplicates-reduce	1ms	1ms	0.00%
4-command-duplicates-complicated	2ms	2ms	0.00%
5-command-duplicates-complicated	1ms	1ms	0.00%
6-category	3ms	2ms	33.33%
7-category-reduces	2ms	1ms	50.00%
8-category-reduces	1ms	1ms	0.00%
9-category-reduces	1ms	1ms	0.00%
10-category-reduces	1ms	1ms	0.00%
11-category-command-reduces	1ms	1ms	0.00%
12-category-command-reduces	1ms	1ms	0.00%
13-category-command-reduces	1ms	1ms	0.00%
14-category-command-reduces	1ms	1ms	0.00%
15-category-command-reduces	1ms	1ms	0.00%
16-category-command-reduces	1ms	1ms	0.00%
17-category-command-reduces	1ms	1ms	0.00%
18-category-command-reduces	2ms	1ms	50.00%
19-category-command-reduces	1ms	1ms	0.00%
20-category-command-reduces	1ms	1ms	0.00%
21-category-command-reduces	1ms	1ms	0.00%
22-category-command-reduces	98ms	1ms	98.98%
23-category-command-reduces	125ms	1ms	99.20%
24-category-command-reduces	2ms	1ms	50.00%
25-multi-category-reduces	1ms	1ms	0.00%
26-multi-category-reduces	1ms	1ms	0.00%
27-multi-category-reduces	1ms	1ms	0.00%
28-multi-category-reduces	6ms	1ms	83.33%

ParseACLRuleDescriptionTimeoutsTest

Name	Before	After	Improvement (%)
1-command-notimeout	2.4s	1ms	99.96%
2-category-command-notimeout	1.3s	1ms	99.92%
3-category-command-notimeout	2.5min	1ms	99.9993%
4-category-command-notimeout	24s	1ms	99.996%
5-category-command-notimeout	21.2s	1ms	99.99%

Follow On Work

The approach taken to producing the ACL description should be evaluated to see if a simpler strategy can be used. For example, a strategy could be used that only reduces at the same level, meaning we would not reduce individual commands when their category is specified.

Fixes #1225

[Copilot]

Pull Request Overview

This PR optimizes ACL parser performance by conditionally applying deep recursive analysis only when needed.

• Introduces a boolean flag (useDeepRationalization) in ACL update methods to determine whether to perform the expensive recursive rationalization.
• Updates the RationalizeACLDescription method signature and replaces the unconditional recursion loop with a conditional one.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
test/Garnet.test/Resp/ACL/AclParserTests.cs	Added new test cases to verify correct ACL description outputs and performance improvements.
libs/server/ACL/User.cs	Modified ACL update methods to incorporate conditional deep rationalization and updated the RationalizeACLDescription accordingly.

[kevin-montrose]

Longer term it's probably worth just reworking this algorithm, but a small patch to fix excessive runtimes is fine for now.