Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ast.all_refs #1076

Merged
merged 1 commit into from
Sep 9, 2024
Merged

Remove ast.all_refs #1076

merged 1 commit into from
Sep 9, 2024

Conversation

anderseknert
Copy link
Member

@anderseknert anderseknert commented Sep 8, 2024

This held a copy of ast.found.refs + imported refs Which was quite redundant. Rewrote all consumers to go for ast.found.refs directly, and imports where needed.

Also added util.intersects for a common pattern used in these rules.


to_set(x) := {y | some y in x} if not is_set(x)

intersects(s1, s2) if count(intersection({to_set(s1), to_set(s2)})) > 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was interested to see if how this compared with another implementation.

I thought that seeing as this was a filter of sorts, it might be best to make sure it's really fast.

I see the mean as 51819.98 and 38949.57 for the some in implementation. Might be worth testing with some other inputs too...

…/b9nzv3xj1s9g_dlpm2jgz43w0000gn/T/tmp.12Vm5MvG8y ➜ opa eval --data util1.rego --profile --format=pretty --count=100 'data.util.intersects({1,2,3}, {3, 2, 1})'
true
+------------------------------+--------+---------+-----------+--------------------+------------------------+
|            METRIC            |  MIN   |   MAX   |   MEAN    |        90%         |          99%           |
+------------------------------+--------+---------+-----------+--------------------+------------------------+
| timer_rego_load_files_ns     | 51542  | 1005250 | 85318.35  | 83600.6            | 1.0002008299999995e+06 |
| timer_rego_module_compile_ns | 278625 | 1980666 | 424951.22 | 646799.7           | 1.9739843399999994e+06 |
| timer_rego_module_parse_ns   | 36000  | 821417  | 63865.43  | 58970.8            | 818032.4099999997      |
| timer_rego_query_compile_ns  | 29958  | 202667  | 44930.95  | 61275.000000000015 | 202517                 |
| timer_rego_query_eval_ns     | 20250  | 955458  | 51819.98  | 33033.4            | 954033.8399999999      |
| timer_rego_query_parse_ns    | 18083  | 220042  | 31407.09  | 36583              | 219272.41999999993     |
+------------------------------+--------+---------+-----------+--------------------+------------------------+
+----------+-----------+----------+----------+-----------+----------+----------+--------------+--------------------------------+
|   MIN    |    MAX    |   MEAN   |   90%    |    99%    | NUM EVAL | NUM REDO | NUM GEN EXPR |            LOCATION            |
+----------+-----------+----------+----------+-----------+----------+----------+--------------+--------------------------------+
| 10.292µs | 916.375µs | 35.058µs | 17.161µs | 913.466µs | 1        | 1        | 1            | data.util.intersects({1,2,3},  |
|          |           |          |          |           |          |          |              | {3, 2, 1})                     |
| 3.958µs  | 88.585µs  | 7.427µs  | 9.486µs  | 87.99µs   | 5        | 5        | 5            | util1.rego:9                   |
| 2.583µs  | 87.333µs  | 4.805µs  | 5.786µs  | 86.531µs  | 2        | 1        | 1            | util1.rego:7                   |
| 1.54µs   | 22.125µs  | 3.112µs  | 3.911µs  | 21.966µs  | 1        | 1        | 1            | util1.rego:5                   |
+----------+-----------+----------+----------+-----------+----------+----------+--------------+--------------------------------+
…/b9nzv3xj1s9g_dlpm2jgz43w0000gn/T/tmp.12Vm5MvG8y ➜ opa eval --data util2.rego --profile --format=pretty --count=100 'data.util.intersects({1,2,3}, {3, 2, 1})'
true
+------------------------------+--------+---------+-----------+--------------------+------------------------+
|            METRIC            |  MIN   |   MAX   |   MEAN    |        90%         |          99%           |
+------------------------------+--------+---------+-----------+--------------------+------------------------+
| timer_rego_load_files_ns     | 51791  | 447875  | 88461.65  | 105012.2           | 446756.6699999999      |
| timer_rego_module_compile_ns | 263875 | 1220417 | 427077.15 | 767003.9           | 1.2185203299999998e+06 |
| timer_rego_module_parse_ns   | 35750  | 384542  | 61666.28  | 76420.6            | 382888.65999999986     |
| timer_rego_query_compile_ns  | 31500  | 261708  | 47220.47  | 52783.7            | 261298.41999999995     |
| timer_rego_query_eval_ns     | 19500  | 381083  | 38949.57  | 43463.100000000006 | 379686.3299999999      |
| timer_rego_query_parse_ns    | 19000  | 146750  | 32872.49  | 41996.1            | 146448.74999999997     |
+------------------------------+--------+---------+-----------+--------------------+------------------------+
+---------+-----------+----------+----------+-----------+----------+----------+--------------+--------------------------------+
|   MIN   |    MAX    |   MEAN   |   90%    |    99%    | NUM EVAL | NUM REDO | NUM GEN EXPR |            LOCATION            |
+---------+-----------+----------+----------+-----------+----------+----------+--------------+--------------------------------+
| 5.416µs | 348.667µs | 16.376µs | 16.749µs | 347.12µs  | 2        | 2        | 2            | util2.rego:10                  |
| 4.792µs | 104.291µs | 8.295µs  | 9.479µs  | 103.439µs | 1        | 1        | 1            | data.util.intersects({1,2,3},  |
|         |           |          |          |           |          |          |              | {3, 2, 1})                     |
| 3.792µs | 14.25µs   | 6.41µs   | 8.328µs  | 14.235µs  | 2        | 2        | 2            | util2.rego:11                  |
| 2.835µs | 9.833µs   | 5.095µs  | 7.007µs  | 9.824µs   | 1        | 1        | 1            | util2.rego:5                   |
| 2.083µs | 6.084µs   | 3.783µs  | 4.994µs  | 6.081µs   | 2        | 1        | 1            | util2.rego:7                   |
+---------+-----------+----------+----------+-----------+----------+----------+--------------+--------------------------------+
…/b9nzv3xj1s9g_dlpm2jgz43w0000gn/T/tmp.12Vm5MvG8y ➜ cat util1.rego
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: util1.rego
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ package util
   2   │
   3   │ import rego.v1
   4   │
   5   │ to_set(x) := x if is_set(x)
   6   │
   7   │ to_set(x) := {y | some y in x} if not is_set(x)
   8   │
   9   │ intersects(s1, s2) if count(intersection({to_set(s1), to_set(s2)})) > 0
───────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
ca…/b9nzv3xj1s9g_dlpm2jgz43w0000gn/T/tmp.12Vm5MvG8y ➜ cat util2.rego
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: util2.rego
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ package util
   2   │
   3   │ import rego.v1
   4   │
   5   │ to_set(x) := x if is_set(x)
   6   │
   7   │ to_set(x) := {y | some y in x} if not is_set(x)
   8   │
   9   │ intersects(s1, s2) if {
  10   │     some e in to_set(s1)
  11   │     e in to_set(s2)
  12   │ }
───────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! It's not used in any hot path today, but having the fastest possible implementation is of course good if it ends up there later. I'll have it fixed in a next PR. Thanks 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, lol, I thought this was merged! I'll update the PR now.

This held a copy of `ast.found.refs` + imported refs
Which was quite redundant. Rewrote all consumers to
go for `ast.found.refs` directly, and imports where
needed.

Also added `util.intesecets` for a common pattern used
in these rules.

Signed-off-by: Anders Eknert <anders@styra.com>
@anderseknert anderseknert merged commit 2e07303 into main Sep 9, 2024
4 checks passed
@anderseknert anderseknert deleted the no-ast-all-refs branch September 9, 2024 10:46
srenatus pushed a commit to srenatus/regal that referenced this pull request Oct 1, 2024
This held a copy of `ast.found.refs` + imported refs
Which was quite redundant. Rewrote all consumers to
go for `ast.found.refs` directly, and imports where
needed.

Also added `util.intesecets` for a common pattern used
in these rules.

Signed-off-by: Anders Eknert <anders@styra.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants