client: perf/WhereCache

[halfcrazy]

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]

CPU profile from bench test

[dcbw]

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

[halfcrazy]

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.

[jcaamano]

@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

[halfcrazy]

Oops, Any suggestions for this method name?

[jcaamano]

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?

[halfcrazy]

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
}
...

[coveralls]

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.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

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
Change from base Build 6159565928:	-0.1%
Covered Lines:	5283
Relevant Lines:	6966

---

💛 - Coveralls