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

client: perf/WhereCache #366

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

halfcrazy
Copy link
Contributor

@halfcrazy halfcrazy commented Sep 19, 2023

Fix #365

// WhereCache creates a Conditional API from a Function that is used to filter cached data
// The function must accept a Model implementation and return a boolean. E.g:
// WhereCache(&LogicalSwitchPort{}, func(m model.Model) bool { l := m.(*LogicalSwitchPort); return l.Enabled })
WhereCache(model.Model, func(model.Model) bool) ConditionalAPI
$ benchstat bench.out.old bench.out.new
goos: linux
goarch: amd64
pkg: github.com/ovn-org/libovsdb/client
cpu: Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
                                                            │ bench.out.old │            bench.out.new             │
                                                            │    sec/op     │    sec/op     vs base                │
APIList/predicate_returns_none-4                               3.297m ±  5%   1.072m ±  7%  -67.48% (p=0.000 n=10)
APIList/predicate_returns_all-4                                74.05m ± 13%   86.59m ±  7%  +16.95% (p=0.029 n=10)
APIList/predicate_on_an_arbitrary_condition-4                  12.39m ± 26%   10.30m ± 12%  -16.85% (p=0.000 n=10)
APIList/predicate_matches_name-4                               4.111m ± 14%   1.433m ± 19%  -65.14% (p=0.000 n=10)
APIList/by_index,_no_predicate-4                               37.06µ ± 21%   37.29µ ± 14%        ~ (p=0.971 n=10)
APIListMultiple/multiple_results_one_at_a_time_with_Get-4      6.706m ± 21%   6.582m ± 26%        ~ (p=0.739 n=10)
APIListMultiple/multiple_results_in_a_batch_with_WhereAny-4    3.104m ±  3%   3.513m ± 11%  +13.19% (p=0.003 n=10)
Update1-4                                                      107.9µ ±  7%   122.1µ ± 10%  +13.14% (p=0.002 n=10)
Update2-4                                                      166.6µ ±  3%   180.9µ ± 12%   +8.60% (p=0.015 n=10)
Update3-4                                                      223.9µ ±  4%   237.3µ ± 18%   +5.98% (p=0.023 n=10)
Update5-4                                                      337.3µ ±  2%   350.4µ ±  5%   +3.89% (p=0.011 n=10)
Update8-4                                                      519.5µ ±  2%   542.6µ ± 11%   +4.45% (p=0.005 n=10)
geomean                                                        1.173m         1.014m        -13.56%

                                                            │ bench.out.old │            bench.out.new             │
                                                            │     B/op      │     B/op      vs base                │
APIList/predicate_returns_none-4                               908.6Ki ± 0%   664.4Ki ± 0%  -26.87% (p=0.000 n=10)
APIList/predicate_returns_all-4                                14.88Mi ± 0%   14.65Mi ± 0%   -1.54% (p=0.000 n=10)
APIList/predicate_on_an_arbitrary_condition-4                  2.468Mi ± 0%   2.229Mi ± 0%   -9.67% (p=0.000 n=10)
APIList/predicate_matches_name-4                               910.3Ki ± 0%   666.1Ki ± 0%  -26.82% (p=0.000 n=10)
APIList/by_index,_no_predicate-4                               82.00Ki ± 0%   82.00Ki ± 0%        ~ (p=1.000 n=10)
APIListMultiple/multiple_results_one_at_a_time_with_Get-4      1.440Mi ± 0%   1.440Mi ± 0%        ~ (p=0.810 n=10)
APIListMultiple/multiple_results_in_a_batch_with_WhereAny-4    713.0Ki ± 0%   713.0Ki ± 0%        ~ (p=0.481 n=10)
Update1-4                                                      32.00Ki ± 0%   32.01Ki ± 0%        ~ (p=0.171 n=10)
Update2-4                                                      49.60Ki ± 0%   49.60Ki ± 0%        ~ (p=0.753 n=10)
Update3-4                                                      67.27Ki ± 0%   67.28Ki ± 0%   +0.02% (p=0.009 n=10)
Update5-4                                                      102.7Ki ± 0%   102.6Ki ± 0%        ~ (p=0.183 n=10)
Update8-4                                                      155.3Ki ± 0%   155.3Ki ± 0%        ~ (p=0.403 n=10)
geomean                                                        359.6Ki        338.0Ki        -6.00%

                                                            │ bench.out.old │             bench.out.new             │
                                                            │   allocs/op   │  allocs/op   vs base                  │
APIList/predicate_returns_none-4                              20010.00 ± 0%    10.00 ± 0%  -99.95% (p=0.000 n=10)
APIList/predicate_returns_all-4                                 230.1k ± 0%   210.1k ± 0%   -8.69% (p=0.000 n=10)
APIList/predicate_on_an_arbitrary_condition-4                   43.36k ± 0%   23.36k ± 0%  -46.12% (p=0.000 n=10)
APIList/predicate_matches_name-4                              20032.00 ± 0%    32.00 ± 0%  -99.84% (p=0.000 n=10)
APIList/by_index,_no_predicate-4                                 32.00 ± 0%    32.00 ± 0%        ~ (p=1.000 n=10) ¹
APIListMultiple/multiple_results_one_at_a_time_with_Get-4       22.02k ± 0%   22.02k ± 0%        ~ (p=1.000 n=10)
APIListMultiple/multiple_results_in_a_batch_with_WhereAny-4     11.51k ± 0%   11.51k ± 0%        ~ (p=1.000 n=10)
Update1-4                                                        855.0 ± 0%    855.0 ± 0%        ~ (p=1.000 n=10) ¹
Update2-4                                                       1.353k ± 0%   1.353k ± 0%        ~ (p=1.000 n=10) ¹
Update3-4                                                       1.851k ± 0%   1.851k ± 0%        ~ (p=1.000 n=10) ¹
Update5-4                                                       2.844k ± 0%   2.844k ± 0%        ~ (p=1.000 n=10) ¹
Update8-4                                                       4.329k ± 0%   4.329k ± 0%        ~ (p=1.000 n=10) ¹
geomean                                                         5.551k        1.624k       -70.75%
¹ all samples are equal

@halfcrazy halfcrazy force-pushed the perf/wherecache branch 2 times, most recently from 78d5c25 to 33fa6b4 Compare September 19, 2023 05:11
@halfcrazy
Copy link
Contributor Author

CPU profile from bench test
new

old

@halfcrazy halfcrazy changed the title DNM: Perf/wherecache DNM: perf/WhereCache Sep 19, 2023
@halfcrazy halfcrazy changed the title DNM: perf/WhereCache client: perf/WhereCache Sep 19, 2023
@dcbw
Copy link
Contributor

dcbw commented Sep 21, 2023

Seems generally the right thing to do... Most (all?) uses of WhereCache() already have a model that the predicate uses.

@halfcrazy
Copy link
Contributor Author

halfcrazy commented Sep 22, 2023

Seems generally the right thing to do... Most (all?) uses of WhereCache() already have a model that the predicate uses.

Emm, I found it difficult to get table information from the predicate callback function since model.Model is just an interface, not a struct. I cannot create a model.Model instance and call Table(). We need a table name to choose which TableCache to use.

@halfcrazy halfcrazy force-pushed the perf/wherecache branch 2 times, most recently from 78d5ea5 to 754b46b Compare September 23, 2023 02:01
@@ -246,6 +246,9 @@ type {{ index . "StructName" }} struct {
{{ end }}
{{ template "extraFields" . }}
}
func (a *{{ index . "StructName" }}) Table() string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@halfcrazy The Table method clashes with the ovn sbdb RBAC_Permission table column
https://github.com/ovn-org/ovn/blob/686caaf66d5be811c655ea2938b082564d5f5f75/ovn-sb.ovsschema#L383

Copy link
Contributor Author

@halfcrazy halfcrazy Sep 27, 2023

Choose a reason for hiding this comment

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

Oops, Any suggestions for this method name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't come up with a good way to future proof this and I am not sure if it is even worth it, given that solving a potential clash would just require a new modelgen along with the libovsdb bump. This might become more meaningful if we ever get past v1.0.0 and I guess we shall decide then which kind of version step we would need to support new schemas we never knew about. Let me know if you think any different.

So how does SchemaTable sound?

Using an uppercase unicode first letter that is not valid for an OVSDB id per the RFC is another option, but I don't think that's idiomatic and I have never seen it done.

@dcbw do you have any other ideas?

Copy link
Contributor Author

@halfcrazy halfcrazy Sep 27, 2023

Choose a reason for hiding this comment

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

Update, rename to TableName

A getter-style func name? GetTableName() like I proposed in issue #365

import "github.com/ovn-org/libovsdb/model"

const RBACPermissionTable = "RBAC_Permission"

// RBACPermission defines an object in RBAC_Permission table
type RBACPermission struct {
	UUID          string   `ovsdb:"_uuid"`
	Authorization []string `ovsdb:"authorization"`
	InsertDelete  bool     `ovsdb:"insert_delete"`
	Table         string   `ovsdb:"table"`
	Update        []string `ovsdb:"update"`
}

func (a *RBACPermission) GetTableName() string {
	return RBACPermissionTable
}

func (a *RBACPermission) GetUUID() string {
	return a.UUID
}
...

@halfcrazy halfcrazy force-pushed the perf/wherecache branch 4 times, most recently from 5e3aade to 60fa694 Compare September 28, 2023 08:16
@coveralls
Copy link

coveralls commented Feb 6, 2024

Pull Request Test Coverage Report for Build 6441663142

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 28 of 33 (84.85%) changed or added relevant lines in 7 files are covered.
  • 15 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.1%) to 75.84%

Changes Missing Coverage Covered Lines Changed/Added Lines %
client/api.go 18 20 90.0%
ovsdb/serverdb/database.go 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
client/client.go 2 73.78%
client/api.go 13 79.34%
Totals Coverage Status
Change from base Build 6159565928: -0.1%
Covered Lines: 5283
Relevant Lines: 6966

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6326111541

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 27 of 33 (81.82%) changed or added relevant lines in 7 files are covered.
  • 14 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.2%) to 75.822%

Changes Missing Coverage Covered Lines Changed/Added Lines %
client/api.go 17 20 85.0%
ovsdb/serverdb/database.go 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
client/client.go 2 73.68%
client/api.go 12 79.28%
Totals Coverage Status
Change from base Build 6159565928: -0.2%
Covered Lines: 5281
Relevant Lines: 6965

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 6326111541

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 27 of 33 (81.82%) changed or added relevant lines in 7 files are covered.
  • 14 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.2%) to 75.822%

Changes Missing Coverage Covered Lines Changed/Added Lines %
client/api.go 17 20 85.0%
ovsdb/serverdb/database.go 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
client/client.go 2 73.68%
client/api.go 12 79.28%
Totals Coverage Status
Change from base Build 6159565928: -0.2%
Covered Lines: 5281
Relevant Lines: 6965

💛 - Coveralls

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.

client: Improve predicateConditional Matches performance
4 participants