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.

Sign up for GitHub

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

Jump to bottom

Implement a new, experimental variant of LookupResources as LookupResources2 #1905

Merged
merged 5 commits into from
Jul 24, 2024
Merged

Implement a new, experimental variant of LookupResources as LookupResources2 #1905

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
75ad751
Implement a new, experimental variant of LookupResources as LookupRes…
josephschorr May 17, 2024
f2c40f4
Add additional steelthread tests for LookupResources and fix issue in…
josephschorr Jul 1, 2024
dedb1f8
Switch experimental LookupResources2 to request additional chunks of …
josephschorr Jul 11, 2024
5147ba1
Re-engineer how check hints are handled to use protos for performance
josephschorr Jul 18, 2024
d889495
Review feedback and additional tests
josephschorr Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
236 changes: 174 additions & 62 deletions internal/caveats/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,21 +63,26 @@ type ExpressionResult interface {
}

type syntheticResult struct {
value bool
contextValues map[string]any
exprString string
value bool
contextValues map[string]any
exprString string
missingContextParams []string
}

func (sr syntheticResult) Value() bool {
return sr.value
}

func (sr syntheticResult) IsPartial() bool {
return false
return len(sr.missingContextParams) > 0
}

func (sr syntheticResult) MissingVarNames() ([]string, error) {
return nil, fmt.Errorf("not a partial value")
if len(sr.missingContextParams) == 0 {
return nil, fmt.Errorf("not a partial value")
}

return sr.missingContextParams, nil
}

func (sr syntheticResult) ContextValues() map[string]any {
Expand Down Expand Up @@ -156,6 +161,14 @@ func (lc loadedCaveats) Get(caveatDefName string) (*core.CaveatDefinition, *cave
return caveat, justDeserialized, nil
}

func isFalseResult(resuilt ExpressionResult) bool {
vroldanbet marked this conversation as resolved.
Show resolved Hide resolved
return !resuilt.Value() && !resuilt.IsPartial()
}

func isTrueResult(resuilt ExpressionResult) bool {
vroldanbet marked this conversation as resolved.
Show resolved Hide resolved
return resuilt.Value() && !resuilt.IsPartial()
}

func runExpressionWithCaveats(
ctx context.Context,
env *caveats.Environment,
Expand Down Expand Up @@ -203,15 +216,30 @@ func runExpressionWithCaveats(
}

cop := expr.GetOperation()
boolResult := false
if cop.Op == core.CaveatOperation_AND {
boolResult = true
}

var contextValues map[string]any
var exprStringPieces []string

var currentResult ExpressionResult = syntheticResult{
value: false,
contextValues: map[string]any{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Initializing syntheticResult.contextValues with an empty map does an unnecessary allocation to the heap. combineMap does take care of nil first argument when merging what has been found, which adds one extra unnecessary allocation.

I did some benchmarks, and combineMaps can be made faster and do one less allocation in the case one of the arguments is nil or empty.

Given the caveat is being executed for each one of the tuples coming out of ReverseQueryRelationships via redispatchOrReportOverDatabaseQuery, I thought this could lead to N unnecessary allocations in the critical path.

func combineMaps(first map[string]any, second map[string]any) map[string]any {
	if first == nil || len(first) == 0 {
		return maps.Clone(second)
	} else if second == nil || len(second) == 0 {
		return maps.Clone(first)
	}

	cloned := maps.Clone(first)
	maps.Copy(cloned, second)
	return cloned
}

Copy link
Member Author

Choose a reason for hiding this comment

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

No need to clone them if they aren't changing

Copy link
Contributor

@vroldanbet vroldanbet Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

please write a benchmark like I did, and make sure it reduces allocations and CPU time for:

  • first is nil, second is not
  • first has elements, second does not

My code above moved the needle from 3 allocations to 2 and made it 50% faster

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I don't clone now at all unless necessary, it should be 1 less even

exprString: "",
missingContextParams: []string{},
}
if cop.Op == core.CaveatOperation_AND {
currentResult = syntheticResult{
value: true,
contextValues: map[string]any{},
exprString: "",
missingContextParams: []string{},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
currentResult = syntheticResult{
value: true,
contextValues: map[string]any{},
exprString: "",
missingContextParams: []string{},
}
currentResult.value = true

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't; the type of the result is not a synthetic

Copy link
Contributor

Choose a reason for hiding this comment

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

Then:

	var currentResult ExpressionResult = syntheticResult{
		value:                cop.Op == core.CaveatOperation_AND,
		contextValues:        map[string]any{},
		exprString:           "",
		missingContextParams: []string{},
	}

}

buildExprString := func() (string, error) {
if debugOption != RunCaveatExpressionWithDebugInformation {
return "", nil
}

switch cop.Op {
case core.CaveatOperation_AND:
return strings.Join(exprStringPieces, " && "), nil
Expand All @@ -227,90 +255,174 @@ func runExpressionWithCaveats(
}
}

for _, child := range cop.Children {
childResult, err := runExpressionWithCaveats(ctx, env, child, context, loadedCaveats, debugOption)
if err != nil {
addDebugInfo := func(found ExpressionResult) error {
if debugOption == RunCaveatExpressionWithDebugInformation {
contextValues = combineMaps(contextValues, found.ContextValues())
exprString, err := found.ExpressionString()
if err != nil {
return err
}

exprStringPieces = append(exprStringPieces, exprString)
}

return nil
}

and := func(existing ExpressionResult, found ExpressionResult) (ExpressionResult, error) {
if err := addDebugInfo(found); err != nil {
return nil, err
}

if childResult.IsPartial() {
return childResult, nil
var missingContextParams []string
if existing.IsPartial() {
params, err := existing.MissingVarNames()
if err != nil {
return nil, err
}

missingContextParams = params
} else if !existing.Value() {
return existing, nil
vroldanbet marked this conversation as resolved.
Show resolved Hide resolved
}

switch cop.Op {
case core.CaveatOperation_AND:
boolResult = boolResult && childResult.Value()
if found.IsPartial() {
params, err := found.MissingVarNames()
if err != nil {
return nil, err
}

if debugOption == RunCaveatExpressionWithDebugInformation {
contextValues = combineMaps(contextValues, childResult.ContextValues())
exprString, err := childResult.ExpressionString()
if err != nil {
return nil, err
}
missingContextParams = append(missingContextParams, params...)
} else if !found.Value() {
return found, nil
}

exprString, err := buildExprString()
if err != nil {
return nil, err
}

exprStringPieces = append(exprStringPieces, exprString)
return syntheticResult{
value: existing.Value() && found.Value(),
contextValues: contextValues,
exprString: exprString,
missingContextParams: missingContextParams,
}, nil
}

or := func(existing ExpressionResult, found ExpressionResult) (ExpressionResult, error) {
if err := addDebugInfo(found); err != nil {
return nil, err
}

var missingContextParams []string
if existing.IsPartial() {
params, err := existing.MissingVarNames()
if err != nil {
return nil, err
}

if !boolResult {
built, err := buildExprString()
if err != nil {
return nil, err
}
missingContextParams = params
} else if existing.Value() {
return found, nil
vroldanbet marked this conversation as resolved.
Show resolved Hide resolved
}

return syntheticResult{false, contextValues, built}, nil
if found.IsPartial() {
params, err := found.MissingVarNames()
if err != nil {
return nil, err
}

case core.CaveatOperation_OR:
boolResult = boolResult || childResult.Value()
missingContextParams = append(missingContextParams, params...)
} else if found.Value() {
return found, nil
}

if debugOption == RunCaveatExpressionWithDebugInformation {
contextValues = combineMaps(contextValues, childResult.ContextValues())
exprString, err := childResult.ExpressionString()
if err != nil {
return nil, err
}
exprString, err := buildExprString()
if err != nil {
return nil, err
}

return syntheticResult{
value: existing.Value() || found.Value(),
contextValues: contextValues,
exprString: exprString,
missingContextParams: missingContextParams,
}, nil
}

exprStringPieces = append(exprStringPieces, exprString)
invert := func(existing ExpressionResult) (ExpressionResult, error) {
if debugOption == RunCaveatExpressionWithDebugInformation {
contextValues = combineMaps(contextValues, existing.ContextValues())
exprString, err := existing.ExpressionString()
if err != nil {
return nil, err
}

if boolResult {
built, err := buildExprString()
if err != nil {
return nil, err
}
exprStringPieces = append(exprStringPieces, "!("+exprString+")")
}

value := !existing.Value()

var missingContextParams []string
if existing.IsPartial() {
value = false // partials always have a value of false.
missingContextParams, _ = existing.MissingVarNames()
}

exprString, err := buildExprString()
if err != nil {
return nil, err
}

return syntheticResult{
value: value,
contextValues: contextValues,
exprString: exprString,
missingContextParams: missingContextParams,
}, nil
}

return syntheticResult{true, contextValues, built}, nil
for _, child := range cop.Children {
childResult, err := runExpressionWithCaveats(ctx, env, child, context, loadedCaveats, debugOption)
if err != nil {
return nil, err
}

switch cop.Op {
case core.CaveatOperation_AND:
cr, err := and(currentResult, childResult)
if err != nil {
return nil, err
}

case core.CaveatOperation_NOT:
if debugOption == RunCaveatExpressionWithDebugInformation {
contextValues = combineMaps(contextValues, childResult.ContextValues())
exprString, err := childResult.ExpressionString()
if err != nil {
return nil, err
}

exprStringPieces = append(exprStringPieces, "!("+exprString+")")
currentResult = cr

if isFalseResult(currentResult) {
return currentResult, nil
}

built, err := buildExprString()
case core.CaveatOperation_OR:
cr, err := or(currentResult, childResult)
if err != nil {
return nil, err
}

return syntheticResult{!childResult.Value(), contextValues, built}, nil
currentResult = cr

if isTrueResult(currentResult) {
return currentResult, nil
}

case core.CaveatOperation_NOT:
return invert(childResult)

default:
return nil, spiceerrors.MustBugf("unknown caveat operation: %v", cop.Op)
}
}

built, err := buildExprString()
if err != nil {
return nil, err
}

return syntheticResult{boolResult, contextValues, built}, nil
return currentResult, nil
}

func combineMaps(first map[string]any, second map[string]any) map[string]any {
Expand Down
Loading
Loading