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: Improve predicateConditional Matches performance #365

Open
halfcrazy opened this issue Sep 16, 2023 · 2 comments · May be fixed by #366
Open

client: Improve predicateConditional Matches performance #365

halfcrazy opened this issue Sep 16, 2023 · 2 comments · May be fixed by #366

Comments

@halfcrazy
Copy link
Contributor

halfcrazy commented Sep 16, 2023

In our scenario, predicate matches in a hot path and causes performance problems. the cost of reflect.ValueOf(c.predicate).Call([]reflect.Value{reflect.ValueOf(m)}) is quite expensive.

The predicate is expected to be like func (model.Model) bool Eg func (lsp *LogicalSwitchPort) bool { return lsp.Name == "lorem" }. We are trying to eliminate the reflect here. Any suggestions?

// matches returns the result of the execution of the predicate
// Type verifications are not performed
// Returns the models that match the conditions
func (c *predicateConditional) Matches() (map[string]model.Model, error) {
...
	for u, m := range tableCache.RowsShallow() {
		// very expensive and reflect.Value.Call could not cache funcLayout
 ==>		ret := reflect.ValueOf(c.predicate).Call([]reflect.Value{reflect.ValueOf(m)})
		if ret[0].Bool() {
			found[u] = model.Clone(m)
		}
	}
	return found, nil
}

https://github.com/ovn-org/libovsdb/blob/main/client/condition.go#L194-L197

I suppose we could change client API

// Create 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:
// ConditionFromFunc(func(l *LogicalSwitch) bool { return l.Enabled })
WhereCache(predicate interface{}) ConditionalAPI

to

// Create 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:
// ConditionFromFunc(func(l *LogicalSwitch) bool { return l.Enabled })
WhereCache(predicate func(model.Model)bool) ConditionalAPI

And model API, GetTableName function could add to modelgen tmpl.
To get the table name from the predicate

// Conditional interface implementation
// FromFunc returns a Condition from a function
func (a api) conditionFromFunc(predicate interface{}) Conditional {
==>	table, err := a.getTableFromFunc(predicate)
	if err != nil {
		return newErrorConditional(err)
	}

	condition, err := newPredicateConditional(table, a.cache, predicate)
	if err != nil {
		return newErrorConditional(err)
	}
	return condition
}

//type MyLogicalRouter struct {
//	UUID          string            `ovsdb:"_uuid"`
//	Name          string            `ovsdb:"name"`
//	ExternalIDs   map[string]string `ovsdb:"external_ids"`
//	LoadBalancers []string          `ovsdb:"load_balancer"`
//}
type Model interface{}

to

//type MyLogicalRouter struct {
//	UUID          string            `ovsdb:"_uuid"`
//	Name          string            `ovsdb:"name"`
//	ExternalIDs   map[string]string `ovsdb:"external_ids"`
//	LoadBalancers []string          `ovsdb:"load_balancer"`
//}
type Model interface{
	GetTableName()string
}

Then Matches could refactor to

func (c *predicateConditional) Matches() (map[string]model.Model, error) {
...
	for u, m := range tableCache.RowsShallow() {
		if c.predicate(m) {
			found[u] = model.Clone(m)
		}
	}
	return found, nil
}
@halfcrazy
Copy link
Contributor Author

xref #198

@halfcrazy
Copy link
Contributor Author

In our scenario, predicate matches in a hot path and causes performance problems. the cost of reflect.ValueOf(c.predicate).Call([]reflect.Value{reflect.ValueOf(m)}) is quite expensive.

The predicate is expected to be like func (model.Model) bool Eg func (lsp *LogicalSwitchPort) bool { return lsp.Name == "lorem" }. We are trying to eliminate the reflect here. Any suggestions?

// matches returns the result of the execution of the predicate
// Type verifications are not performed
// Returns the models that match the conditions
func (c *predicateConditional) Matches() (map[string]model.Model, error) {
...
	for u, m := range tableCache.RowsShallow() {
		// very expensive and reflect.Value.Call could not cache funcLayout
 ==>		ret := reflect.ValueOf(c.predicate).Call([]reflect.Value{reflect.ValueOf(m)})
		if ret[0].Bool() {
			found[u] = model.Clone(m)
		}
	}
	return found, nil
}

https://github.com/ovn-org/libovsdb/blob/main/client/condition.go#L194-L197

I suppose we could change client API

// Create 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:
// ConditionFromFunc(func(l *LogicalSwitch) bool { return l.Enabled })
WhereCache(predicate interface{}) ConditionalAPI

to

// Create 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:
// ConditionFromFunc(func(l *LogicalSwitch) bool { return l.Enabled })
WhereCache(predicate func(model.Model)bool) ConditionalAPI

And model API, GetTableName function could add to modelgen tmpl. To get the table name from the predicate

// Conditional interface implementation
// FromFunc returns a Condition from a function
func (a api) conditionFromFunc(predicate interface{}) Conditional {
==>	table, err := a.getTableFromFunc(predicate)
	if err != nil {
		return newErrorConditional(err)
	}

	condition, err := newPredicateConditional(table, a.cache, predicate)
	if err != nil {
		return newErrorConditional(err)
	}
	return condition
}
//type MyLogicalRouter struct {
//	UUID          string            `ovsdb:"_uuid"`
//	Name          string            `ovsdb:"name"`
//	ExternalIDs   map[string]string `ovsdb:"external_ids"`
//	LoadBalancers []string          `ovsdb:"load_balancer"`
//}
type Model interface{}

to

//type MyLogicalRouter struct {
//	UUID          string            `ovsdb:"_uuid"`
//	Name          string            `ovsdb:"name"`
//	ExternalIDs   map[string]string `ovsdb:"external_ids"`
//	LoadBalancers []string          `ovsdb:"load_balancer"`
//}
type Model interface{
	GetTableName()string
}

Then Matches could refactor to

func (c *predicateConditional) Matches() (map[string]model.Model, error) {
...
	for u, m := range tableCache.RowsShallow() {
		if c.predicate(m) {
			found[u] = model.Clone(m)
		}
	}
	return found, nil
}

Edit:
According to the test file looks like there was a Table function in model.Model interface

//LogicalSwitchPort struct defines an object in Logical_Switch_Port table
type testLogicalSwitchPort struct {
	UUID             string            `ovsdb:"_uuid"`
	Up               *bool             `ovsdb:"up"`
	Dhcpv4Options    *string           `ovsdb:"dhcpv4_options"`
	Name             string            `ovsdb:"name"`
	DynamicAddresses *string           `ovsdb:"dynamic_addresses"`
	HaChassisGroup   *string           `ovsdb:"ha_chassis_group"`
	Options          map[string]string `ovsdb:"options"`
	Enabled          *bool             `ovsdb:"enabled"`
	Addresses        []string          `ovsdb:"addresses"`
	Dhcpv6Options    *string           `ovsdb:"dhcpv6_options"`
	TagRequest       *int              `ovsdb:"tag_request"`
	Tag              *int              `ovsdb:"tag"`
	PortSecurity     []string          `ovsdb:"port_security"`
	ExternalIds      map[string]string `ovsdb:"external_ids"`
	Type             string            `ovsdb:"type"`
	ParentName       *string           `ovsdb:"parent_name"`
}

// Table returns the table name. It's part of the Model interface
func (*testLogicalSwitchPort) Table() string {
	return "Logical_Switch_Port"
}

@halfcrazy halfcrazy linked a pull request Sep 19, 2023 that will close this issue
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 a pull request may close this issue.

1 participant