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

eval: performance improvements #392

Merged
merged 2 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions CHANGELOG_PENDING.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
### Improvements

- Improve evaluation performance and memory footprint.
[#392](https://github.com/pulumi/esc/pull/392)

### Bug Fixes

### Breaking changes

- `schema`: `ObjectBuilder.Properties` and `Record` now take a `MapBuilder` in order to avoid copies.
[#392](https://github.com/pulumi/esc/pull/392)
6 changes: 3 additions & 3 deletions analysis/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,18 @@ import (
)

var testProviderSchema = schema.Object().
Properties(map[string]schema.Builder{
Properties(schema.BuilderMap{
"address": schema.String().
Description("The URL of the Vault server. Must contain a scheme and hostname, but no path."),
"jwt": schema.Object().
Properties(map[string]schema.Builder{
Properties(schema.BuilderMap{
"mount": schema.String().Description("The name of the authentication engine mount."),
"role": schema.String().Description("The name of the role to use for login."),
}).
Required("role").
Description("Options for JWT login. JWT login uses an OIDC token issued by the Pulumi Cloud to generate an ephemeral token."),
"token": schema.Object().
Properties(map[string]schema.Builder{
Properties(schema.BuilderMap{
"displayName": schema.String().Description("The display name of the ephemeral token. Defaults to 'pulumi'."),
"token": schema.String().Description("The parent token."),
"maxTtl": schema.String().
Expand Down
2 changes: 1 addition & 1 deletion ast/environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func (d *MapDecl[T]) parse(name string, node syntax.Node) syntax.Diagnostics {
kvp := obj.Index(i)

var v T
vname := fmt.Sprintf("%s.%s", name, kvp.Key.Value())
vname := name + "." + kvp.Key.Value()
vdiags := parseNode(vname, &v, kvp.Value)
diags.Extend(vdiags...)

Expand Down
2 changes: 1 addition & 1 deletion ast/expr.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ type SymbolExpr struct {
func Symbol(accessors ...PropertyAccessor) *SymbolExpr {
property := &PropertyAccess{Accessors: accessors}
return &SymbolExpr{
exprNode: expr(syntax.String(fmt.Sprintf("${%v}", property))),
exprNode: expr(syntax.String("${" + property.String() + "}")),
Property: property,
}
}
Expand Down
6 changes: 3 additions & 3 deletions cmd/esc/cli/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ func TestCheckYAMLEnvironment(t *testing.T) {
expected := &esc.Environment{
Exprs: map[string]esc.Expr{"foo": {Literal: "bar"}},
Properties: map[string]esc.Value{"foo": esc.NewValue("bar")},
Schema: schema.Record(map[string]schema.Builder{"foo": schema.String().Const("bar")}).Schema(),
Schema: schema.Record(schema.BuilderMap{"foo": schema.String().Const("bar")}).Schema(),
}

client := newTestClient(t, http.MethodPost, "/api/esc/environments/test-org/yaml/check", func(w http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -604,7 +604,7 @@ func TestGetOpenEnvironment(t *testing.T) {
expected := &esc.Environment{
Exprs: map[string]esc.Expr{"foo": {Literal: "bar"}},
Properties: map[string]esc.Value{"foo": esc.NewValue("bar")},
Schema: schema.Record(map[string]schema.Builder{"foo": schema.String().Const("bar")}).Schema(),
Schema: schema.Record(schema.BuilderMap{"foo": schema.String().Const("bar")}).Schema(),
}

client := newTestClient(t, http.MethodGet, "/api/esc/environments/test-org/test-project/test-env/open/session", func(w http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -638,7 +638,7 @@ func TestGetAnonymousOpenEnvironment(t *testing.T) {
expected := &esc.Environment{
Exprs: map[string]esc.Expr{"foo": {Literal: "bar"}},
Properties: map[string]esc.Value{"foo": esc.NewValue("bar")},
Schema: schema.Record(map[string]schema.Builder{"foo": schema.String().Const("bar")}).Schema(),
Schema: schema.Record(schema.BuilderMap{"foo": schema.String().Const("bar")}).Schema(),
}

client := newTestClient(t, http.MethodGet, "/api/esc/environments/test-org/yaml/open/session", func(w http.ResponseWriter, r *http.Request) {
Expand Down
8 changes: 4 additions & 4 deletions eval/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ func (e *evalContext) evaluate() (*value, syntax.Diagnostics) {
// root.
properties := make(map[string]*expr, len(e.env.Values.GetEntries()))
e.root = &expr{
path: fmt.Sprintf("<%v>", e.name),
path: "<" + e.name + ">",
repr: &objectExpr{
node: ast.Object(),
properties: properties,
Expand Down Expand Up @@ -402,7 +402,7 @@ func (e *evalContext) evaluateImports() {
e.evaluateImport(myImports, entry)
}

properties := make(map[string]schema.Builder, len(myImports))
properties := make(schema.SchemaMap, len(myImports))
for k, v := range myImports {
properties[k] = v.schema
}
Expand Down Expand Up @@ -582,7 +582,7 @@ func (e *evalContext) evaluateObject(x *expr, repr *objectExpr) *value {
keys := maps.Keys(repr.properties)
sort.Strings(keys)

object, properties := make(map[string]*value, len(keys)), make(map[string]schema.Builder, len(keys))
object, properties := make(map[string]*value, len(keys)), make(schema.SchemaMap, len(keys))
for _, k := range keys {
pv := e.evaluateExpr(repr.properties[k])
object[k], properties[k] = pv, pv.schema
Expand Down Expand Up @@ -921,7 +921,7 @@ func (e *evalContext) evaluateBuiltinOpen(x *expr, repr *openExpr) *value {

output, err := provider.Open(e.ctx, inputs.export("").Value.(map[string]esc.Value), e.execContext)
if err != nil {
e.errorf(repr.syntax(), err.Error())
e.errorf(repr.syntax(), "%s", err.Error())
v.unknown = true
return v
}
Expand Down
12 changes: 6 additions & 6 deletions eval/eval_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func accept() bool {
type errorProvider struct{}

func (errorProvider) Schema() (*schema.Schema, *schema.Schema) {
return schema.Record(map[string]schema.Builder{"why": schema.String()}).Schema(), schema.Always()
return schema.Record(schema.BuilderMap{"why": schema.String()}).Schema(), schema.Always()
}

func (errorProvider) Open(ctx context.Context, inputs map[string]esc.Value, context esc.EnvExecContext) (esc.Value, error) {
Expand All @@ -54,12 +54,12 @@ type testSchemaProvider struct{}

func (testSchemaProvider) Schema() (*schema.Schema, *schema.Schema) {
s := schema.Object().
Defs(map[string]schema.Builder{
"defRecord": schema.Record(map[string]schema.Builder{
Defs(schema.BuilderMap{
"defRecord": schema.Record(schema.BuilderMap{
"baz": schema.String().Const("qux"),
}),
}).
Properties(map[string]schema.Builder{
Properties(schema.BuilderMap{
"null": schema.Null(),
"boolean": schema.Boolean(),
"false": schema.Boolean().Const(false),
Expand All @@ -71,7 +71,7 @@ func (testSchemaProvider) Schema() (*schema.Schema, *schema.Schema) {
"array": schema.Array().Items(schema.Always()),
"tuple": schema.Tuple(schema.String().Const("hello"), schema.String().Const("world")),
"map": schema.Object().AdditionalProperties(schema.Always()),
"record": schema.Record(map[string]schema.Builder{
"record": schema.Record(schema.BuilderMap{
"foo": schema.String(),
}),
"anyOf": schema.AnyOf(schema.String(), schema.Number()),
Expand All @@ -87,7 +87,7 @@ func (testSchemaProvider) Schema() (*schema.Schema, *schema.Schema) {
"double": schema.Tuple(schema.String(), schema.Number()),
"triple": schema.Tuple(schema.String(), schema.Number(), schema.Boolean()),
"dependentReq": schema.Object().
Properties(map[string]schema.Builder{
Properties(schema.BuilderMap{
"foo": schema.String(),
"bar": schema.Number(),
}).DependentRequired(map[string][]string{"foo": {"bar"}}),
Expand Down
4 changes: 2 additions & 2 deletions eval/expr.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,8 @@ func (x *expr) export(environment string) esc.Expr {
ex.Builtin = &esc.BuiltinExpr{
Name: name,
NameRange: convertRange(repr.node.Name().Syntax().Syntax().Range(), environment),
ArgSchema: schema.Record(map[string]schema.Builder{
"provider": schema.String(),
ArgSchema: schema.Record(schema.SchemaMap{
"provider": schema.String().Schema(),
"inputs": repr.inputSchema,
}).Schema(),
Arg: esc.Expr{
Expand Down
53 changes: 41 additions & 12 deletions eval/value.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ type value struct {
base *value // the base value, if any
schema *schema.Schema // the value's schema

mergedKeys []string // the value's merged keys. computed lazily--use keys().
exported *esc.Value // non-nil if this value has already been exported

// true if the value is unknown (e.g. because it did not evaluate successfully or is the result of an unevaluated
// fn::open)
unknown bool
Expand Down Expand Up @@ -149,20 +152,36 @@ func (v *value) combine(others ...*value) {
// keys returns the value's keys if the value is an object. This method should be used instead of accessing the
// underlying map[string]*value directly, as it takes JSON merge patch semantics into account.
func (v *value) keys() []string {
keySet := make(map[string]struct{})
for v != nil {
if v == nil {
return nil
}
if v.mergedKeys == nil {
m, ok := v.repr.(map[string]*value)
if !ok {
break
return nil
}
for k := range m {
keySet[k] = struct{}{}

baseKeys := v.base.keys()
if len(baseKeys) == 0 {
v.mergedKeys = maps.Keys(m)
} else {
l := len(baseKeys)
if l < len(m) {
l = len(m)
}
keySet := make(map[string]struct{}, l)

for _, k := range baseKeys {
keySet[k] = struct{}{}
}
for k := range m {
keySet[k] = struct{}{}
}
v.mergedKeys = maps.Keys(keySet)
}
v = v.base
sort.Strings(v.mergedKeys)
}
keys := maps.Keys(keySet)
sort.Strings(keys)
return keys
return v.mergedKeys
}

// property returns the named property (if any) as per JSON merge patch semantics. If the receiver is unknown,
Expand Down Expand Up @@ -269,6 +288,10 @@ func (v *value) toString() (str string, unknown bool, secret bool) {

// export converts the value into its serializable representation.
func (v *value) export(environment string) esc.Value {
if v.exported != nil {
return *v.exported
}

var pv any
switch repr := v.repr.(type) {
case []*value:
Expand All @@ -295,7 +318,7 @@ func (v *value) export(environment string) esc.Value {
base = &b
}

return esc.Value{
v.exported = &esc.Value{
Value: pv,
Secret: v.secret,
Unknown: v.unknown,
Expand All @@ -304,6 +327,7 @@ func (v *value) export(environment string) esc.Value {
Base: base,
},
}
return *v.exported
}

// unexport creates a value from a Value. This is used when interacting with providers, as the Provider API works on
Expand All @@ -327,7 +351,7 @@ func unexport(v esc.Value, x *expr) *value {
}
vv.repr, vv.schema = a, schema.Tuple(items...).Schema()
case map[string]esc.Value:
m, properties := make(map[string]*value, len(pv)), make(map[string]schema.Builder, len(pv))
m, properties := make(map[string]*value, len(pv)), make(schema.SchemaMap, len(pv))
for k, v := range pv {
uv := unexport(v, x)
m[k], properties[k] = uv, uv.schema
Expand All @@ -348,7 +372,12 @@ func mergedSchema(base, top *schema.Schema) *schema.Schema {
return top
}

record := make(map[string]schema.Builder)
l := len(base.Properties)
if l < len(top.Properties) {
l = len(top.Properties)
}

record := make(schema.SchemaMap, l)
for k, base := range base.Properties {
record[k] = base
}
Expand Down
15 changes: 7 additions & 8 deletions schema/objects.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ func Object() *ObjectBuilder {
return &ObjectBuilder{}
}

func Record(m map[string]Builder) *ObjectBuilder {
names := maps.Keys(m)
func Record(m MapBuilder) *ObjectBuilder {
props := m.Build()

names := maps.Keys(props)
sort.Strings(names)

return Object().Properties(m).Required(names...)
return Object().Properties(SchemaMap(props)).Required(names...)
}

func (b *ObjectBuilder) Defs(defs map[string]Builder) *ObjectBuilder {
Expand All @@ -53,11 +55,8 @@ func (b *ObjectBuilder) OneOf(oneOf ...Builder) *ObjectBuilder {
return buildOneOf(b, oneOf)
}

func (b *ObjectBuilder) Properties(m map[string]Builder) *ObjectBuilder {
b.s.Properties = make(map[string]*Schema, len(m))
for k, v := range m {
b.s.Properties[k] = v.Schema()
}
func (b *ObjectBuilder) Properties(m MapBuilder) *ObjectBuilder {
b.s.Properties = m.Build()
return b
}

Expand Down
6 changes: 3 additions & 3 deletions schema/objects_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (

func TestObjects(t *testing.T) {
t.Run("record", func(t *testing.T) {
record := map[string]Builder{
record := BuilderMap{
"bool": Boolean(),
"string": String(),
}
Expand All @@ -39,7 +39,7 @@ func TestObjects(t *testing.T) {
})

t.Run("defs", func(t *testing.T) {
defs := map[string]Builder{
defs := BuilderMap{
"bool": Boolean(),
}
s := Object().
Expand Down Expand Up @@ -89,7 +89,7 @@ func TestObjects(t *testing.T) {
})

t.Run("properties", func(t *testing.T) {
properties := map[string]Builder{
properties := BuilderMap{
"bool": Boolean(),
"string": String(),
}
Expand Down
20 changes: 20 additions & 0 deletions schema/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,26 @@ type Builder interface {
Schema() *Schema
}

type MapBuilder interface {
Build() map[string]*Schema
}

type BuilderMap map[string]Builder

func (m BuilderMap) Build() map[string]*Schema {
s := make(map[string]*Schema, len(m))
for k, v := range m {
s[k] = v.Schema()
}
return s
}

type SchemaMap map[string]*Schema

func (m SchemaMap) Build() map[string]*Schema {
return m
}

func Never() *Schema {
return &Schema{Never: true}
}
Expand Down
10 changes: 5 additions & 5 deletions schema/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func TestProperty(t *testing.T) {

t.Run("valid", func(t *testing.T) {
s := Object().
Properties(map[string]Builder{"a": String(), "b": Number()}).
Properties(BuilderMap{"a": String(), "b": Number()}).
AdditionalProperties(Boolean()).
Schema()

Expand All @@ -122,12 +122,12 @@ func TestProperty(t *testing.T) {

t.Run("anyOf", func(t *testing.T) {
a := Object().
Properties(map[string]Builder{"a": String(), "b": Number()}).
Properties(BuilderMap{"a": String(), "b": Number()}).
AdditionalProperties(Never()).
Schema()

b := Object().
Properties(map[string]Builder{"a": Number(), "b": Boolean()}).
Properties(BuilderMap{"a": Number(), "b": Boolean()}).
AdditionalProperties(Number()).
Schema()

Expand All @@ -141,12 +141,12 @@ func TestProperty(t *testing.T) {

t.Run("oneOf", func(t *testing.T) {
a := Object().
Properties(map[string]Builder{"a": String(), "b": Number()}).
Properties(BuilderMap{"a": String(), "b": Number()}).
AdditionalProperties(Never()).
Schema()

b := Object().
Properties(map[string]Builder{"a": Number(), "b": Boolean()}).
Properties(BuilderMap{"a": Number(), "b": Boolean()}).
AdditionalProperties(Number()).
Schema()

Expand Down
Loading