Optimize trait loop evaluation by vitorenesduarte · Pull Request #19097 · gravitational/teleport

vitorenesduarte · 2022-12-06T12:08:24Z

This PR reduces the number of log.Debugs in the trait loop evaluation in case of a mismatch.

This PR also optimizes the trait eval loop ensuring that each regexp is compiled at-most once, and only if strictly needed.

Testing

None, besides the existing/updated unit tests.

Benchmark

# master branch
go test -run='^$' -bench=^BenchmarkTraitToRoles$ github.com/gravitational/teleport/lib/services -count=5 > master_bench
go test -v -run='^$' -bench=^BenchmarkTraitToRoles$ github.com/gravitational/teleport/lib/services -count=5 > master_bench_verbose

# this PR
go test -run='^$' -bench=^BenchmarkTraitToRoles$ github.com/gravitational/teleport/lib/services -count=5 > pr_bench
go test -v -run='^$' -bench=^BenchmarkTraitToRoles$ github.com/gravitational/teleport/lib/services -count=5 > pr_bench_verbose

go test:

❯ benchstat master_bench pr_bench
name                                                                                    old time/op  new time/op  delta
TraitToRoles/no_mappings_no_match-10                                                    2.99ns ± 0%  3.05ns ± 0%   +2.13%  (p=0.008 n=5+5)
TraitToRoles/simple_mappings_no_match-10                                                64.7ns ± 0%  64.6ns ± 0%     ~     (p=0.841 n=5+5)
TraitToRoles/simple_mappings_no_value_match-10                                          11.3µs ± 0%  12.0µs ± 0%   +6.36%  (p=0.008 n=5+5)
TraitToRoles/simple_mappings_direct_admin_value_match-10                                18.7µs ± 0%  12.7µs ± 0%  -32.43%  (p=0.008 n=5+5)
TraitToRoles/simple_mappings_direct_user_value_match-10                                 11.7µs ± 1%  11.6µs ± 0%     ~     (p=0.095 n=5+5)
TraitToRoles/simple_mappings_direct_user_value_match_with_array-10                      11.7µs ± 0%  11.6µs ± 0%   -1.11%  (p=0.008 n=5+5)
TraitToRoles/regexp_mappings_match_no_match-10                                          33.4ns ± 0%  33.4ns ± 0%     ~     (p=0.667 n=5+4)
TraitToRoles/regexp_mappings_match_no_match_-_subprefix-10                              6.70µs ± 0%  6.99µs ± 0%   +4.36%  (p=0.008 n=5+5)
TraitToRoles/regexp_mappings_match_value_with_capture_match-10                          16.1µs ± 0%   8.7µs ± 0%  -46.22%  (p=0.008 n=5+5)
TraitToRoles/regexp_mappings_match_multiple_value_with_capture_match,_deduplication-10  32.2µs ± 0%  10.2µs ± 0%  -68.22%  (p=0.008 n=5+5)
TraitToRoles/regexp_mappings_match_first_matches,_second_does_not-10                    23.3µs ± 1%  12.1µs ± 0%  -48.22%  (p=0.008 n=5+5)
TraitToRoles/regexp_compilation_invalid_regexp-10                                       36.9µs ± 1%  20.2µs ± 0%  -45.33%  (p=0.008 n=5+5)
TraitToRoles/regexp_compilation_regexp_are_not_compiled_if_not_needed-10                28.6ns ± 0%  28.6ns ± 0%     ~     (p=0.571 n=4+5)
TraitToRoles/empty_expands_are_skipped_value_with_capture_match-10                      12.2µs ± 0%   8.2µs ± 1%  -32.93%  (p=0.008 n=5+5)
TraitToRoles/glob_wildcard_match_empty_value_match-10                                   5.29µs ± 0%  5.07µs ± 0%   -4.14%  (p=0.008 n=5+5)
TraitToRoles/glob_wildcard_match_any_value_match-10                                     5.47µs ± 1%  5.20µs ± 0%   -4.85%  (p=0.008 n=5+5)
TraitToRoles/Whitespace/dashes_Matches_multiple_groups-10                                102µs ± 1%    61µs ± 1%  -40.06%  (p=0.008 n=5+5)
TraitToRoles/Whitespace/dashes_Matches_one_group-10                                     42.1µs ± 0%  42.3µs ± 0%   +0.53%  (p=0.008 n=5+5)
TraitToRoles/Whitespace/dashes_Matches_one_group_with_multiple_roles-10                 57.5µs ± 1%  44.6µs ± 0%  -22.39%  (p=0.008 n=5+5)
TraitToRoles/Whitespace/dashes_No_match_only_due_to_case-sensitivity-10                 45.3µs ± 1%  45.2µs ± 1%     ~     (p=0.690 n=5+5)

go test -v:

❯ benchstat master_bench_verbose pr_bench_verbose
name                                                                                    old time/op  new time/op  delta
TraitToRoles/no_mappings_no_match-10                                                    2.99ns ± 0%  3.05ns ± 0%   +2.11%  (p=0.008 n=5+5)
TraitToRoles/simple_mappings_no_match-10                                                64.6ns ± 0%  64.7ns ± 0%     ~     (p=0.429 n=5+5)
TraitToRoles/simple_mappings_no_value_match-10                                          34.3µs ± 2%  35.2µs ± 4%     ~     (p=0.056 n=5+5)
TraitToRoles/simple_mappings_direct_admin_value_match-10                                31.6µs ± 1%  25.4µs ± 1%  -19.81%  (p=0.008 n=5+5)
TraitToRoles/simple_mappings_direct_user_value_match-10                                 23.9µs ± 1%  24.2µs ± 2%   +1.19%  (p=0.016 n=5+5)
TraitToRoles/simple_mappings_direct_user_value_match_with_array-10                      23.9µs ± 2%  24.1µs ± 0%     ~     (p=0.222 n=5+5)
TraitToRoles/regexp_mappings_match_no_match-10                                          33.7ns ± 1%  33.4ns ± 0%     ~     (p=0.190 n=5+4)
TraitToRoles/regexp_mappings_match_no_match_-_subprefix-10                              19.1µs ± 0%  19.4µs ± 1%   +1.52%  (p=0.016 n=4+5)
TraitToRoles/regexp_mappings_match_value_with_capture_match-10                          15.9µs ± 1%   8.6µs ± 0%  -45.93%  (p=0.008 n=5+5)
TraitToRoles/regexp_mappings_match_multiple_value_with_capture_match,_deduplication-10  32.0µs ± 0%  10.2µs ± 0%  -68.04%  (p=0.008 n=5+5)
TraitToRoles/regexp_mappings_match_first_matches,_second_does_not-10                    36.9µs ± 1%  24.7µs ± 1%  -33.04%  (p=0.008 n=5+5)
TraitToRoles/regexp_compilation_invalid_regexp-10                                       50.9µs ± 1%  32.8µs ± 2%  -35.46%  (p=0.008 n=5+5)
TraitToRoles/regexp_compilation_regexp_are_not_compiled_if_not_needed-10                28.6ns ± 0%  28.6ns ± 0%     ~     (p=0.143 n=5+4)
TraitToRoles/empty_expands_are_skipped_value_with_capture_match-10                      12.1µs ± 1%   8.1µs ± 0%  -33.23%  (p=0.008 n=5+5)
TraitToRoles/glob_wildcard_match_empty_value_match-10                                   5.28µs ± 0%  5.05µs ± 0%   -4.38%  (p=0.008 n=5+5)
TraitToRoles/glob_wildcard_match_any_value_match-10                                     5.47µs ± 1%  5.19µs ± 0%   -5.06%  (p=0.008 n=5+5)
TraitToRoles/Whitespace/dashes_Matches_multiple_groups-10                                180µs ± 2%   111µs ± 1%  -38.22%  (p=0.008 n=5+5)
TraitToRoles/Whitespace/dashes_Matches_one_group-10                                     79.6µs ± 2%  79.4µs ± 1%     ~     (p=1.000 n=5+5)
TraitToRoles/Whitespace/dashes_Matches_one_group_with_multiple_roles-10                 96.6µs ± 1%  81.1µs ± 1%  -16.03%  (p=0.008 n=5+5)
TraitToRoles/Whitespace/dashes_No_match_only_due_to_case-sensitivity-10                 83.7µs ± 2%  82.4µs ± 1%     ~     (p=0.095 n=5+5)

zmb3 · 2022-12-06T13:56:11Z

Might be nice to add a benchmark here as well, which would show how much of an improvement this change makes, as well as make it easier to detect performance regressions in the future.

codingllama · 2022-12-06T17:21:40Z

Might be nice to add a benchmark here as well, which would show how much of an improvement this change makes, as well as make it easier to detect performance regressions in the future.

+1 , let's get some numbers in. It's hard to assess how well this works just by reading it.

r0mant · 2022-12-07T00:29:35Z

+
+			// warn at most maxMismatchedTraitValuesWarned trait values to prevent huge warning lines
+			if len(mismatched) > maxMismatchedTraitValuesWarned {
+				warnings = append(warnings, fmt.Sprintf(


I know I suggested adding this to the returned warnings myself but now that I'm thinking about it again I wonder if we'll only be increasing amount of log noise this way - because these returned warnings are logged at "warn" level on the caller's side. I think we should just log these "mismatched" traits locally in this method and keep it at debug.

I had similar thoughts here. +1.

Done in 15df538.

Co-authored-by: Alan Parra <alan.parra@goteleport.com>

codingllama

Code looks reasonable. @vitorenesduarte, could you provide some measurements and maybe add a Go benchmark test, so we have data behind the PR? Other than that, happy to stamp it.

zmb3 · 2022-12-07T15:00:55Z

 }

+
+/*


There's no need to include this comment in the source code, but it would be nice to see a before/after in the PR description. You can use golang.org/x/perf/cmd/benchstat to generate a nice diff.

I saw results like these in other benchmarks. Removed in 7eaa6ea.

Thanks for suggesting benchstat!!

Numbers look great, thanks!

vitorenesduarte · 2022-12-07T15:41:49Z

@zmb3 @codingllama ce86dc0 adds a benchmark using the test cases we already had for TestOIDCMapping (this made the PR harder to review though).

I ran the benchmark both with go test and go test -v to try to also account for the log.Debug calls.
The PR description now contains some benchmark results.

codingllama · 2022-12-07T19:38:52Z

 }

+
+/*


nit: Remove the comment with benchmark results - it's better to run it again if necessary.

Done in 7eaa6ea.

codingllama · 2022-12-07T19:39:23Z

@zmb3 @codingllama ce86dc0 adds a benchmark using the test cases we already had for TestOIDCMapping (this made the PR harder to review though).

I ran the benchmark both with go test and go test -v to try to also account for the log.Debug calls. The PR description now contains some benchmark results.

Many thanks! LGTM.

github-actions · 2022-12-07T22:35:53Z

@vitorenesduarte See the table below for backport results.

Branch	Result
branch/v10	Failed
branch/v11	Create PR
branch/v9	Failed

This commit reduces the number of `log.Debug`s in the trait loop evaluation in case of a mismatch. This commit also optimizes the trait eval loop ensuring that each regexp is compiled at-most once, and only if strictly needed.

* Refactor tests under services package. Refactored all tests under "lib/services/suite" to use testify instead of gocheck. * Optimize trait loop evaluation (#19097) This commit reduces the number of `log.Debug`s in the trait loop evaluation in case of a mismatch. This commit also optimizes the trait eval loop ensuring that each regexp is compiled at-most once, and only if strictly needed. Co-authored-by: Russell Jones <rjones@goteleport.com>

This commit reduces the number of `log.Debug`s in the trait loop evaluation in case of a mismatch. This commit also optimizes the trait eval loop ensuring that each regexp is compiled at-most once, and only if strictly needed.

* Refactor tests under services package. Refactored all tests under "lib/services/suite" to use testify instead of gocheck. * Optimize trait loop evaluation (#19097) This commit reduces the number of `log.Debug`s in the trait loop evaluation in case of a mismatch. This commit also optimizes the trait eval loop ensuring that each regexp is compiled at-most once, and only if strictly needed. Co-authored-by: Russell Jones <rjones@goteleport.com>

Vitor Enes added 9 commits December 2, 2022 13:42

Compile regexp once

132ceef

Use FindStringIndex instead of FindAllStringIndex

449f352

Replace log.Debug line with a single warning

1279556

Improve warning output

d02466c

Add invalid regexp test case

511a52c

Add array trimming in warning test

5f38379

Improve docs

af4b809

Avoid compiling trait mapping values if no traits

8ec52b1

Ensure regexps are compiled at-most once, and only if strictly needed

fde2482

vitorenesduarte self-assigned this Dec 6, 2022

github-actions Bot requested review from GavinFrazar and codingllama December 6, 2022 12:08

vitorenesduarte added backport/branch/v9 labels Dec 6, 2022

vitorenesduarte changed the title ~~Optimize trait loop evalution~~ Optimize trait loop evaluation Dec 6, 2022

Vitor Enes added 2 commits December 6, 2022 12:13

Merge branch 'master' into vitor/traits-to-roles-regexp-precompile

2a7d021

gofmt

c677beb

codingllama reviewed Dec 6, 2022

View reviewed changes

Comment thread lib/services/user_test.go Outdated

Comment thread lib/services/user_test.go Outdated

Comment thread lib/services/user_test.go Outdated

Comment thread lib/services/traits.go Outdated

Comment thread lib/services/traits.go Outdated

r0mant reviewed Dec 7, 2022

View reviewed changes

Vitor Enes and others added 4 commits December 7, 2022 09:04

Improve test readability.

8a231c7

Co-authored-by: Alan Parra <alan.parra@goteleport.com>

log.Debug mismatch warnings

15df538

Use WithField to log mismatched trait values

5958806

Merge branch 'master' into vitor/traits-to-roles-regexp-precompile

10f6ad7

codingllama reviewed Dec 7, 2022

View reviewed changes

Vitor Enes added 2 commits December 7, 2022 14:27

Turn TestOIDCMapping test cases into a benchmark

ce86dc0

Merge branch 'master' into vitor/traits-to-roles-regexp-precompile

56ef006

zmb3 reviewed Dec 7, 2022

View reviewed changes

Remove bench results

7eaa6ea

Merge branch 'master' into vitor/traits-to-roles-regexp-precompile

74dee58

vitorenesduarte requested review from codingllama, r0mant and zmb3 December 7, 2022 15:43

zmb3 approved these changes Dec 7, 2022

View reviewed changes

vitorenesduarte enabled auto-merge (squash) December 7, 2022 15:51

codingllama approved these changes Dec 7, 2022

View reviewed changes

github-actions Bot removed the request for review from GavinFrazar December 7, 2022 19:39

r0mant approved these changes Dec 7, 2022

View reviewed changes

Merge branch 'master' into vitor/traits-to-roles-regexp-precompile

1750c5c

vitorenesduarte merged commit 9819e19 into master Dec 7, 2022

vitorenesduarte mentioned this pull request Dec 7, 2022

[v11] Optimize trait loop evaluation #19170

Merged

This was referenced Dec 8, 2022

[v9] Optimize trait loop evaluation #19182

Merged

[v10] Optimize trait loop evaluation #19183

Merged

vitorenesduarte deleted the vitor/traits-to-roles-regexp-precompile branch December 8, 2022 15:03

rosstimothy mentioned this pull request Feb 6, 2025

Improve performance of utils.ReplaceRegexp #51872

Merged

Conversation

vitorenesduarte commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Benchmark

Uh oh!

zmb3 commented Dec 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codingllama commented Dec 6, 2022

Uh oh!

r0mant Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

codingllama Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

vitorenesduarte Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

codingllama left a comment

Choose a reason for hiding this comment

Uh oh!

zmb3 Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

vitorenesduarte Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmb3 Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

vitorenesduarte commented Dec 7, 2022

Uh oh!

codingllama Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

vitorenesduarte Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

codingllama commented Dec 7, 2022

Uh oh!

github-actions Bot commented Dec 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vitorenesduarte commented Dec 6, 2022 •

edited

Loading

vitorenesduarte Dec 7, 2022 •

edited

Loading